Refactor block/blobs types #4491

henridf · 2023-01-11T10:53:57Z

Use type system to enforce invariant that a pre-4844 block cannot have a sidecar.

This is a first step along the lines of what we discussed in #4454 (comment). This could go be extended further by changing more/all places that take a Opt[blob] to drop the Opt and take a ForkySignedBeaconBlockMaybeBlobs instead (better/shorter name suggestions welcome).

@tersec what do you think?

github-actions · 2023-01-11T13:57:51Z

Unit Test Results

0 files -         9 0 suites - 1 050 0s ⏱️ - 32m 33s
0 tests -   3 502 0 ✔️ -   3 265 0 💤 - 237 0 ❌ ±0
0 runs - 15 194 0 ✔️ - 14 929 0 💤 - 265 0 ❌ ±0

Results for commit 6a282b2. ± Comparison against base commit 46fc571.

♻️ This comment has been updated with latest results.

Use type system to enforce invariant that a pre-4844 block cannot have a sidecar.

tersec · 2023-01-12T21:59:37Z

If you update this to current unstable, it should make the unrelated GitHub Actions windows-amd64 CI failure (https://github.com/status-im/nimbus-eth2/actions/runs/3894352819/jobs/6648236396) less likely:

2023-01-11T18:08:25.2828522Z gcc.exe -c  -w -fmax-errors=3 -mno-ms-bitfields -DWIN32_LEAN_AND_MEAN -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-libbacktrace -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-libbacktrace/install/usr/include -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-secp256k1/secp256k1/../secp256k1_wrapper -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-secp256k1/secp256k1/../secp256k1_wrapper/secp256k1 -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-secp256k1/secp256k1/../secp256k1_wrapper/secp256k1/src -DHAVE_CONFIG_H -DUSE_ASM_X86_64 -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-bearssl/bearssl/abi/../csources/src/ -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-bearssl/bearssl/abi/../csources/inc/ -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-bearssl/bearssl/abi/../csources/tools/ -DBR_USE_WIN32_TIME=1 -DBR_USE_WIN32_RAND=1 -DBR_LE_UNALIGNED=1 -DBR_64=1  -DBR_amd64=1 -DBR_INT128=1 -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nim-zlib/zlib/csources -DHAVE_UNISTD_H -march=native -mno-avx512f -fno-omit-frame-pointer -g3 -Og -gdwarf-3 -O3 -fno-strict-aliasing -fno-ident -fno-lto   -ID:/a/nimbus-eth2/nimbus-eth2/vendor/nimbus-build-system/vendor/Nim/lib -ID:/a/nimbus-eth2/nimbus-eth2/research -o D:/a/nimbus-eth2/nimbus-eth2/nimcache/release/state_sim/sqlite3.c.o D:/a/nimbus-eth2/nimbus-eth2/vendor/nim-sqlite3-abi/sqlite3.c
2023-01-11T18:08:40.9149973Z 
2023-01-11T18:08:40.9152404Z cc1.exe: out of memory allocating 82368 bytes
2023-01-11T18:08:40.9579338Z mingw32-make[1]: *** [nimcache/release/state_sim/state_sim.makefile:911: D:/a/nimbus-eth2/nimbus-eth2/nimcache/release/state_sim/sqlite3.c.o] Error 1
2023-01-11T18:08:41.0566196Z mingw32-make: *** [Makefile:308: state_sim] Error 2
2023-01-11T18:08:41.0566815Z mingw32-make: *** Waiting for unfinished jobs....

via #4495

…bs-invariant

henridf · 2023-01-12T22:05:58Z

If you update this to current unstable

done

beacon_chain/nimbus_beacon_node.nim

Co-authored-by: tersec <tersec@users.noreply.github.com>

tersec · 2023-01-13T17:12:26Z

beacon_chain/rpc/rest_beacon_api.nim

+          let signedBlockAndBlobs =
+            when blck is eip4844.SignedBeaconBlock:
+              # TODO: Fetch blobs from EE
+              eip4844.SignedBeaconBlockAndBlobsSidecar(


Similar post-EIP4844 issue as in the other PR -- this will fall back to just blck, without the sidecar, unless someone notices specifically by auditing all the usages of eip4844.SignedBeaconBlock or otherwise.

The exact fallout of this depends on what uses which types.

https://github.com/henridf/nimbus-eth2/blob/6feb0bb34fc3b28898826c13dcc07971c683a4d9/beacon_chain/validators/message_router.nim#L85-L87

proc routeSignedBeaconBlock*( router: ref MessageRouter, blckAndBlobs: ForkySignedBeaconBlockMaybeBlobs): Future[RouteBlockResult] {.async.} =

Currently this PR has

ForkySignedBeaconBlockMaybeBlobs* = phase0.SignedBeaconBlock | altair.SignedBeaconBlock | bellatrix.SignedBeaconBlock | capella.SignedBeaconBlock | eip4844.SignedBeaconBlockAndBlobsSidecar

which means that it will result in a compile-time error, which is good -- it'll alert people that this is maybe not the intended use of that function.

However, the ForkySignedBeaconBlockMaybeBlobs type looks potentially usable as a more general-purpose function.

Maybe making it clearer, via its name or some in-line comments/documentation, that it's meant for gossip-routable and similar blocks, and it would be incorrect to add plain eip4844.SignedBeaconBlock (or, probably, future, forks' SignedBeaconBlocks) to it.

Then, one can safely use this when foo is eip4844.SignedBeaconBlock construction safely.

Maybe making it clearer, via its name or some in-line comments/documentation

Will add a comment. Re name, I've had a hard time coming up with a more descriptive (not to mention shorter) name. Will noodle on it some more.

However, the ForkySignedBeaconBlockMaybeBlobs type looks potentially usable as a more general-purpose function.

I don't think I follow - it's a type, but could be a function - maybe you could sketch the signature of the function you have in mind?

Oops, you're right, mis-typed. Not "function", but "type".

Yeah, a more general-purpose type. That is, per above, as already discussed, because it doesn't specify that it's specifically crafted around the requirements of block gossip, it's potentially tempting to reuse or extend that type for other purposes in a way which creates potential bugs for its original purpose.

Similar post-EIP4844 issue as in the other PR -- this will fall back to just blck, without the sidecar, unless someone notices specifically by auditing all the usages of eip4844.SignedBeaconBlock or otherwise.

I tried addressing this with the init* constructor approach you outlined below but... the fundamental issue is still that we do have to explicitly check here (either literally here, at L806, or in some nearby function call) if the incoming block is eip4844.SignedBeaconBlock, in order to know that we need to fetch the corresponding blobs. To call the init* templates below, we'd need to have already determined if this is a eip4844 block.

So I guess the solution here (at least for the specific issue of avoiding silent future failure) would be static: doAssert high(BeaconStateFork) == BeaconStateFork.EIP4844 as you indicated in the other PR.

Unless I'm missing something?

template init*(blck: phase0.SignedBeaconBlock): ForkySignedBeaconBlockMaybeBlobs = blck template init*(blck: altair.SignedBeaconBlock): ForkySignedBeaconBlockMaybeBlobs = blck template init*(blck: bellatrix.SignedBeaconBlock): ForkySignedBeaconBlockMaybeBlobs = blck template init*(blck: capella.SignedBeaconBlock): ForkySignedBeaconBlockMaybeBlobs = blck template init*(blck: eip4844.SignedBeaconBlock, blobs: eip4844.BlobsSidecar): ForkySignedBeaconBlockMaybeBlobs = eip4844.SignedBeaconBlockAndBlobsSidecar( beacon_block: blck, blobs_sidecar: blobs )

Sure, then I agree that static: doAssert high(BeaconStateFork) == BeaconStateFork.EIP4844 is the best available approach.

Actually - the high(BeaconStateFork) == BeaconStateFork.EIP4844 pattern is for enums, not generic types.

And returning to the root issue:

Similar post-EIP4844 issue as in the other PR -- this will fall back to just blck, without the sidecar

I think this isn't an issue here? If a new SignedBeaconBlock type is added, the ForkySignedBeaconBlockMaybeBlobs generic isn't extended to included it, that should be result compilation error at any place (for example, a function call) where that new SignedBeaconBlock type is used in lieu of a ForkySignedBeaconBlockMaybeBlobs. Conversely, if I remove one of the types from ForkySignedBeaconBlockMaybeBlobs, I get a "type mismatch" compilation error.

The high(BeaconStateFork) == BeaconStateFork.EIP4844 pattern doesn't really depend on what it's guarding -- it's a generic compile-time mechanism to flag that a particular section of code depends in some way not otherwise found in a compile-time or maybe runtime way on the particular forks known. It can be used just as readily for code based on generic types as enums, though typically when there are generic types, there are better approaches. For most specific cases, indeed, there are better methods, but it's a fallback approach.

And, yes, what you note, that if "a new SignedBeaconBlock type is added, the ForkySignedBeaconBlockMaybeBlobs generic isn't extended to included it" is true by default, if people remain aware that it's supposed to match the gossip types, not some other potentially interesting use of a type called ForkySignedBeaconBlockMaybeBlobs`

To be a bit more explicit, the sequence of events I'm concerned about, arrayed over months/years, is:

this PR gets merged, without better in-line documentation (comments, naming of type, some static: doAssert not default(eip4844.SignedBeaconBlock) is ForkySignedBeaconBlockMaybeBlobs thing, etc)

someone else, unaware of the very specific purpose of the shape of ForkySignedBeaconBlockMaybeBlobs, either (unlikely) for EIP4844, or (more likely) more-or-less mechanically adding on to this type later, doesn't know that they shouldn't add the bare SignedBeaconBlock for EIP4844 or later, and only the with-blobs version. This will not trigger any immediate error or compilation warning -- it'll just be one more possible type that type class can match.

There's still no full bug here, but it's primed and one step away:

then, later yet, someone changes the routing or other functions which take a ForkySignedBeaconBlockMaybeBlobs, and now they also (accidentally, from step 2) take a bare SignedBeaconBlock type from an EIP48844-or-later fork, and, e.g., try to broadcast it. That's a bug. Currently, the type system will catch this category of bugs, but after this PR, it might not.

(2) and (3) are more likely to happen specifically in the next couple of hardforks after EIP4844 -- enough of them, and the pattern of only the with-blobs version would become obvious, but after the first one, it might look like just an oversight or omission.

It's not a current bug, and the code is sort of fine as it is in a narrow way, but it's a potential future maintenance hazard by weakening the type checking guarantees if someone without full understanding of a the reason/context for the code to via steps (2)/(3), maybe different people even for each, induce a bug.

high(BeaconStateFork) == BeaconStateFork.EIP4844 [..] can be used just as readily for code based on generic types as enums

What's the equivalent of "high" for generic types? it must be staring at me in the eyes, but I don't see one.

(comments, naming of type, some static: doAssert not default(eip4844.SignedBeaconBlock) is ForkySignedBeaconBlockMaybeBlobs thing, etc)

Added a comment and assert in c9232d0.

Re name, the best I'm coming up with is GossipSignedBeaconBlockMaybeBlobs. That's a tad more descriptive and emphasises the gossip-oriented motivation for this type, but on the other hand there's a strong discipline here (as commented at the top of forks.nim) that generic types for dealing with forks are Forky*, so I'm not sure why this particular case should deviate from that. Happy to change if you think it's worthwhile.

Re name, the best I'm coming up with is GossipSignedBeaconBlockMaybeBlobs. That's a tad more descriptive and emphasises the gossip-oriented motivation for this type, but on the other hand there's a strong discipline here (as commented at the top of forks.nim) that generic types for dealing with forks are Forky*, so I'm not sure why this particular case should deviate from that. Happy to change if you think it's worthwhile.

Agree. With the other changes, the existing name is good.

beacon_chain/rpc/rest_beacon_api.nim

beacon_chain/gossip_processing/gossip_validation.nim

henridf · 2023-01-15T13:37:34Z

At a high-level my takeaway given all this discussion is that this thing isn't (yet?) structured properly. I guess the question is, is it an improvement over status quo (#4454 (comment)) ? Doesn't seem like an obvious yes.

tersec · 2023-01-15T14:05:13Z

beacon_chain/spec/forks.nim

  ForkySignedBeaconBlockMaybeBlobs* =
    phase0.SignedBeaconBlock |
    altair.SignedBeaconBlock |
    bellatrix.SignedBeaconBlock |
    capella.SignedBeaconBlock |
    eip4844.SignedBeaconBlockAndBlobsSidecar
+  # This type should only contain types that are gossiped.
+  static: doAssert not default(eip4844.SignedBeaconBlock) is ForkySignedBeaconBlockMaybeBlobs


This can't be put in a type section -- everything here is implicitly type Foo, e.g., ForkySignedBeaconBlockMaybeBlobs* = ...

c9232d0 addresses this

tersec · 2023-01-15T14:06:48Z

At a high-level my takeaway given all this discussion is that this thing isn't (yet?) structured properly. I guess the question is, is it an improvement over status quo (#4454 (comment)) ? Doesn't seem like an obvious yes.

With your latest commit (fixed by moving it outside the type section so it's not trying to parse it as a type definition or declaration), I do view it as an improvement over the status quo, yeah.

Refactor block/blobs types

48288c7

Use type system to enforce invariant that a pre-4844 block cannot have a sidecar.

henridf force-pushed the type-refactor-blobs-invariant branch from 0131a87 to 48288c7 Compare January 11, 2023 14:10

Merge branch 'unstable' into type-refactor-blobs-invariant

488ff1d

Merge remote-tracking branch 'origin/unstable' into type-refactor-blo…

76970bc

…bs-invariant

henridf added 2 commits January 13, 2023 09:18

Merge branch 'unstable' into type-refactor-blobs-invariant

d2223d0

Merge branch 'unstable' into type-refactor-blobs-invariant

cbf21ea

tersec reviewed Jan 13, 2023

View reviewed changes

beacon_chain/nimbus_beacon_node.nim Outdated Show resolved Hide resolved

Update beacon_chain/nimbus_beacon_node.nim

6feb0bb

Co-authored-by: tersec <tersec@users.noreply.github.com>

tersec reviewed Jan 13, 2023

View reviewed changes

beacon_chain/rpc/rest_beacon_api.nim Show resolved Hide resolved

tersec reviewed Jan 13, 2023

View reviewed changes

beacon_chain/gossip_processing/gossip_validation.nim Show resolved Hide resolved

henridf marked this pull request as ready for review January 15, 2023 10:44

review feedback

c9232d0

tersec reviewed Jan 15, 2023

View reviewed changes

henridf mentioned this pull request Jan 16, 2023

Implement EIP-4844 #4395

Closed

21 tasks

tersec approved these changes Jan 16, 2023

View reviewed changes

tersec enabled auto-merge (squash) January 16, 2023 11:58

Merge branch 'unstable' into type-refactor-blobs-invariant

aea04bd

tersec merged commit 727920a into status-im:unstable Jan 16, 2023

henridf deleted the type-refactor-blobs-invariant branch February 6, 2023 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor block/blobs types #4491

Refactor block/blobs types #4491

henridf commented Jan 11, 2023

github-actions bot commented Jan 11, 2023 •

edited

Loading

tersec commented Jan 12, 2023

henridf commented Jan 12, 2023

tersec Jan 13, 2023

henridf Jan 13, 2023

henridf Jan 13, 2023

tersec Jan 13, 2023

henridf Jan 13, 2023 •

edited

Loading

tersec Jan 13, 2023

henridf Jan 15, 2023

tersec Jan 15, 2023

henridf Jan 15, 2023 •

edited

Loading

tersec Jan 16, 2023

henridf commented Jan 15, 2023

tersec Jan 15, 2023

tersec Jan 16, 2023

tersec commented Jan 15, 2023

Refactor block/blobs types #4491

Refactor block/blobs types #4491

Conversation

henridf commented Jan 11, 2023

github-actions bot commented Jan 11, 2023 • edited Loading

Unit Test Results

tersec commented Jan 12, 2023

henridf commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridf Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridf Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henridf commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tersec commented Jan 15, 2023

github-actions bot commented Jan 11, 2023 •

edited

Loading

henridf Jan 13, 2023 •

edited

Loading

henridf Jan 15, 2023 •

edited

Loading