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

chore: Move optimism root checks into crates/optimism #10932

Closed
wants to merge 1 commit into from

Conversation

garwahl
Copy link
Contributor

@garwahl garwahl commented Sep 16, 2024

WIP for #10713

Moves calculate_receipt_root_no_memo_optimism and associated usages from crates/primitive to crates/optimism

@joshieDo joshieDo added A-consensus Related to the consensus engine A-op-reth Related to Optimism and op-reth labels Sep 16, 2024
@garwahl
Copy link
Contributor Author

garwahl commented Sep 18, 2024

Hey @mattsse where should Receipts::optimism_root_slow() be moved to? Do you have a preference?

Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

make sense for it to live next to this function

/// Calculates the receipt root for a header.
pub(crate) fn calculate_receipt_root_optimism(
receipts: &[ReceiptWithBloom],
chain_spec: &ChainSpec,
timestamp: u64,
) -> B256 {
// There is a minor bug in op-geth and op-erigon where in the Regolith hardfork,
// the receipt root calculation does not include the deposit nonce in the receipt
// encoding. In the Regolith Hardfork, we must strip the deposit nonce from the
// receipts before calculating the receipt root. This was corrected in the Canyon
// hardfork.
if chain_spec.is_fork_active_at_timestamp(OptimismHardfork::Regolith, timestamp) &&
!chain_spec.is_fork_active_at_timestamp(OptimismHardfork::Canyon, timestamp)
{
let receipts = receipts
.iter()
.cloned()
.map(|mut r| {
r.receipt.deposit_nonce = None;
r
})
.collect::<Vec<_>>();
return ordered_trie_root_with_encoder(receipts.as_slice(), |r, buf| {
r.encode_inner(buf, false)
})
}
ordered_trie_root_with_encoder(receipts, |r, buf| r.encode_inner(buf, false))
}

@emhane
Copy link
Member

emhane commented Sep 18, 2024

although, @garwahl, this is a method on Receipts. Ideally we need to implement it on an op receipts type. this issue is likely blocked by bigger sdk issue #7649. The problem by defining a trait inside of the reth-primitives crate with trait method root_slow, is that we won't be able to implement it in reth-optimism-consensus on an alloy type, and the end goal is to use alloy primitive data types throughout the codebase, injected via top level node builder.

@garwahl
Copy link
Contributor Author

garwahl commented Sep 19, 2024

although, @garwahl, this is a method on Receipts. Ideally we need to implement it on an op receipts type. this issue is likely blocked by bigger sdk issue #7649. The problem by defining a trait inside of the reth-primitives crate with trait method root_slow, is that we won't be able to implement it in reth-optimism-consensus on an alloy type, and the end goal is to use alloy primitive data types throughout the codebase, injected via top level node builder.

To clarify, is there any benefit in continuing to extract things out into crates/optimism/* given you think ultimately these will move into alloy types? Am I understanding that correctly that the rollup specific code will be shifted into Alloy?

@emhane
Copy link
Member

emhane commented Sep 19, 2024

although, @garwahl, this is a method on Receipts. Ideally we need to implement it on an op receipts type. this issue is likely blocked by bigger sdk issue #7649. The problem by defining a trait inside of the reth-primitives crate with trait method root_slow, is that we won't be able to implement it in reth-optimism-consensus on an alloy type, and the end goal is to use alloy primitive data types throughout the codebase, injected via top level node builder.

To clarify, is there any benefit in continuing to extract things out into crates/optimism/* given you think ultimately these will move into alloy types? Am I understanding that correctly that the rollup specific code will be shifted into Alloy?

not quite, just that this pr will be blocked at some point. either the whole consensus auto seal code has to move into op crate + needs to be used via the op crate too, or we need to do smthg like #10997 and add a trait method receipt_root_slow to an alloy receipt trait. things that still need design.

@garwahl
Copy link
Contributor Author

garwahl commented Sep 20, 2024

although, @garwahl, this is a method on Receipts. Ideally we need to implement it on an op receipts type. this issue is likely blocked by bigger sdk issue #7649. The problem by defining a trait inside of the reth-primitives crate with trait method root_slow, is that we won't be able to implement it in reth-optimism-consensus on an alloy type, and the end goal is to use alloy primitive data types throughout the codebase, injected via top level node builder.

To clarify, is there any benefit in continuing to extract things out into crates/optimism/* given you think ultimately these will move into alloy types? Am I understanding that correctly that the rollup specific code will be shifted into Alloy?

not quite, just that this pr will be blocked at some point. either the whole consensus auto seal code has to move into op crate + needs to be used via the op crate too, or we need to do smthg like #10997 and add a trait method receipt_root_slow to an alloy receipt trait. things that still need design.

No worries at all, let me know if I can lend a hand, would love to jump into the alloy repo as well if you see anything

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sorry for not reviewing this in time...

this has since been done and wasn't that easy

@mattsse mattsse closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Related to the consensus engine A-op-reth Related to Optimism and op-reth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants