Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for L1 to L2 message log syncing #2251

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

t00ts
Copy link
Contributor

@t00ts t00ts commented Sep 19, 2024

Introduces functionality to sync L1 to L2 message logs, improving our ability to track cross-chain interactions.

Key Changes

  1. Added get_logs method to the Ethereum client:
    • Allows fetching logs for a specified block range
    • Implements recursive fetching with automatic range splitting for large queries
  2. Implemented bulk syncing of L1ToL2MessageLog:
    • Fetches logs from the last synced L1 block up to the current state
    • Stores these logs for later matching with L2 transactions
  3. Enhanced L1 sync process:
    • Bulk fetches logs at the beginning of execution
    • Waits for L2 blocks and matches l1_handler transactions with corresponding L1 transaction hashes

@t00ts t00ts added this to the JSON-RPC 0.8.0 milestone Sep 19, 2024
@t00ts t00ts force-pushed the t00ts/messaging branch 5 times, most recently from 3fcee20 to fc4b2eb Compare September 23, 2024 09:08
@t00ts t00ts marked this pull request as ready for review September 23, 2024 09:09
@t00ts t00ts requested a review from a team as a code owner September 23, 2024 09:09
CHANGELOG.md Outdated Show resolved Hide resolved
crates/common/src/lib.rs Outdated Show resolved Hide resolved
crates/ethereum/src/lib.rs Outdated Show resolved Hide resolved
crates/ethereum/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kkovaacs kkovaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a quite a few comments, most of them in these categories:

  • using structured fields in tracing macros: https://docs.rs/tracing/latest/tracing/index.html#recording-fields
  • we should be adding a newtype for L1 block number because we seem to be mixing L1/L2 block numbers at a few places
  • using storage row accessors and type conversions for L1 block number / L1 block hash instead of ad-hoc conversions in storage code

crates/ethereum/src/lib.rs Show resolved Hide resolved
crates/pathfinder/src/state/sync.rs Show resolved Hide resolved
@@ -686,8 +718,42 @@ async fn consumer(
}
}
L1ToL2Message(msg) => {
tracing::trace!("Got a new L1 to L2 message log: {:?}", msg);
// todo!()
tracing::debug!("Got an L1ToL2Message: {:?}", msg.message_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::debug!("Got an L1ToL2Message: {:?}", msg.message_hash);
tracing::trace!(message_hash=%msg.message_hash, "Got an L1ToL2Message");

..
}) = tx.fetch_l1_to_l2_message_log(&msg.message_hash)?
{
tracing::debug!("Found L2 tx for L1 Tx {:?}", l1_tx_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::debug!("Found L2 tx for L1 Tx {:?}", l1_tx_hash);
tracing::trace!(%l1_tx_hash, %l2_tx_hash, "Found L2 tx for L1 tx);

}
// Otherwise, we insert the message log with an empty L2 tx hash
else {
tracing::debug!("L2 tx NOT found for L1 Tx {:?}", l1_tx_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::debug!("L2 tx NOT found for L1 Tx {:?}", l1_tx_hash);
tracing::trace!(%l1_tx_hash, "L2 tx NOT found for L1 tx");

let raw_data = stmt
.query_row(params![&message_hash.as_bytes().to_vec()], |row| {
Ok((
row.get::<_, Option<u64>>(0)?,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
row.get::<_, Option<u64>>(0)?,
row.get_optional_block_number(0)?,

This is an L1 block number so we should just use an the newtype for L1 block numbers.

Comment on lines +66 to +75
let debug_tx_str = match (&data.1, &data.2) {
(Some(_), None) => "[L1, X]",
(None, Some(_)) => "[X, L2]",
_ => "N/A",
};
tracing::debug!(
"Fetched (and found: {}) an L1 to L2 message log for {:?}",
debug_tx_str,
message_hash
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let debug_tx_str = match (&data.1, &data.2) {
(Some(_), None) => "[L1, X]",
(None, Some(_)) => "[X, L2]",
_ => "N/A",
};
tracing::debug!(
"Fetched (and found: {}) an L1 to L2 message log for {:?}",
debug_tx_str,
message_hash
);
tracing::trace!(
?message_hash,
l1_tx_hash=?data.1,
l2_tx_hash=%data.2,
"Fetched an L1 to L2 message log"
);

)
.context("Removing L1 to L2 message log")?;

tracing::trace!("Removed L1 to L2 message log: {:?}", message_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracing::trace!("Removed L1 to L2 message log: {:?}", message_hash);
tracing::trace!(?message_hash, "Removed L1 to L2 message log);

}
_ => BlockNumber::new_or_panic(0),
};
Ok(l1_handler_block.unwrap_or(default_block_number))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not sure I follow. hightest_block_with_l1_handler_tx() gets us the L1 block number for the latest L1 block where we have already found a matching L1 handler transaction in an L2 block.

I think that's a bit more pessimistic than what we'd need here. Ideally we should be just making sure that we're continuing from the block for which we've already fetched L1->L2 message logs, independently if there was a matching transaction on L2?

l1_tx_hash BLOB NOT NULL,
l2_tx_hash BLOB NOT NULL
);
CREATE INDEX idx_l1_handler_txs_l1_tx_hash ON l1_handler_txs(l1_tx_hash);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought: the specification seems to require us returning the L2 transaction statuses for an L1 transaction hash in sending order. We do not currently store any information related to that -- we probably should?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants