From e2afc583cb1bf31a995c6c6bfa246561e507291c Mon Sep 17 00:00:00 2001 From: Agnish Ghosh Date: Fri, 21 Jun 2024 14:51:54 +0530 Subject: [PATCH] fix: reviews, pass1 --- beacon_chain/beacon_chain_db.nim | 2 +- beacon_chain/beacon_node.nim | 3 ++- .../consensus_object_pools/data_column_quarantine.nim | 6 +++--- beacon_chain/sync/request_manager.nim | 7 ++++--- beacon_chain/sync/sync_manager.nim | 3 ++- beacon_chain/sync/sync_protocol.nim | 7 +++++-- 6 files changed, 17 insertions(+), 11 deletions(-) diff --git a/beacon_chain/beacon_chain_db.nim b/beacon_chain/beacon_chain_db.nim index b242f3b49..a8eb2e56a 100644 --- a/beacon_chain/beacon_chain_db.nim +++ b/beacon_chain/beacon_chain_db.nim @@ -254,7 +254,7 @@ func blobkey(root: Eth2Digest, index: BlobIndex) : array[40, byte] = ret -func columnkey(root: Eth2Digest, index: ColumnIndex) : array[40, byte] = +func columnkey(root: Eth2Digest, index: ColumnIndex): array[40, byte] = var ret: array[40, byte] ret[0..<8] = toBytes(index) ret[8..<40] = root.data diff --git a/beacon_chain/beacon_node.nim b/beacon_chain/beacon_node.nim index c52f243a5..987bb9d3f 100644 --- a/beacon_chain/beacon_node.nim +++ b/beacon_chain/beacon_node.nim @@ -22,7 +22,8 @@ import ./el/el_manager, ./consensus_object_pools/[ blockchain_dag, blob_quarantine, block_quarantine, consensus_manager, - data_column_quarantine, attestation_pool, sync_committee_msg_pool, validator_change_pool], + data_column_quarantine, attestation_pool, sync_committee_msg_pool, + validator_change_pool], ./spec/datatypes/[base, altair], ./spec/eth2_apis/dynamic_fee_recipients, ./sync/[sync_manager, request_manager], diff --git a/beacon_chain/consensus_object_pools/data_column_quarantine.nim b/beacon_chain/consensus_object_pools/data_column_quarantine.nim index 5b6d8a340..16d233a4a 100644 --- a/beacon_chain/consensus_object_pools/data_column_quarantine.nim +++ b/beacon_chain/consensus_object_pools/data_column_quarantine.nim @@ -40,11 +40,11 @@ func shortLog*(x: seq[DataColumnFetchRecord]): string = func put*(quarantine: var DataColumnQuarantine, dataColumnSidecar: ref DataColumnSidecar) = if quarantine.data_columns.lenu64 >= MaxDataColumns: # FIFO if full. For example, sync manager and request manager can race to - # put blobs in at the same time, so one gets blob insert -> block resolve - # -> blob insert sequence, which leaves garbage blobs. + # put data columns in at the same time, so one gets blob insert -> block resolve + # -> data columns insert sequence, which leaves garbage data columns. # # This also therefore automatically garbage-collects otherwise valid garbage - # blobs which are correctly signed, point to either correct block roots or a + # data columns which are correctly signed, point to either correct block roots or a # block root which isn't ever seen, and then are for any reason simply never # used. var oldest_column_key: (Eth2Digest, ColumnIndex) diff --git a/beacon_chain/sync/request_manager.nim b/beacon_chain/sync/request_manager.nim index 7dcb0aa6d..dcf34acc8 100644 --- a/beacon_chain/sync/request_manager.nim +++ b/beacon_chain/sync/request_manager.nim @@ -291,7 +291,7 @@ proc fetchDataColumnsFromNetwork(rman: RequestManager, discard await rman.blockVerifier(col, false) else: - debug "Data Columns by root request failed", + debug "Data columns by root request failed", peer = peer, columns = shortLog(colIdList), err = columns.error() peer.updateScore(PeerScoreNoValues) @@ -472,7 +472,8 @@ proc getMissingDataColumns(rman: RequestManager): seq[DataColumnIdentifier] = wallTime = rman.getBeaconTime() wallSlot = wallTime.slotOrZero() delay = wallTime - wallSlot.start_beacon_time() - waitDur = TimeDiff(nanoseconds: DATA_COLUMN_GOSSIP_WAIT_TIME_NS) + + const waitDur = TimeDiff(nanoseconds: DATA_COLUMN_GOSSIP_WAIT_TIME_NS) var fetches: seq[DataColumnIdentifier] @@ -496,7 +497,7 @@ proc getMissingDataColumns(rman: RequestManager): seq[DataColumnIdentifier] = if id notin fetches: fetches.add(id) else: - # this is a programming error and it should occur + # this is a programming error and it not should occur warn "missing data column handler found columnless block with all data columns", blk = columnless.root, commitments=len(forkyBlck.message.body.blob_kzg_commitments) diff --git a/beacon_chain/sync/sync_manager.nim b/beacon_chain/sync/sync_manager.nim index 8c6ce0b48..d560079b4 100644 --- a/beacon_chain/sync/sync_manager.nim +++ b/beacon_chain/sync/sync_manager.nim @@ -80,7 +80,8 @@ type BeaconBlocksRes = NetRes[List[ref ForkedSignedBeaconBlock, Limit MAX_REQUEST_BLOCKS]] BlobSidecarsRes = NetRes[List[ref BlobSidecar, Limit(MAX_REQUEST_BLOB_SIDECARS)]] - DataColumnSidecarsRes = NetRes[List[ref DataColumnSidecar, Limit(MAX_REQUEST_DATA_COLUMNS)]] + DataColumnSidecarsRes = + NetRes[List[ref DataColumnSidecar, Limit(MAX_REQUEST_DATA_COLUMNS)]] proc now*(sm: typedesc[SyncMoment], slots: uint64): SyncMoment {.inline.} = SyncMoment(stamp: now(chronos.Moment), slots: slots) diff --git a/beacon_chain/sync/sync_protocol.nim b/beacon_chain/sync/sync_protocol.nim index cc6ec469b..ad03139cd 100644 --- a/beacon_chain/sync/sync_protocol.nim +++ b/beacon_chain/sync/sync_protocol.nim @@ -38,8 +38,8 @@ type slot: Slot BlockRootsList* = List[Eth2Digest, Limit MAX_REQUEST_BLOCKS] - BlobIdentifierList* = List[BlobIdentifier, Limit (MAX_REQUEST_BLOB_SIDECARS)] - DataColumnIdentifierList* = List[DataColumnIdentifier, Limit (MAX_REQUEST_DATA_COLUMNS)] + BlobIdentifierList* = List[BlobIdentifier, Limit MAX_REQUEST_BLOB_SIDECARS] + DataColumnIdentifierList* = List[DataColumnIdentifier, Limit MAX_REQUEST_DATA_COLUMNS] proc readChunkPayload*( conn: Connection, peer: Peer, MsgType: type (ref ForkedSignedBeaconBlock)): @@ -403,6 +403,9 @@ p2pProtocol BeaconSync(version = 1, if columnIds.len == 0: raise newException(InvalidInputsError, "No data columns requested") + if columnIds.lenu64 > MAX_REQUEST_DATA_COLUMNS: + raise newException(InvalidInputsError, "Exceeding data column request limit") + let dag = peer.networkState.dag count = columnIds.len