From 2fc43c9ba70e3b43a023c3646be5d7355b849156 Mon Sep 17 00:00:00 2001 From: tersec Date: Fri, 1 Dec 2023 18:58:46 +0000 Subject: [PATCH] track block/blob matching/quarantines using both indices and commitments (#5621) --- .../blob_quarantine.nim | 39 +++++++++---------- .../gossip_processing/block_processor.nim | 3 +- .../gossip_processing/eth2_processor.nim | 4 +- beacon_chain/nimbus_beacon_node.nim | 2 +- beacon_chain/spec/state_transition.nim | 2 - beacon_chain/sync/request_manager.nim | 2 - beacon_chain/validators/beacon_validators.nim | 2 - 7 files changed, 23 insertions(+), 31 deletions(-) diff --git a/beacon_chain/consensus_object_pools/blob_quarantine.nim b/beacon_chain/consensus_object_pools/blob_quarantine.nim index 0bf137129..dfabd3984 100644 --- a/beacon_chain/consensus_object_pools/blob_quarantine.nim +++ b/beacon_chain/consensus_object_pools/blob_quarantine.nim @@ -16,7 +16,8 @@ const type BlobQuarantine* = object - blobs*: OrderedTable[(Eth2Digest, BlobIndex), ref BlobSidecar] + blobs*: + OrderedTable[(Eth2Digest, BlobIndex, KzgCommitment), ref BlobSidecar] BlobFetchRecord* = object block_root*: Eth2Digest indices*: seq[BlobIndex] @@ -32,22 +33,19 @@ func put*(quarantine: var BlobQuarantine, blobSidecar: ref BlobSidecar) = # 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. - var oldest_blob_key: (Eth2Digest, BlobIndex) + # + # This also therefore automatically garbage-collects otherwise valid garbage + # blobs 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_blob_key: (Eth2Digest, BlobIndex, KzgCommitment) for k in quarantine.blobs.keys: oldest_blob_key = k break quarantine.blobs.del oldest_blob_key let block_root = hash_tree_root(blobSidecar.signed_block_header.message) discard quarantine.blobs.hasKeyOrPut( - (block_root, blobSidecar.index), blobSidecar) - -func blobIndices*(quarantine: BlobQuarantine, digest: Eth2Digest): - seq[BlobIndex] = - var r: seq[BlobIndex] = @[] - for i in 0..= ConsensusFork.Deneb: if self.blobQuarantine[].hasBlobs(signedBlock): - Opt.some(self.blobQuarantine[].popBlobs(signedBlock.root)) + Opt.some(self.blobQuarantine[].popBlobs(signedBlock.root, signedBlock)) else: if not self.quarantine[].addBlobless(self.dag.finalizedHead.slot, signedBlock): @@ -310,7 +310,7 @@ proc processBlobSidecar*( self.blockProcessor[].enqueueBlock( MsgSource.gossip, ForkedSignedBeaconBlock.init(blobless), - Opt.some(self.blobQuarantine[].popBlobs(block_root))) + Opt.some(self.blobQuarantine[].popBlobs(block_root, blobless))) else: discard self.quarantine[].addBlobless(self.dag.finalizedHead.slot, blobless) diff --git a/beacon_chain/nimbus_beacon_node.nim b/beacon_chain/nimbus_beacon_node.nim index 81e7b190d..7e42b9c2e 100644 --- a/beacon_chain/nimbus_beacon_node.nim +++ b/beacon_chain/nimbus_beacon_node.nim @@ -408,7 +408,7 @@ proc initFullNode( Result[void, VerifierError].err(VerifierError.MissingParent), "rmanBlockVerifier") else: - let blobs = blobQuarantine[].popBlobs(forkyBlck.root) + let blobs = blobQuarantine[].popBlobs(forkyBlck.root, forkyBlck) blockProcessor[].addBlock(MsgSource.gossip, signedBlock, Opt.some(blobs), maybeFinalized = maybeFinalized) diff --git a/beacon_chain/spec/state_transition.nim b/beacon_chain/spec/state_transition.nim index 5e99b0858..7c61109ca 100644 --- a/beacon_chain/spec/state_transition.nim +++ b/beacon_chain/spec/state_transition.nim @@ -504,8 +504,6 @@ proc makeBeaconBlock*( else: {.error: "Unsupported fork".} -# workaround for https://github.com/nim-lang/Nim/issues/20900 rather than have -# these be default arguments proc makeBeaconBlock*( cfg: RuntimeConfig, state: var ForkedHashedBeaconState, proposer_index: ValidatorIndex, randao_reveal: ValidatorSig, diff --git a/beacon_chain/sync/request_manager.nim b/beacon_chain/sync/request_manager.nim index 96b1cd461..18883b08e 100644 --- a/beacon_chain/sync/request_manager.nim +++ b/beacon_chain/sync/request_manager.nim @@ -293,7 +293,6 @@ proc getMissingBlobs(rman: RequestManager): seq[BlobIdentifier] = if len(missing.indices) == 0: warn "quarantine missing blobs, but missing indices is empty", blk=blobless.root, - indices=rman.blobQuarantine[].blobIndices(blobless.root), commitments=len(blobless.message.body.blob_kzg_commitments) for idx in missing.indices: let id = BlobIdentifier(block_root: blobless.root, index: idx) @@ -303,7 +302,6 @@ proc getMissingBlobs(rman: RequestManager): seq[BlobIdentifier] = # this is a programming error should it occur. warn "missing blob handler found blobless block with all blobs", blk=blobless.root, - indices=rman.blobQuarantine[].blobIndices(blobless.root), commitments=len(blobless.message.body.blob_kzg_commitments) discard rman.blockVerifier(ForkedSignedBeaconBlock.init(blobless), false) diff --git a/beacon_chain/validators/beacon_validators.nim b/beacon_chain/validators/beacon_validators.nim index b170bab79..3ae68c6cf 100644 --- a/beacon_chain/validators/beacon_validators.nim +++ b/beacon_chain/validators/beacon_validators.nim @@ -560,8 +560,6 @@ proc makeBeaconBlockForHeadAndSlot*( else: err(blck.error) -# workaround for https://github.com/nim-lang/Nim/issues/20900 to avoid default -# parameters proc makeBeaconBlockForHeadAndSlot*( PayloadType: type ForkyExecutionPayloadForSigning, node: BeaconNode, randao_reveal: ValidatorSig, validator_index: ValidatorIndex, graffiti: GraffitiBytes, head: BlockRef,