modularise message routing for sidecars by agnxsh · Pull Request #7751 · status-im/nimbus-eth2

agnxsh · 2025-11-23T22:03:40Z

No description provided.

github-actions · 2025-11-23T23:29:06Z

Unit Test Results

      12 files ±0   2 440 suites ±0 49m 34s ⏱️ - 14m 50s
12 668 tests ±0   9 191 ✔️ ±0   3 477 💤 ±0 0 ❌ ±0
63 856 runs ±0 51 426 ✔️ ±0 12 430 💤 ±0 0 ❌ ±0

Results for commit a3c1040. ± Comparison against base commit 791cb3a.

♻️ This comment has been updated with latest results.

beacon_chain/validators/message_router.nim

arnetheduck · 2025-11-24T08:07:44Z

beacon_chain/validators/message_router.nim

+    const finalSidecars = noSidecars
+  elif someSidecarsOpt is NoSidecarsAtFork and
+       typeof(blck).kind in ConsensusFork.Deneb..ConsensusFork.Electra:
+    let finalSidecars = Opt.none(BlobSidecars)


why is there no routing here?

the routing is called in the else block, apart from that, NoSidecarsAtFork is trying to capture all cases where we are in sidecar fork but there's no sidecar proposed for that block.

so .. why would we call this function with NoSideCarsAtFork and block from a fork that has sidecars?

basically this should be when someSidecarsOpt is NoSidecarsAtFork: nosidecars else: await publishSidecars(router, blck, someSidecarsOpt)

or add a noop overload for publishSidecars(..., NoSidecars)

okay, plausible

arnetheduck · 2025-11-24T08:11:26Z

beacon_chain/rpc/rest_beacon_api.nim

              forkyBlck, Opt.some(
                forkyBlck.create_blob_sidecars(kzg_proofs, blobs)),
-              Opt.none(seq[fulu.DataColumnSidecar]),
              checkValidator = true)


why not overload routeSignedBeaconBlock too?

we would have to re-write validate block and publish block part in each of the overloads, unless we add another function adapter of sorts, apart from that, there's no reason to not have it.

this looks ok too to me, i'm trying to assess what's the benefit of the overload.

beacon_chain/validators/message_router.nim

tersec · 2025-11-24T12:44:44Z

beacon_chain/validators/message_router.nim

+    NoSidecarsAtFork |
+    Opt[seq[BlobSidecar]] |
+    Opt[seq[fulu.DataColumnSidecar]] |
+    Opt[seq[gloas.DataColumnSidecar]]


This PR doesn't seem to treat gloas.DataColumnSidecar at all?

I'm trying to manage this in a follow up pr, to keep gloas changes atomic

beacon_chain/validators/message_router.nim

tersec · 2026-01-19T09:11:41Z

beacon_chain/gossip_processing/block_processor.nim

            signature = shortLog(signedBlock.signature),
            msg = r.error()
          return err(VerifierError.Invalid)
-  elif consensusFork in ConsensusFork.Phase0 .. ConsensusFork.Capella:


Removing this and replacing it with

when sidecarsOpt is NoSidecars: static: doAssert consensusFork in ConsensusFork.Phase0 .. ConsensusFork.Capella

