-
Notifications
You must be signed in to change notification settings - Fork 20
feat: run the l1 info tree syncer with safe block finality
#1148
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
feat: run the l1 info tree syncer with safe block finality
#1148
Conversation
852e734 to
f92a988
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors hash computation to use the standard crypto.Keccak256Hash function instead of manual sha3 implementations, removes an unused field from EVMDownloader, changes pointer receivers to value receivers where appropriate, and adds a new GetLatestL1InfoGER method to directly retrieve the Global Exit Root without fetching the entire leaf object.
- Replaces manual sha3.NewLegacyKeccak256() implementations with crypto.Keccak256Hash calls
- Removes unused
finalizedBlockTypefield from EVMDownloader struct - Adds GetLatestL1InfoGER method to efficiently retrieve only the GER without the full leaf
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types/block_finality.go | Changed BlockNumber method receiver from pointer to value |
| tree/tree.go | Replaced manual sha3 hashing with crypto.Keccak256Hash, removed sha3 import |
| sync/evmdownloader.go | Removed unused finalizedBlockType field from struct |
| l1infotreesync/processor.go | Replaced manual sha3 hashing with crypto.Keccak256Hash in CalculateGER, added GetLatestL1InfoGER method |
| l1infotreesync/processor_test.go | Added tests for new GetLatestL1InfoGER functionality and CalculateGER, removed unnecessary loop variable shadowing |
| l1infotreesync/l1infotreesync.go | Added GetLatestL1InfoGER public method, fixed grammar in comment |
| l1infotreesync/l1infotreesync_test.go | Added test for GetLatestL1InfoGER error handling |
| l1infotreesync/e2e_test.go | Updated loop syntax to range expression, added GetLatestL1InfoGER verification |
| go.mod | Moved golang.org/x/crypto from direct to indirect dependency |
| cmd/run.go | Renamed parameters for clarity, changed finality type from FinalizedBlock to SafeBlock, changed type to interface |
| aggoracle/oracle.go | Updated L1InfoTreeSyncer interface to use GetLatestL1InfoGER instead of GetLatestL1InfoLeaf |
| aggoracle/e2e_test.go | Updated loop syntax to range expression |
592185e to
631885a
Compare
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
631885a to
30af17d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The coverage is below 80%
| DBPath = "{{PathRWData}}/L1InfoTreeSync.sqlite" | ||
| GlobalExitRootAddr = "{{L1NetworkConfig.GlobalExitRootManagerAddr}}" | ||
| RollupManagerAddr = "{{L1NetworkConfig.RollupManagerAddr}}" | ||
| BlockFinality = "SafeBlock" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure that we want Safe as default? the situation of changing to Safe is for always or just an specific use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original requirement was to make the safe, in order to make the aggoracle runs faster (and consequentially be able to inject GERs at a higher frequency). Also I understand this is just an intermediary approach until syncers and reorg detector is refactored. Note that, in the near future, we are aiming to have GERs injected on every 6th L1 block, which is considered as good enough (and thus probably unlikely to be reorged). As you can see on the Etherscan, the reorgs are happening, but are often pretty shallow (most often the fork is only 1 block deep)
As it is highly unlikely that the safe block gets reorged, I think it makes sense to run on this BlockFinality by default. If for any reason we experience some issues, we can always override it through a config.
IMO it would be an overkill at this point to refactor this, and create a separate configuration for the aggoracle on the Kurtosis CDK and what not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in for setting this as Default, the "highly unlikely" in the safe definition is quite high.
No, safe blocks have not been reorged in Ethereum. A "safe" block is one that has received attestations from two-thirds of the validator set, making it highly unlikely to be reorganized under normal network conditions. Reorganizations of "safe" blocks would require a catastrophic attack on the network, as it is understood to be crypto-economically secure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cd09d6a to
6bbe2a6
Compare
|



🔄 Changes Summary
safe, in order to makeaggoracleruns faster. Re-introduceBlockFinalityin theL1InfoTreeSyncerconfig and consequently theReorgDetector.keccak256hash calculations, by relying on go ethereum library.URLRPCL1config parameters fromAggoracleandL1InfoTreeSync, since they are unused.ReadTreer– provides read-only access to tree data (roots, leaves, proofs).LeafWriter– defines write operations for adding new leaves to the tree.ReorganizeTreer– extends ReadTreer with the ability to handle blockchain reorgs.FullTreer– combines all capabilities (ReadTreer,LeafWriter,ReorganizeTreer), representing a fully functional, mutable Merkle tree.N/A
📋 Config Updates
L1InfoTreeSync.BlockFinalityconfig param.Aggoracle.URLRPCL1config param.L1InfoTreeSync.URLRPCL1config param.✅ Testing
🐞 Issues
🔗 Related PRs
N/A
📝 Notes
N/A