adopt columns into the BN, except column syncing
#6945
Conversation
| # this repairing will almost never happen unless these malformed | ||
| # columns coming via req/resp. | ||
| if not columnsOk: | ||
| if dataColumnsOpt.get.len >= (NUMBER_OF_COLUMNS div 2): |
There was a problem hiding this comment.
It's probably clearer to combine these -- if (not columnsOk) and dataColumnsOpt.get.len >= (NUMBER_OF_COLUMNS div 2)
| var blobsOk = true | ||
| let blobs = | ||
| withBlck(parentBlck.get()): | ||
| when consensusFork >= ConsensusFork.Deneb: | ||
| when consensusFork >= ConsensusFork.Deneb and |
There was a problem hiding this comment.
This is at this point a forever-fixed set of two items -- when consensusFork in [ConsensusFork.Deneb, ConsensusFork.Electra] or similar makes this clearer.
| @@ -146,8 +156,57 @@ proc recover_matrix*(partial_matrix: seq[MatrixEntry], | |||
|
|
|||
| ok(extended_matrix) | |||
|
|
|||
| proc recover_cells_and_proofs*( | |||
| data_columns: seq[DataColumnSidecar]): | |||
There was a problem hiding this comment.
Both callers to this function first dereference and copy into a separate seq, fairly wastefully:
nimbus-eth2/beacon_chain/gossip_processing/block_processor.nim
Lines 220 to 221 in 66986ed
and
Would be better if recover_cells_and_proofs rather took a seq[ref DataColumnSidecar] and didn't generate pointless sequence copies.
| reconstructed_columns = | ||
| get_data_column_sidecars(forkyBlck, recovered_cps.get) | ||
| for rc in reconstructed_columns: | ||
| if rc notin columns.mapIt(it[]): |
There was a problem hiding this comment.
This columns.mapIt(it[]) too much to calculate for a loop body over reconstructed_columns where columns is the same across all iterations.
It shouldn't be recalculating this mapIt every iteration, and ideally doesn't have to calculate it at all, when the outcome ultimately is just this bool of presence of lack of presence in columns.
This is basically another artifact of the mix between the seq[ref DataColumnSidecar] and seq[DataColumnSidecar] which is happening here. It's probably be better to standardize on seq[ref DataColumnSidecar], between the two, but randomly doing this expand-seq[ref DataColumnSidecar]-to-seq[DataColumnSidecar] (in loop bodies, even) sort of undermines the point of the distinction.
* extend configurability in peerdas * cast balance to u64 * revert cancelAndWait * fix function call * fix another cfg issue * avoid multiple copies of hash sets
Continued from the
columnsbranch. This branch essentially attempts to add column support to the nimbus-eth2 CL post FULU_FORK_EPOCH. However it does NOT contain column syncing baked into existingSyncManagerNote that all column related mechanics, are gated to and after the Fulu fork, would be helpful to have more eyes on the gating.