means that if one tries to call verifySidecars(ConsensusFork.Phase0 through Capella, but not NoSidecars, it will now fall into the Unknown consensus fork` error message case, a bit misleadingly.

tersec · 2026-01-19T09:12:56Z

beacon_chain/networking/eth2_network.nim


 proc broadcastDataColumnSidecar*(
-    node: Eth2Node, subnet_id: uint64, data_column: fulu.DataColumnSidecar):
+    node: Eth2Node, subnet_id: uint64,


Why split this line? it's 76 characters total

tersec · 2026-01-19T09:14:12Z

beacon_chain/networking/eth2_network.nim


+proc broadcastDataColumnSidecar*(
+    node: Eth2Node, subnet_id: uint64,
+    data_column: gloas.DataColumnSidecar):


Same as the fulu one, sort of, regarding splitting across two lines -- it's be 77 characters as a single line, which is fine.

Ok, why does nph do this?

tersec · 2026-01-19T09:22:50Z

beacon_chain/rpc/rest_beacon_api.nim

            return RestApiResponse.jsonError(Http400, InvalidBlockObjectError)

-          when consensusFork in [ConsensusFork.Deneb, ConsensusFork.Electra]:
+          when consensusFork == ConsensusFork.Gloas:


In H fork, which we'll start development on sometime in the next few months, this will silently resolve to the pre-Deneb case, which is incorrect. Hopefully other compiletime checks pick it up, but regardless this isn't correct. We have at leasttwo basic approaches to this:

Use when consensusFork >= ConsensusFork.Gloas: when we're pretty sure that H fork has a good chance of still using Gloas datacolumns. It slightly a bet, so it does depend; or

If we want to express no view on the likely future of Gloas datacolumns, directly becore when consensusFork == ConsensusFork.Gloas: insert a static: doAssert high(ConsensusFork) == ConsensusFork.Gloas to signal to whoever does this that they have to specifically verify that the code is updated for H fork correctly.

But this has the failure mode of potentially silently breaking in H fork.

tersec · 2026-01-19T09:26:10Z

beacon_chain/rpc/rest_beacon_api.nim

-          await node.router.routeSignedBeaconBlock(
-            forkyBlck, Opt.none(seq[BlobSidecar]),
-            Opt.none(seq[fulu.DataColumnSidecar]), checkValidator = true)
+          when consensusFork >= ConsensusFork.Bellatrix:


How can consensusFork >= ConsensusFork.Bellatrix possibly be true here? it's the else clause directly after

elif consensusFork >= ConsensusFork.Bellatrix: return RestApiResponse.jsonError( Http400, $consensusFork & " builder API unsupported") else: # ... this code is here ...

They're on separate objects (currentfork vs withBlck) but in that case the question is why they'd be inconsistent.

tersec · 2026-01-19T11:27:33Z

beacon_chain/rpc/rest_beacon_api.nim

+              forkyBlck, Opt.none(seq[gloas.DataColumnSidecar]),
              checkValidator = true)
-          elif consensusFork >= ConsensusFork.Fulu:
+          elif consensusFork >= ConsensusFork.Fulu and


This is exactly one hardfork, Fulu

* modularise message routing for sidecars * remove unused proc * some reviews applied * address some of the reviews * some more changes, caller functions * reviews

agnxsh force-pushed the mro branch from 4aa61ef to 4afc14b Compare November 23, 2025 22:03

arnetheduck reviewed Nov 24, 2025

View reviewed changes

tersec reviewed Nov 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Nov 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Nov 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Nov 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

tersec reviewed Nov 24, 2025

View reviewed changes

beacon_chain/validators/message_router.nim Outdated Show resolved Hide resolved

scutuatua-crypto approved these changes Dec 1, 2025

View reviewed changes

agnxsh force-pushed the mro branch from 78b2aa9 to e719139 Compare January 2, 2026 22:00

agnxsh force-pushed the mro branch 2 times, most recently from 064b158 to 86aef48 Compare January 19, 2026 09:06

tersec reviewed Jan 19, 2026

View reviewed changes

agnxsh force-pushed the mro branch from 81ec9b9 to 8f8fe68 Compare January 20, 2026 09:58

agnxsh added 6 commits January 21, 2026 15:53

modularise message routing for sidecars

e941fa9

remove unused proc

ef407c6

some reviews applied

7926a6c

address some of the reviews

3cc1a0b

some more changes, caller functions

5f32d62

reviews

53f63d5

agnxsh force-pushed the mro branch from 8f8fe68 to 53f63d5 Compare January 21, 2026 10:23

Merge branch 'unstable' into mro

a3c1040

tersec enabled auto-merge (squash) January 22, 2026 12:00

tersec merged commit 2025a6d into unstable Jan 22, 2026
11 checks passed

tersec deleted the mro branch January 22, 2026 13:21

agnxsh added a commit that referenced this pull request Jan 24, 2026

modularise message routing for sidecars (#7751)

147d3db

* modularise message routing for sidecars * remove unused proc * some reviews applied * address some of the reviews * some more changes, caller functions * reviews

Conversation

agnxsh commented Nov 23, 2025

Uh oh!

github-actions bot commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Nov 23, 2025 •

edited

Loading