From 5ef748b19d031ba8000876d9d1c4fb6fcb1ff568 Mon Sep 17 00:00:00 2001 From: henridf Date: Sun, 21 May 2023 19:47:00 +0200 Subject: [PATCH] Clarify addOrphan error/logging (#4981) * Clarify addOrphan error/logging addOrphan returned a bool to indicate success. Change this to a Result so that different errors can be distinguished. * Update beacon_chain/consensus_object_pools/block_quarantine.nim Co-authored-by: tersec * Update beacon_chain/gossip_processing/gossip_validation.nim --------- Co-authored-by: tersec --- .../consensus_object_pools/block_quarantine.nim | 10 +++++----- beacon_chain/gossip_processing/block_processor.nim | 10 ++++++---- beacon_chain/gossip_processing/gossip_validation.nim | 9 ++++++--- tests/test_block_quarantine.nim | 8 ++++---- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/beacon_chain/consensus_object_pools/block_quarantine.nim b/beacon_chain/consensus_object_pools/block_quarantine.nim index 15c728dc7..602eff3c6 100644 --- a/beacon_chain/consensus_object_pools/block_quarantine.nim +++ b/beacon_chain/consensus_object_pools/block_quarantine.nim @@ -251,11 +251,11 @@ func clearAfterReorg*(quarantine: var Quarantine) = # likely imminent arrival. func addOrphan*( quarantine: var Quarantine, finalizedSlot: Slot, - signedBlock: ForkedSignedBeaconBlock): bool = + signedBlock: ForkedSignedBeaconBlock): Result[void, cstring] = ## Adds block to quarantine's `orphans` and `missing` lists. if not isViable(finalizedSlot, getForkedBlockField(signedBlock, slot)): quarantine.addUnviable(signedBlock.root) - return false + return err("block unviable") quarantine.cleanupOrphans(finalizedSlot) @@ -263,19 +263,19 @@ func addOrphan*( if parent_root in quarantine.unviable: quarantine.unviable[signedBlock.root] = () - return true + return ok() # Even if the quarantine is full, we need to schedule its parent for # downloading or we'll never get to the bottom of things quarantine.addMissing(parent_root) if quarantine.orphans.lenu64 >= MaxOrphans: - return false + return err("block quarantine full") quarantine.orphans[(signedBlock.root, signedBlock.signature)] = signedBlock quarantine.missing.del(signedBlock.root) - true + ok() iterator pop*(quarantine: var Quarantine, root: Eth2Digest): ForkedSignedBeaconBlock = diff --git a/beacon_chain/gossip_processing/block_processor.nim b/beacon_chain/gossip_processing/block_processor.nim index 91cb3c60a..f1c732c95 100644 --- a/beacon_chain/gossip_processing/block_processor.nim +++ b/beacon_chain/gossip_processing/block_processor.nim @@ -520,12 +520,14 @@ proc storeBlock*( return err((VerifierError.UnviableFork, ProcessingStatus.completed)) - if not self.consensusManager.quarantine[].addOrphan( - dag.finalizedHead.slot, ForkedSignedBeaconBlock.init(signedBlock)): - debug "Block quarantine full", + if (let r = self.consensusManager.quarantine[].addOrphan( + dag.finalizedHead.slot, ForkedSignedBeaconBlock.init(signedBlock)); + r.isErr()): + debug "storeBlock: could not add orphan", blockRoot = shortLog(signedBlock.root), blck = shortLog(signedBlock.message), - signature = shortLog(signedBlock.signature) + signature = shortLog(signedBlock.signature), + err = r.error() of VerifierError.UnviableFork: # Track unviables so that descendants can be discarded properly self.consensusManager.quarantine[].addUnviable(signedBlock.root) diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 088546ac1..f43e6c818 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -457,10 +457,13 @@ proc validateBeaconBlock*( # When the parent is missing, we can't validate the block - we'll queue it # in the quarantine for later processing - if not quarantine[].addOrphan( + if (let r = quarantine[].addOrphan( dag.finalizedHead.slot, - ForkedSignedBeaconBlock.init(signed_beacon_block)): - debug "Block quarantine full" + ForkedSignedBeaconBlock.init(signed_beacon_block)); r.isErr): + debug "validateBeaconBlock: could not add orphan", + blockRoot = shortLog(signed_beacon_block.root), + blck = shortLog(signed_beacon_block.message), + err = r.error() return errIgnore("BeaconBlock: Parent not found") diff --git a/tests/test_block_quarantine.nim b/tests/test_block_quarantine.nim index 593680144..f7ba2d001 100644 --- a/tests/test_block_quarantine.nim +++ b/tests/test_block_quarantine.nim @@ -44,13 +44,13 @@ suite "Block quarantine": check: FetchRecord(root: b1.root) in quarantine.checkMissing() - quarantine.addOrphan(Slot 0, b1) + quarantine.addOrphan(Slot 0, b1).isOk FetchRecord(root: b1.root) notin quarantine.checkMissing() - quarantine.addOrphan(Slot 0, b2) - quarantine.addOrphan(Slot 0, b3) - quarantine.addOrphan(Slot 0, b4) + quarantine.addOrphan(Slot 0, b2).isOk + quarantine.addOrphan(Slot 0, b3).isOk + quarantine.addOrphan(Slot 0, b4).isOk quarantine.addBlobless(Slot 0, b5) quarantine.addBlobless(Slot 0, b6)