Skip to content

Conversation

@arnetheduck
Copy link
Member

@arnetheduck arnetheduck commented Sep 20, 2025

There are several assumptions in the code about the clearance state that lead to fragility - resolve some of these by moving things around a bit

  • advance clearance state together with proposal fcU
  • pass state in OnBlockAdded to emphasize the lifetime of its validity
  • remove some code duplication between block_processor / sync_overseer (there's a lot more to do but it's scheduled for dedicated refactoring so do just the minimum)
  • move blob/column validation to its own proc shared between backfill/head blocks

@github-actions
Copy link

github-actions bot commented Sep 20, 2025

Unit Test Results

       15 files  ±0    3 005 suites  ±0   1h 19m 5s ⏱️ + 3m 5s
11 390 tests ±0  10 828 ✔️ ±0  562 💤 ±0  0 ±0 
72 956 runs  ±0  72 164 ✔️ ±0  792 💤 ±0  0 ±0 

Results for commit 401d3d5. ± Comparison against base commit 2a57434.

♻️ This comment has been updated with latest results.

proc verifySidecars(
signedBlock: ForkySignedBeaconBlock,
blobsOpt: Opt[BlobSidecars],
dataColumnsOpt: Opt[DataColumnSidecars],
Copy link
Contributor

Choose a reason for hiding this comment

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

This function signature combines blobsOpt and columnsOpt, but knows (because Forky) statically at compile-time which it has. It's strictly an error to provide non-none columnsOpt to pre-Fulu, and likewise non-none blobsOpt to post-Fulu.

blobsOpt: Opt[BlobSidecars],
dataColumnsOpt: Opt[DataColumnSidecars],
) =
doAssert not (blobsOpt.isSome and dataColumnsOpt.isSome),
Copy link
Contributor

Choose a reason for hiding this comment

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

Both callers of this, storeBlock and storeBackfillBlock, know at compile time which of these is true, and this is straightforward to set up to be an error that can't occur, statically at compile-time, by only allowing the correct kind of sidecar to be provided (or not provided, per Opt).

Copy link
Member Author

@arnetheduck arnetheduck Sep 21, 2025

Choose a reason for hiding this comment

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

know at compile time which of these is true

sort of - they both work from two Opt instances whose contents are managed at runtime (also because that's how it's stored in the queue etc and the only question is where to put the if to pick one of them per fork (and discard / assert against the other) - if you look at https://github.com/status-im/nimbus-eth2/pull/7518/files#diff-a09c99ea4dc7b4118405b5b5c2985124f3ac10247f3d69c9611c22ca7c496f48L389, it does the same runtime->compile-time selection as this code but without the extra sanity checking only for storeBackfillBlock, and because it's placed at the "outer" layer, we end up duplicating all logic that does not differ between the forks. It would be much worse to approach it the same way for storeBlock which has a lot more shared logic. I don't see the gain of doing it this way.

The aim of this PR is really to make the two store functions uniform and avoid the two slightly different ways of checking sidecars.

If we want this properly fixed, what should be done is essentially Opt[ForkedSidecar] that only allows one of the two sidecar types to be present in the code, per fork. This is the only clean way really to achieve what you're after, ie to make the shape of the data structures match the ongoings of the protocol - that's a much bigger change which will be easier to do after this PR - because once you've done that change, the code will just overload verifySidecar - ie the refactoring that makes verifySidecar a separate function is still useful in that world.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are several assumptions in the code about the clearance state that
lead to fragility - resolve some of these by moving things around a bit

* advance clearance state together with proposal fcU
* pass state in `OnBlockAdded` to emphasize the lifetime of its validity
* remove some code duplication between block_processor / sync_overseer
(there's a lot more to do but it's scheduled for dedicated refactoring
so do just the minimum)
* move blob/column validation to its own proc shared between
backfill/head blocks
* avoid pointless block lookup from dag when processing block
attestations
@siddarthkay siddarthkay force-pushed the unstable branch 2 times, most recently from 6c84d01 to b118196 Compare September 24, 2025 12:34
@tersec tersec merged commit 5cc18c3 into unstable Sep 25, 2025
12 checks passed
@tersec tersec deleted the clearance-state branch September 25, 2025 22:28
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.

2 participants