Conversation
beacon_chain/consensus_object_pools/partial_column_quarantine.nim
Outdated
Show resolved
Hide resolved
| DataColumn, DataColumnSidecar, | ||
| MAX_BLOB_COMMITMENTS_PER_BLOCK | ||
|
|
||
| from ../spec/datatypes/deneb import KzgCommitments, KzgProofs |
There was a problem hiding this comment.
If one imports spec/forks, there's no reason to import either ../spec/datatypes/deneb or ../spec/datatypes/fulu explicitly. But it's not obvious me it needs/benefits from ../spec/forks to begin with -- what does it use it for?
beacon_chain/conf.nim
Outdated
| partialColumns* {. | ||
| defaultValue: false, | ||
| desc: "Backward compatible partial data column sidecar support", | ||
| name: "partial-columns" .}: bool |
There was a problem hiding this comment.
Better to leave this hidden and debug-foo for now.
If we need to make it visible and part of the official set of options, can do that easily, can't go back the other way as easily.
In particular, once eventually deployed, it should not need this option at all. It should just be on, by default, because that's how Ethereum will work.
beacon_chain/consensus_object_pools/partial_column_quarantine.nim
Outdated
Show resolved
Hide resolved
|
|
||
| var cellIdx = 0 | ||
| for blobIdx in 0 ..< int(MAX_BLOB_COMMITMENTS_PER_BLOCK): | ||
| if sidecar.cells_present_bitmap[Natural(blobIdx)]: |
| quarantine: ref Quarantine, | ||
| blobQuarantine: ref BlobQuarantine, | ||
| dataColumnQuarantine: ref ColumnQuarantine, | ||
| partialColumnQuarantine: ref PartialColumnQuarantine, |
There was a problem hiding this comment.
Ultimately it doesn't really make sense to have two different quarantines here, or?
If a full data column is present in the data column quarantine, then it should be representable in the more general partialColumnQuarantine. And right now, this leads to two possible quarantines to check.
|
|
||
| ok() | ||
|
|
||
|
|
There was a problem hiding this comment.
extra empty line between functions?
| @@ -0,0 +1,973 @@ | |||
| # beacon_chain | |||
| # Copyright (c) 2025-2026 Status Research & Development GmbH | |||
There was a problem hiding this comment.
This module didn't exist in 2025.
| colIdx = ColumnIndex(0) | ||
| numBlobs = 3 | ||
|
|
||
| discard quarantine.getOrCreateEntry(root, colIdx, numBlobs) |
|
|
||
| # Pick column index 0 for the round-trip test | ||
| let colIdx = ColumnIndex(0) | ||
| discard pcq.getOrCreateEntry(blockRoot, colIdx, numBlobs) |
| wallTime: BeaconTime, subnet_id: uint64): | ||
| Result[void, ValidationError] = | ||
|
|
||
| # === For all partial messages === |
There was a problem hiding this comment.
are there other partial messages? Also, this seems to be still fairly partial column-specific. Not really for "all partial messages"
There was a problem hiding this comment.
technically there's a cell only situation and a cells + header situation
| # TODO Is this an error? | ||
| beacon_block_production_errors.inc() | ||
| return head # Errors logged in router | ||
| when consensusFork != ConsensusFork.Fulu: |
There was a problem hiding this comment.
Presumably the partial cell messaging would continue in some form into Gloas? I'm not sure if/how that's specified though.
There was a problem hiding this comment.
yes, but the experimental spec branch does not suggest that, i've tried to keep it that way
| # Batch verify that the cells match the corresponding commitments and proofs | ||
| var commitments = newSeqOfCap[KzgCommitment](blobIndices.len) | ||
| for i in blobIndices: | ||
| commitments.add(all_commitments[i]) |
There was a problem hiding this comment.
This loop can be more clearly expressed as a mapIt:
let commitments = blobIndices.mapIt(all_commitments[it])
| return dag.checkedReject( | ||
| "PartialDataColumnSidecar: cells and proofs count mismatch") | ||
|
|
||
| let column_index = ColumnIndex(subnet_id) |
There was a problem hiding this comment.
Subnet indices can't be uniquely mapped back to data column indices, e.g., see
nimbus-eth2/beacon_chain/spec/network.nim
Lines 139 to 144 in 6aa946f
compute_subnet_for_data_column_sidecar:
def compute_subnet_for_data_column_sidecar(column_index: ColumnIndex) -> SubnetID:
return SubnetID(column_index % DATA_COLUMN_SIDECAR_SUBNET_COUNT)There was a problem hiding this comment.
To the extent this is true, as it currently is, this needs another one of those static: doAssert DATA_COLUMN_SIDECAR_SUBNET_COUNT == NUMBER_OF_COLUMNS assertions to verify that it's currently safe (but that might change in the future, and it should not compile the moment it isn't).
|
|
||
| proc routeSignedBeaconBlock*( | ||
| router: ref MessageRouter, | ||
| blck: electra.SignedBeaconBlock | fulu.SignedBeaconBlock | |
There was a problem hiding this comment.
Is there ever going to be a reason to route a fulu.SignedBeaconBlock (or a gloas.SignedBeaconBlock, when the partial cell dissemination is defined for that fork) without the partialSidecars of the overload added in this PR? It seems like it should just not compile, insofar as it allows silently neglecting to send the partial sidecars.
Or is there a use case for this?
| node.forkDigestAtEpoch(contextEpoch), subnet_id) | ||
| node.broadcast(topic, data_column) | ||
|
|
||
| proc broadcastPartialDataColumnHeader*( |
There was a problem hiding this comment.
Nothing ever seems to call this function?
There was a problem hiding this comment.
yeah i thought earlier there's a function to publish the header separately, since we have atomic header management in gossip validation function
| var decompressed = snappy.decode(message.data, maxSize) | ||
| let res = if decompressed.len > 0: | ||
| block: | ||
| var result = ValidationResult.Reject |
There was a problem hiding this comment.
result has special meaning in Nim. Better to use res or any other non-literally-result name for this kind of variable.
There was a problem hiding this comment.
Here there's already a res, so could be a different variation or etc of res.
| PartialColumnEntry* = object | ||
| ## Tracks accumulated cells for a single (block_root, column_index) pair. | ||
| headerValidated*: bool | ||
| cellsReceived*: BitSeq |
There was a problem hiding this comment.
Is this redundant with the isSome/isNone-ness of each cell/proof entry? Can they diverge?
|
|
||
| let | ||
| block_root = hash_tree_root(block_header) | ||
| column_index = ColumnIndex(subnet_id) |
There was a problem hiding this comment.
This is only true if
static: doAssert DATA_COLUMN_SIDECAR_SUBNET_COUNT == NUMBER_OF_COLUMNSso if relying on this assertion implicitly, should add it explicitly, so if that ever changes, the compilation will immediately fail and the fix will be more obvious.
| column_index: ColumnIndex): | ||
| Result[void, ValidationError] = | ||
| let res = p_data_column.verify_partial_data_column_sidecar_kzg_proofs( | ||
| all_commitments, column_index) |
There was a problem hiding this comment.
Reduce indentation by 2 spaces.
| router: ref MessageRouter, | ||
| blck: fulu.SignedBeaconBlock, | ||
| someSidecarsOpt: seq[fulu.DataColumnSidecar], | ||
| partialSidecars: seq[fulu.PartialDataColumnSidecar], |
There was a problem hiding this comment.
Generally speaking, it seems like the someSidecarsOpt and partialSidecars should be constructible from each other, or?
There was a problem hiding this comment.
yes we are passing both to broadcast one and adding the other to block processor without wasting much time and resources converting
|
Merge conflicts in |
2 participants
tasks:
make a partial column quarantine to cache headers as they come in via gossipthat quarantine should also be able to handlePartialColumnDataSidecaruntil the adequate no. of cells are hit, then assemble them intoDataColumnSidecarwrite tests for both of these caseswrite the new datatypes and gossip validation conditionsmake these assemble sidecars ingest intoColumnQuaratinewrite a backward compatible broadcast mechanism to handle partial and data column sidecar, can add a flag maybeinvestigate if there's anything left in the Nimbus Eth2 <-> Nim Libp2p layer in terms of orchestration/compatibilitymanage eth2processor to deal with partial columns and then on requisite no of them populate data column quarantine