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

Str 294 update l1 manifest #527

Merged
merged 6 commits into from
Dec 19, 2024
Merged

Str 294 update l1 manifest #527

merged 6 commits into from
Dec 19, 2024

Conversation

bewakes
Copy link
Contributor

@bewakes bewakes commented Dec 9, 2024

Description

This PR adds the epoch information to L1BlockManifest and rescans recent blocks if the scan rules have changed in the new epoch.
This also does some minor refactoring in btcio::query.rs to become more "testing" friendly.

Idea

If there is a new epoch and filtering rules change on that epoch, that means recent blocks needs to be reparsed with the new rule.

But what defines recent blocks?

An epoch change is only identified when the L1 block containing the epoch is finalized. So, between the time the epoch block appears on L1(Say at L1 height h) and the time it is buried and identified in chain, we would have scanned L1 blocks using the previous epoch's rules. This means, we would have scanned blocks from h to current block(at the time of updating new epoch), using the old rules. Those are the "recent blocks".

So, once the rule changes, a L1Event is emitted to revert to height h and reader's state is also updated accordingly.

Assumption

The recent_blocks field in ReaderState holds enough blocks to be able to revert to the point where checkpoint appeared.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature/Enhancement (non-breaking change which adds functionality or enhances an existing one)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactor
  • New or updated tests
  • Dependency Update

Notes to Reviewers

We still have to derive the filter rules from chainstate which will be the part of another Ticket.

Checklist

  • I have performed a self-review of my code.
  • I have commented my code where necessary.
  • I have updated the documentation if needed.
  • My changes do not introduce new warnings.
  • I have added tests that prove my changes are effective or that my feature works.
  • New and existing tests pass with my changes.

Related Issues

STR-716

@bewakes bewakes requested a review from delbonis December 9, 2024 10:28
@bewakes bewakes force-pushed the STR-294-update-l1-manifest branch from 0b49fd1 to 89b8f5c Compare December 9, 2024 11:10
Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Need more rationale for why this new design that tracks epoch transitions was needed. It's rather complicated and the core scanning logic shouldn't need it, since we're only ever using one epoch's rules at a time.

crates/primitives/src/l1.rs Outdated Show resolved Hide resolved
crates/primitives/src/l1.rs Outdated Show resolved Hide resolved
crates/primitives/src/l1.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/query.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/query.rs Outdated Show resolved Hide resolved
@bewakes
Copy link
Contributor Author

bewakes commented Dec 10, 2024

@delbonis, scanning using only one epoch rule is definitely the ideal scenario. But as discussed somewhere in slack, it seems like having to scan blocks(at least the deposit transactions within) requires two(or more probably?) rules to adhere to. That's why I've used the transitions for epochs. I have a feeling that this might still not be sufficient, but I can't see how transitions can be avoided or simplified to use a single rule(at the very least for deposits).

@delbonis
Copy link
Contributor

@bewakes

But as discussed somewhere in slack, it seems like having to scan blocks(at least the deposit transactions within) requires two(or more probably?) rules to adhere to. That's why I've used the transitions for epochs.

Can you DM me a link to this conversation? This doesn't seem right.

@bewakes bewakes force-pushed the STR-294-update-l1-manifest branch from 7501c9c to 53248cb Compare December 11, 2024 09:08
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 74.47552% with 73 lines in your changes missing coverage. Please review.

Project coverage is 56.97%. Comparing base (ed5319c) to head (4148f40).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
crates/btcio/src/reader/query.rs 78.14% 40 Missing ⚠️
crates/primitives/src/l1.rs 40.00% 12 Missing ⚠️
crates/consensus-logic/src/l1_handler.rs 0.00% 5 Missing ⚠️
crates/btcio/src/reader/state.rs 87.50% 3 Missing ⚠️
crates/status/src/status_manager.rs 78.57% 3 Missing ⚠️
crates/btcio/src/status.rs 0.00% 2 Missing ⚠️
crates/consensus-logic/src/genesis.rs 33.33% 2 Missing ⚠️
crates/test-utils/src/bitcoin.rs 60.00% 2 Missing ⚠️
crates/primitives/src/params.rs 0.00% 1 Missing ⚠️
crates/primitives/src/sorted_vec.rs 0.00% 1 Missing ⚠️
... and 2 more
@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   56.68%   56.97%   +0.28%     
==========================================
  Files         303      303              
  Lines       31002    31244     +242     
==========================================
+ Hits        17575    17802     +227     
- Misses      13427    13442      +15     
Files with missing lines Coverage Δ
...rates/consensus-logic/src/csm/client_transition.rs 58.70% <100.00%> (+0.30%) ⬆️
crates/proof-impl/btc-blockspace/src/filter.rs 53.84% <100.00%> (ø)
crates/state/src/chain_state.rs 94.82% <100.00%> (+0.59%) ⬆️
crates/state/src/client_state.rs 53.84% <100.00%> (+2.86%) ⬆️
crates/state/src/l1/header.rs 78.94% <100.00%> (ø)
crates/tx-parser/src/filter.rs 95.53% <100.00%> (ø)
crates/tx-parser/src/messages.rs 31.03% <ø> (ø)
crates/primitives/src/params.rs 14.60% <0.00%> (ø)
crates/primitives/src/sorted_vec.rs 85.93% <0.00%> (ø)
crates/rpc/types/src/types.rs 0.00% <0.00%> (ø)
... and 9 more

... and 4 files with indirect coverage changes

@bewakes bewakes force-pushed the STR-294-update-l1-manifest branch from abddbeb to b25bb01 Compare December 12, 2024 11:13
@bewakes bewakes marked this pull request as ready for review December 12, 2024 11:14
@bewakes bewakes requested review from a team as code owners December 12, 2024 11:14
@bewakes bewakes force-pushed the STR-294-update-l1-manifest branch from b25bb01 to 924e809 Compare December 13, 2024 04:32
@bewakes bewakes enabled auto-merge December 13, 2024 05:00
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

Cosmetic nits

crates/btcio/src/reader/state.rs Outdated Show resolved Hide resolved
crates/primitives/src/l1.rs Outdated Show resolved Hide resolved
crates/rpc/types/src/types.rs Show resolved Hide resolved
@bewakes bewakes force-pushed the STR-294-update-l1-manifest branch from 924e809 to f33a5ec Compare December 16, 2024 04:42
storopoli
storopoli previously approved these changes Dec 16, 2024
Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK f33a5ec

Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

Have some more thorough comments now that this is in a more current state.

crates/btcio/src/reader/query.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/query.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/query.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/state.rs Outdated Show resolved Hide resolved
crates/btcio/src/reader/state.rs Show resolved Hide resolved
crates/primitives/src/l1.rs Show resolved Hide resolved
crates/primitives/src/l1.rs Outdated Show resolved Hide resolved
crates/state/src/chain_state.rs Show resolved Hide resolved
@bewakes
Copy link
Contributor Author

bewakes commented Dec 18, 2024

@delbonis, those are great suggestions! I've updated them accordingly.

Copy link
Member

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

ACK 4148f40

Copy link
Contributor

@delbonis delbonis left a comment

Choose a reason for hiding this comment

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

I think this is fine now.

@bewakes bewakes added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit 5008f3d Dec 19, 2024
19 checks passed
@bewakes bewakes deleted the STR-294-update-l1-manifest branch December 19, 2024 04:55
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.

5 participants