From 85826e614ffa4a021cbddbbfa558fb65558fe9eb Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Thu, 30 Oct 2025 16:10:56 +0100 Subject: [PATCH 1/3] Trace invalid blocks in unviable quarantine A block may become unviable for two main reasons: * the fork it was on is orphaned by chain finality - ie the block was valid in some history, but not any more in the canonical one * the block is invalid, either because it is itself invalid or it builds on an invalid block Until now we've treated both kinds the same when it comes to the `unviable` quarantine - because of this, when checking gossip rules the client has in some cases opted for the more lenient "IGNORE" respons instead of using "REJECT". This PR brings the client more closely in line with ignore/reject rules by tracking the original error that led to the unviablility. * use `minilru` instead of `OrderedTable` for simplified capacity management and performance * simplify quarantine operations that deal with unviables * remove unused quarantine events * keep track of the block being processed to avoid it being readded to the quarantine during processing --- beacon_chain.nimble | 5 +- .../attestation_pool.nim | 22 +- .../block_quarantine.nim | 270 ++++++++---------- .../consensus_object_pools/blockchain_dag.nim | 2 +- .../consensus_manager.nim | 3 +- .../gossip_processing/block_processor.nim | 186 +++++++----- .../gossip_processing/gossip_validation.nim | 189 ++++++------ tests/test_block_processor.nim | 1 - tests/test_block_quarantine.nim | 67 +++-- vendor/nim-minilru | 2 +- 10 files changed, 406 insertions(+), 341 deletions(-) diff --git a/beacon_chain.nimble b/beacon_chain.nimble index 14a4fcbd33..b4f9561f9c 100644 --- a/beacon_chain.nimble +++ b/beacon_chain.nimble @@ -14,7 +14,6 @@ license = "MIT or Apache License 2.0" requires( "nim == 2.2.4", - "https://github.com/status-im/NimYAML", "bearssl", "blscurve", "chronicles", @@ -28,6 +27,7 @@ requires( "libbacktrace", "libp2p", "metrics", + "minilru", "nat_traversal", "nimcrypto", "normalize", @@ -41,11 +41,12 @@ requires( "stint", "taskpools", "testutils", + "toml_serialization", "unicodedb >= 0.10", "unittest2", + "yaml", "web3", "zlib", - "toml_serialization", "https://github.com/status-im/nim-kzg4844.git", "zxcvbn" ) diff --git a/beacon_chain/consensus_object_pools/attestation_pool.nim b/beacon_chain/consensus_object_pools/attestation_pool.nim index 0f4da61b61..abd418d896 100644 --- a/beacon_chain/consensus_object_pools/attestation_pool.nim +++ b/beacon_chain/consensus_object_pools/attestation_pool.nim @@ -953,19 +953,23 @@ proc getBeaconHead*( proc selectOptimisticHead*( pool: var AttestationPool, wallTime: BeaconTime): Opt[BeaconHead] = ## Trigger fork choice and returns the new head block. - let newHeadRoot = pool.forkChoice.get_head(pool.dag, wallTime) - if newHeadRoot.isErr: - error "Couldn't select head", err = newHeadRoot.error - return err() + let newHeadRoot = pool.forkChoice.get_head(pool.dag, wallTime).valueOr: + error "Couldn't select head", err = error + return Opt.none(BeaconHead) - let headBlock = pool.dag.getBlockRef(newHeadRoot.get()).valueOr: + let headBlock = pool.dag.getBlockRef(newHeadRoot).valueOr: # This should normally not happen, but if the chain dag and fork choice # get out of sync, we'll need to try to download the selected head - in # the meantime, return nil to indicate that no new head was chosen - warn "Fork choice selected unknown head, trying to sync", - root = newHeadRoot.get() - pool.quarantine[].addMissing(newHeadRoot.get()) - return err() + + pool.quarantine[].addMissing(newHeadRoot).isOkOr: + # The newly selected head is unviable for some reason - the only way out + # here is that fork choice gets information about some other head + warn "Fork choice selected unviable head - cannot sync", newHeadRoot, err = error + return Opt.none(BeaconHead) + + warn "Fork choice selected unknown head, trying to sync", newHeadRoot + return Opt.none(BeaconHead) ok pool.getBeaconHead(headBlock) diff --git a/beacon_chain/consensus_object_pools/block_quarantine.nim b/beacon_chain/consensus_object_pools/block_quarantine.nim index 788b5668ef..c2f14144a4 100644 --- a/beacon_chain/consensus_object_pools/block_quarantine.nim +++ b/beacon_chain/consensus_object_pools/block_quarantine.nim @@ -7,21 +7,18 @@ {.push raises: [], gcsafe.} -import - std/tables, - chronicles, chronos, - ../spec/[block_id, forks, presets] +import std/tables, minilru, chronicles, ../spec/[block_id, forks, presets] -export tables, forks +export tables, minilru, forks const MaxRetriesPerMissingItem = 7 ## Exponential backoff, double interval between each attempt MaxMissingItems* = 1024 ## Arbitrary - MaxOrphans = SLOTS_PER_EPOCH * 3 + MaxOrphans = int(SLOTS_PER_EPOCH * 3) ## Enough for finalization in an alternative fork - MaxSidecarless = SLOTS_PER_EPOCH * 128 + MaxSidecarless = int(SLOTS_PER_EPOCH * 128) ## Arbitrary MaxUnviables = 16 * 1024 ## About a day of blocks - most likely not needed but it's quite cheap.. @@ -33,6 +30,14 @@ type FetchRecord* = object root*: Eth2Digest + UnviableKind* {.pure.} = enum + UnviableFork + Invalid + + OrphanLru = LruCache[(Eth2Digest, ValidatorSig), ForkedSignedBeaconBlock] + SidecarlessLru = LruCache[Eth2Digest, ForkedSignedBeaconBlock] + UnviableLru = LruCache[Eth2Digest, UnviableKind] + Quarantine* = object ## Keeps track of unvalidated blocks coming from the network ## and that cannot yet be added to the chain @@ -41,8 +46,7 @@ type ## ChainDAGRef DAG due to missing ancestor(s). ## ## Trivially invalid blocks may be dropped before reaching this stage. - - orphans*: OrderedTable[(Eth2Digest, ValidatorSig), ForkedSignedBeaconBlock] + orphans*: OrphanLru ## Blocks that we don't have a parent for - when we resolve the ## parent, we can proceed to resolving the block as well - we ## index this by root and signature such that a block with @@ -50,26 +54,21 @@ type ## to be dropped. An orphan block may also be "blobless" (see ## below) - if so, upon resolving the parent, it should be ## added to the blobless table, after verifying its signature. - orphansEvent*: AsyncEvent - ## Asynchronous event which will be set, when new block appears in - ## orphans table. - sidecarless*: OrderedTable[Eth2Digest, ForkedSignedBeaconBlock] + sidecarless*: SidecarlessLru ## Blocks that we don't have sidecars (BlobSidecar/DataColumnSidecar) for. ## When we have received all sidecars for this block, we can proceed to ## resolving the block as well. Block inserted into this table must ## have a resolved parent (i.e., it is not an orphan). - sidecarlessEvent*: AsyncEvent - ## Asynchronous event which will be set, when new block appears in - ## sidecarless table. - - unviable*: OrderedTable[Eth2Digest, tuple[]] - ## Unviable blocks are those that come from a history that does not - ## include the finalized checkpoint we're currently following, and can - ## therefore never be included in our canonical chain - we keep their hash - ## around so that we can avoid cluttering the orphans table with their - ## descendants - the ChainDAG only keeps track blocks that make up the - ## valid and canonical history. + + unviable*: UnviableLru + ## Unviable blocks are those that can no longer be included in the + ## canonical chain either because the fork they were on became unviable + ## due to finalization or because they were invalid. + ## + ## We keep their hash around so that we can avoid cluttering the orphans + ## table with their descendants - the ChainDAG only keeps track blocks + ## that make up the valid and canonical history. ## ## Entries are evicted in FIFO order - recent entries are more likely to ## appear again in attestations and blocks - however, the unviable block @@ -84,18 +83,19 @@ type missing*: Table[Eth2Digest, MissingBlock] ## Roots of blocks that we would like to have (either parent_root of ## unresolved blocks or block roots of attestations) - missingEvent*: AsyncEvent - ## Asynchronous event which will be set, when new block appears in - ## missing table. + + processing: Eth2Digest + ## This block is currently being processed and should therefore not be + ## added to the quarantine cfg*: RuntimeConfig func init*(T: type Quarantine, cfg: RuntimeConfig): T = T( cfg: cfg, - sidecarlessEvent: newAsyncEvent(), - missingEvent: newAsyncEvent(), - orphansEvent: newAsyncEvent() + orphans: OrphanLru.init(MaxOrphans), + sidecarless: SidecarlessLru.init(MaxSidecarless), + unviable: UnviableLru.init(MaxUnviables), ) func checkMissing*(quarantine: var Quarantine, max: int): seq[FetchRecord] = @@ -118,17 +118,24 @@ func checkMissing*(quarantine: var Quarantine, max: int): seq[FetchRecord] = if result.len >= max: break -proc addMissing*(quarantine: var Quarantine, root: Eth2Digest) = - ## Schedule the download a the given block +proc addMissing*(quarantine: var Quarantine, root: Eth2Digest): Result[void, UnviableKind] = + ## Schedule the download a given block or its ancestor, if we're keeping + ## track of it as an orphan + + # If the block is unviable, tell the caller + quarantine.unviable.get(root).isErrOr: + return err(value) + if quarantine.missing.len >= MaxMissingItems: - return + # The block might still be viable, but we don't have space to investigate + return ok() + + if root == quarantine.processing: + # It's not in flight if we're in the middle of processing it + return ok() var r = root for i in 0 .. MaxOrphans: # Blocks are not trusted, avoid endless loops - if r in quarantine.unviable: - # Won't get anywhere with this block - return - # It's not really missing if we're keeping it in the quarantine. # In that case, add the next missing parent root instead var found = false @@ -141,35 +148,30 @@ proc addMissing*(quarantine: var Quarantine, root: Eth2Digest) = # Add if it's not there, but don't update missing counter if not found: discard quarantine.missing.hasKeyOrPut(r, MissingBlock()) - quarantine.missingEvent.fire() - return + break -func removeOrphan*( - quarantine: var Quarantine, signedBlock: ForkySignedBeaconBlock) = - quarantine.orphans.del((signedBlock.root, signedBlock.signature)) + ok() -func removeSidecarless*( - quarantine: var Quarantine, signedBlock: ForkySignedBeaconBlock) = +func remove*(quarantine: var Quarantine, signedBlock: ForkySignedBeaconBlock) = + quarantine.orphans.del((signedBlock.root, signedBlock.signature)) quarantine.sidecarless.del(signedBlock.root) + quarantine.missing.del(signedBlock.root) + +func startProcessing*(quarantine: var Quarantine, signedBlock: ForkySignedBeaconBlock) = + quarantine.remove(signedBlock) + quarantine.processing = signedBlock.root -func isViable( - finalizedSlot: Slot, slot: Slot): bool = - # The orphan must be newer than the finalization point so that its parent - # either is the finalized block or more recent - slot > finalizedSlot +func clearProcessing*(quarantine: var Quarantine) = + quarantine.processing.reset() -func cleanupUnviable(quarantine: var Quarantine) = - while quarantine.unviable.len() >= MaxUnviables: - var toDel: Eth2Digest - for k in quarantine.unviable.keys(): - toDel = k - break # Cannot modify while for-looping - quarantine.unviable.del(toDel) +func isUnviableFork(blck: ForkyBeaconBlock, finalizedSlot: Slot): bool = + # Block is from a fork that for certain will not be included in the canonical + # chain - notably, the block may still turn out to be unviable when its + # ancestors have been processed and one of _them_ is unviable. + blck.slot <= finalizedSlot func removeUnviableOrphanTree( - quarantine: var Quarantine, - toCheck: var seq[Eth2Digest], - tbl: var OrderedTable[(Eth2Digest, ValidatorSig), ForkedSignedBeaconBlock] + quarantine: var Quarantine, toCheck: var seq[Eth2Digest], kind: UnviableKind ): seq[Eth2Digest] = # Remove the tree of orphans whose ancestor is unviable - they are now also # unviable! This helps avoiding junk in the quarantine, because we don't keep @@ -182,7 +184,7 @@ func removeUnviableOrphanTree( let root = toCheck.pop() if root notin checked: checked.add(root) - for k, v in tbl.mpairs(): + for k, v in quarantine.orphans.mpairs(): let blockRoot = getForkedBlockField(v, parent_root) if blockRoot == root: toCheck.add(k[0]) @@ -191,25 +193,22 @@ func removeUnviableOrphanTree( toRemove.add(k) for k in toRemove: - tbl.del k - quarantine.unviable[k[0]] = () + quarantine.orphans.del k + quarantine.unviable.put(k[0], kind) toRemove.setLen(0) checked func removeUnviableSidecarlessTree( - quarantine: var Quarantine, - toCheck: var seq[Eth2Digest], - tbl: var OrderedTable[Eth2Digest, ForkedSignedBeaconBlock]) = - var - toRemove: seq[Eth2Digest] # Can't modify while iterating + quarantine: var Quarantine, toCheck: var seq[Eth2Digest], kind: UnviableKind +) = + var toRemove: seq[Eth2Digest] # Can't modify while iterating while toCheck.len > 0: let root = toCheck.pop() - for k, v in tbl.mpairs(): - let blockRoot = - withBlck(v): - forkyBlck.message.parent_root + for k, v in quarantine.sidecarless.mpairs(): + let blockRoot = withBlck(v): + forkyBlck.message.parent_root if blockRoot == root: toCheck.add(k) toRemove.add(k) @@ -217,46 +216,51 @@ func removeUnviableSidecarlessTree( toRemove.add(k) for k in toRemove: - tbl.del k - quarantine.unviable[k] = () + quarantine.sidecarless.del k + quarantine.unviable.put(k, kind) toRemove.setLen(0) -func addUnviable*(quarantine: var Quarantine, root: Eth2Digest) = +func addUnviable*(quarantine: var Quarantine, root: Eth2Digest, kind: UnviableKind): UnviableKind = # Unviable - don't try to download again! quarantine.missing.del(root) - if root in quarantine.unviable: - return + quarantine.unviable.get(root).isErrOr: + # If the block was already in unviable, don't downgrade `Invalid` to `UnviableFork` + if kind == UnviableKind.Invalid and value == UnviableKind.UnviableFork: + # Previously "UnviableFork", now "invalid" - this can potentially happen when + # an `UnviableFork` blob races with an `Invalid` block. + quarantine.unviable.put(root, kind) + return kind + return value - quarantine.cleanupUnviable() var toCheck = @[root] - var checked = quarantine.removeUnviableOrphanTree(toCheck, quarantine.orphans) - quarantine.removeUnviableSidecarlessTree(checked, quarantine.sidecarless) + var checked = quarantine.removeUnviableOrphanTree(toCheck, kind) + quarantine.removeUnviableSidecarlessTree(checked, kind) - quarantine.unviable[root] = () + quarantine.unviable.put(root, kind) + kind func cleanupOrphans(quarantine: var Quarantine, finalizedSlot: Slot) = var toDel: seq[(Eth2Digest, ValidatorSig)] for k, v in quarantine.orphans: - if not isViable(finalizedSlot, getForkedBlockField(v, slot)): + if withBlck(v, forkyBlck.message.isUnviableFork(finalizedSlot)): toDel.add k for k in toDel: - quarantine.addUnviable k[0] + discard quarantine.addUnviable(k[0], UnviableKind.UnviableFork) quarantine.orphans.del k func cleanupSidecarless(quarantine: var Quarantine, finalizedSlot: Slot) = var toDel: seq[Eth2Digest] for k, v in quarantine.sidecarless: - withBlck(v): - if not isViable(finalizedSlot, forkyBlck.message.slot): - toDel.add k + if withBlck(v, forkyBlck.message.isUnviableFork(finalizedSlot)): + toDel.add k for k in toDel: - quarantine.addUnviable k + discard quarantine.addUnviable(k, UnviableKind.UnviableFork) quarantine.sidecarless.del k func clearAfterReorg*(quarantine: var Quarantine) = @@ -266,9 +270,7 @@ func clearAfterReorg*(quarantine: var Quarantine) = quarantine.orphans.reset() func pruneAfterFinalization*( - quarantine: var Quarantine, - epoch: Epoch, - needsBackfill: bool + quarantine: var Quarantine, epoch: Epoch, needsBackfill: bool ) = let startEpoch = @@ -302,20 +304,22 @@ proc addOrphan*( quarantine: var Quarantine, finalizedSlot: Slot, signedBlock: ForkySignedBeaconBlock -): Result[void, cstring] = - ## Adds block to quarantine's `orphans` and `missing` lists. - - if not isViable(finalizedSlot, signedBlock.message.slot): - quarantine.addUnviable(signedBlock.root) # will remove from missing - return err("block unviable") - +): Result[void, UnviableKind] = + ## Adds block to quarantine's `orphans` and `missing` lists assuming the + ## parent isn't unviable quarantine.cleanupOrphans(finalizedSlot) let parent_root = signedBlock.message.parent_root + quarantine.unviable.get(parent_root).isErrOr: + # Inherit unviable kind from parent + return err(quarantine.addUnviable(signedBlock.root, value)) - if parent_root in quarantine.unviable: - quarantine.addUnviable(signedBlock.root) - return err("block parent unviable") + if signedBlock.message.isUnviableFork(finalizedSlot): + # will remove from missing + return err(quarantine.addUnviable(signedBlock.root, UnviableKind.UnviableFork)) + + if signedBlock.root == quarantine.processing: + return ok() # It's no longer missing if we downloaded it - remove before adding to make # sure parent chains get downloaded even if missing list is full (works as @@ -324,25 +328,18 @@ proc addOrphan*( # 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) + discard quarantine.addMissing(parent_root) - if quarantine.orphans.lenu64 >= MaxOrphans: - # Evict based on FIFO - var oldest_orphan_key: (Eth2Digest, ValidatorSig) - for k in quarantine.orphans.keys: - oldest_orphan_key = k - break - quarantine.orphans.del oldest_orphan_key - quarantine.sidecarless.del oldest_orphan_key[0] - - quarantine.orphans[(signedBlock.root, signedBlock.signature)] = - ForkedSignedBeaconBlock.init(signedBlock) - quarantine.orphansEvent.fire() + for (evicted, key, _) in quarantine.orphans.putWithEvicted( + (signedBlock.root, signedBlock.signature), ForkedSignedBeaconBlock.init(signedBlock) + ): + if evicted: + # When an orphan gets evicted, also evict the sidecars + quarantine.sidecarless.del key[0] ok() -iterator pop*(quarantine: var Quarantine, root: Eth2Digest): - ForkedSignedBeaconBlock = +iterator pop*(quarantine: var Quarantine, root: Eth2Digest): ForkedSignedBeaconBlock = # Pop orphans whose parent is the block identified by `root` var toRemove: seq[(Eth2Digest, ValidatorSig)] @@ -361,25 +358,18 @@ proc addSidecarless( fulu.SignedBeaconBlock | gloas.SignedBeaconBlock ): bool = if finalizedSlot.isSome(): - if not isViable(finalizedSlot.get(), signedBlock.message.slot): - quarantine.addUnviable(signedBlock.root) + if signedBlock.message.isUnviableFork(finalizedSlot.get()): + discard quarantine.addUnviable(signedBlock.root, UnviableKind.UnviableFork) return false - if quarantine.sidecarless.lenu64 >= MaxSidecarless: - var oldestKey: Eth2Digest - for k in quarantine.sidecarless.keys: - oldestKey = k - break - quarantine.sidecarless.del(oldestKey) - debug "Block without sidecars has been added to the quarantine", - block_root = shortLog(signedBlock.root) - quarantine.sidecarless[signedBlock.root] = - ForkedSignedBeaconBlock.init(signedBlock) + block_root = shortLog(signedBlock.root) + quarantine.sidecarless.put( + signedBlock.root, ForkedSignedBeaconBlock.init(signedBlock) + ) quarantine.last_block_slot = Opt.some(BlockId(slot: signedBlock.message.slot, root: signedBlock.root)) quarantine.missing.del(signedBlock.root) - quarantine.sidecarlessEvent.fire() true proc addSidecarless*( @@ -397,25 +387,15 @@ proc addSidecarless*( discard quarantine.addSidecarless(Opt.none(Slot), signedBlock) func popSidecarless*( - quarantine: var Quarantine, - root: Eth2Digest + quarantine: var Quarantine, root: Eth2Digest ): Opt[ForkedSignedBeaconBlock] = - var blck: ForkedSignedBeaconBlock - if quarantine.sidecarless.pop(root, blck): - Opt.some(blck) - else: - Opt.none(ForkedSignedBeaconBlock) + quarantine.sidecarless.pop(root) func getColumnless*( - quarantine: var Quarantine, - root: Eth2Digest): Opt[ForkedSignedBeaconBlock] = - try: - Opt.some(quarantine.sidecarless[root]) - except KeyError: - Opt.none(ForkedSignedBeaconBlock) - -iterator peekSidecarless*( - quarantine: var Quarantine -): ForkedSignedBeaconBlock = - for k, v in quarantine.sidecarless.mpairs(): + quarantine: var Quarantine, root: Eth2Digest +): Opt[ForkedSignedBeaconBlock] = + quarantine.sidecarless.peek(root) + +iterator peekSidecarless*(quarantine: Quarantine): ForkedSignedBeaconBlock = + for k, v in quarantine.sidecarless.pairs(): yield v diff --git a/beacon_chain/consensus_object_pools/blockchain_dag.nim b/beacon_chain/consensus_object_pools/blockchain_dag.nim index 672a4583fa..ba442b9e58 100644 --- a/beacon_chain/consensus_object_pools/blockchain_dag.nim +++ b/beacon_chain/consensus_object_pools/blockchain_dag.nim @@ -61,7 +61,6 @@ const ## to be pruned every time the prune call is made (once per slot typically) ## unless head is moving faster (ie during sync) - proc putBlock*( dag: ChainDAGRef, signedBlock: ForkyTrustedSignedBeaconBlock) = dag.db.putBlock(signedBlock) @@ -444,6 +443,7 @@ func atSlot*(dag: ChainDAGRef, bid: BlockId, slot: Slot): Opt[BlockSlotId] = else: dag.getBlockIdAtSlot(slot) +type LRUCache[I: static[int], T] = block_pools_types.LRUCache[I, T] func nextTimestamp[I, T](cache: var LRUCache[I, T]): uint32 = if cache.timestamp == uint32.high: for i in 0 ..< I: diff --git a/beacon_chain/consensus_object_pools/consensus_manager.nim b/beacon_chain/consensus_object_pools/consensus_manager.nim index c99c9e6342..34491bf843 100644 --- a/beacon_chain/consensus_object_pools/consensus_manager.nim +++ b/beacon_chain/consensus_object_pools/consensus_manager.nim @@ -487,7 +487,8 @@ proc forkchoiceUpdated( head.blck.markExecutionValid(false) self.attestationPool[].forkChoice.mark_root_invalid(head.blck.root) - self.quarantine[].addUnviable(head.blck.root) + # TODO differentiate invalid execution from invalid consensus + discard self.quarantine[].addUnviable(head.blck.root, UnviableKind.Invalid) false proc updateExecutionHead*( diff --git a/beacon_chain/gossip_processing/block_processor.nim b/beacon_chain/gossip_processing/block_processor.nim index a6d8635e7e..43573d2447 100644 --- a/beacon_chain/gossip_processing/block_processor.nim +++ b/beacon_chain/gossip_processing/block_processor.nim @@ -25,7 +25,8 @@ from ../consensus_object_pools/block_dag import from ../consensus_object_pools/block_pools_types import ChainDAGRef, EpochRef, OnBlockAdded, VerifierError, timeParams from ../consensus_object_pools/block_quarantine import - addSidecarless, addOrphan, addUnviable, pop, removeOrphan, removeSidecarless + addSidecarless, addOrphan, addUnviable, clearProcessing, contains, get, pop, + remove, startProcessing, clearProcessing, UnviableKind from ../consensus_object_pools/blob_quarantine import BlobQuarantine, ColumnQuarantine, popSidecars, put from ../validators/validator_monitor import @@ -146,6 +147,11 @@ proc new*(T: type BlockProcessor, func hasBlocks*(self: BlockProcessor): bool = self.pendingStores > 0 +func toVerifierError(v: UnviableKind): VerifierError = + case v + of UnviableKind.UnviableFork: VerifierError.UnviableFork + of UnviableKind.Invalid: VerifierError.Invalid + # Storage # ------------------------------------------------------------------------------ @@ -240,8 +246,9 @@ proc storeBackfillBlock( signedBlock: ForkySignedBeaconBlock, sidecarsOpt: SomeOptSidecars, ): Result[void, VerifierError] = - # The block is certainly not missing any more - self.consensusManager.quarantine[].missing.del(signedBlock.root) + let quarantine = self.consensusManager.quarantine + # In case the block was added to any part of the quarantine.. + quarantine[].remove(signedBlock) ?verifySidecars(signedBlock, sidecarsOpt) @@ -250,22 +257,33 @@ proc storeBackfillBlock( if res.isErr(): case res.error of VerifierError.MissingParent: - if signedBlock.message.parent_root in - self.consensusManager.quarantine[].unviable: + quarantine[].unviable.get(signedBlock.message.parent_root).isErrOr: # DAG doesn't know about unviable ancestor blocks - we do! Translate # this to the appropriate error so that sync etc doesn't retry the block - self.consensusManager.quarantine[].addUnviable(signedBlock.root) - return err(VerifierError.UnviableFork) + return err(quarantine[].addUnviable(signedBlock.root, value).toVerifierError()) + + # TODO Is the block always from an unviable fork? It didn't match the + # expected backfill block, so we could potentially mark it as + # UnviableFork here + res of VerifierError.UnviableFork: # Track unviables so that descendants can be discarded properly - self.consensusManager.quarantine[].addUnviable(signedBlock.root) - else: discard - return res - - # Only store side cars after successfully establishing block viability. - self.storeSidecars(sidecarsOpt) + err( + quarantine[] + .addUnviable(signedBlock.root, UnviableKind.UnviableFork) + .toVerifierError() + ) + of VerifierError.Invalid: + # TODO track invalid blocks once we can differentiate between invalid + # proposer signature and other errors + res + of VerifierError.Duplicate: + res + else: + # Only store side cars after successfully establishing block viability. + self.storeSidecars(sidecarsOpt) - res + res from web3/engine_api_types import PayloadExecutionStatus from ../el/el_manager import ELManager, DeadlineFuture, sendNewPayload @@ -362,12 +380,12 @@ proc enqueueBlock*( discard self.addBlock(src, blck, sidecarsOpt, maybeFinalized, validationDur) proc enqueueQuarantine(self: ref BlockProcessor, parent: BlockRef) = - ## Enqueue blocks whose parent is `root` - ie when `root` has been added to - ## the blockchain dag, its direct descendants are now candidates for - ## processing + ## Enqueue the blocks that are no longer orphans as a result of `parent` being + ## added to the DAG let dag = self.consensusManager[].dag quarantine = self.consensusManager[].quarantine + for quarantined in quarantine[].pop(parent.root): # Process the blocks that had the newly accepted block as parent debug "Block from quarantine", parent, quarantined = shortLog(quarantined.root) @@ -503,6 +521,7 @@ proc enqueueFromDb(self: ref BlockProcessor, root: Eth2Digest) = let sidecarsOpt = when consensusFork >= ConsensusFork.Fulu: + debugGloasComment "" var data_column_sidecars: fulu.DataColumnSidecars for i in self.dataColumnQuarantine[].custodyColumns: let data_column = fulu.DataColumnSidecar.new() @@ -540,7 +559,7 @@ proc storeBlock( ## storeBlock is the main entry point for unvalidated blocks - all untrusted ## blocks, regardless of origin, pass through here. When storing a block, ## we will add it to the dag and pass it to all block consumers that need - ## to know about it, such as the fork choice and the monitoring + ## to know about it, such as the fork choice and the monitoring. let ap = self.consensusManager.attestationPool @@ -558,36 +577,11 @@ proc storeBlock( chronos.nanoseconds((slotTime - wallTime).nanoseconds) deadline = sleepAsync(deadlineTime) - # If the block is missing its parent, it will be re-orphaned below - self.consensusManager.quarantine[].removeOrphan(signedBlock) - self.consensusManager.quarantine[].removeSidecarless(signedBlock) - # The block is certainly not missing any more - self.consensusManager.quarantine[].missing.del(signedBlock.root) - - if signedBlock.message.parent_root in - self.consensusManager.quarantine[].unviable: - # DAG doesn't know about unviable ancestor blocks - we do however! - return err(VerifierError.UnviableFork) - # We have to be careful that there exists only one in-flight entry point # for adding blocks or the checks performed in `checkHeadBlock` might # be invalidated (ie a block could be added while we wait for EL response # here) - let parent = dag.checkHeadBlock(signedBlock).valueOr: - if error == VerifierError.MissingParent: - # This indicates that no `BlockRef` is available for the `parent_root`. - # However, the block may still be available in local storage. On startup, - # only the canonical branch is imported into `blockchain_dag`, while - # non-canonical branches are re-discovered with sync/request managers. - # Data from non-canonical branches that has already been verified during - # a previous run of the beacon node is already stored in the database but - # only lacks a `BlockRef`. Loading the branch from the database saves a - # lot of time, especially when a non-canonical branch has non-trivial - # depth. Note that if it turns out that a non-canonical branch eventually - # becomes canonical, it is vital to import it as quickly as possible. - self.enqueueFromDb(signedBlock.message.parent_root) - - return err(error) + let parent = ?dag.checkHeadBlock(signedBlock) const consensusFork = typeof(signedBlock).kind let @@ -700,21 +694,26 @@ proc addBlock*( maybeFinalized = false, validationDur = Duration(), ): Future[Result[void, VerifierError]] {.async: (raises: [CancelledError]).} = - ## Enqueue a Gossip-validated block for consensus verification + ## Enqueue a Gossip-validated block for consensus verification - only one + ## block at a time gets processed # Backpressure: - # There is no backpressure here - producers must wait for `resfut` to - # constrain their own processing + # Callers that don't await the returned future are responsible for implementing + # their own backpressure handling, limiting concurrent `addBlock` calls to + # reasonable amounts # Producers: # - Gossip (when synced) # - SyncManager (during sync) # - RequestManager (missing ancestor blocks) # - API - let blockRoot = blck.root + let + blockRoot = blck.root + dag = self.consensusManager.dag + quarantine = self.consensusManager.quarantine logScope: blockRoot = shortLog(blockRoot) - if blck.message.slot <= self.consensusManager.dag.finalizedHead.slot: + if blck.message.slot <= dag.finalizedHead.slot: # let backfill blocks skip the queue - these are always "fast" to process # because there are no state rewinds to deal with return self[].storeBackfillBlock(blck, sidecarsOpt) @@ -732,6 +731,12 @@ proc addBlock*( self.pendingStores += 1 await self.storeLock.acquire() + # Since block processing is async, we want to make sure it doesn't get + # (re)added there while we're busy - the start of processing also removes + # the block from the various quarantines. + # The processing status is cleared in the finally block below. + quarantine[].startProcessing(blck) + # Cooperative concurrency: one block per loop iteration - because # we run both networking and CPU-heavy things like block processing # on the same thread, we need to make sure that there is steady progress @@ -754,6 +759,8 @@ proc addBlock*( src, wallTime, blck, sidecarsOpt, maybeFinalized, queueTick, validationDur ) finally: + quarantine[].clearProcessing() + try: self.storeLock.release() self.pendingStores -= 1 @@ -765,40 +772,63 @@ proc addBlock*( if res.isOk(): # Once a block is successfully stored, enqueue the direct descendants self.enqueueQuarantine(res[]) + res.mapConvert(void) else: case res.error() of VerifierError.MissingParent: - let finalizedSlot = self.consensusManager.dag.finalizedHead.slot - if ( - let r = self.consensusManager.quarantine[].addOrphan(finalizedSlot, blck) - r.isErr() - ): + quarantine[].addOrphan(dag.finalizedHead.slot, blck).isOkOr: debug "Could not add orphan", - blck = shortLog(blck), signature = shortLog(blck.signature), err = r.error() + blck = shortLog(blck), signature = shortLog(blck.signature), err = error + return err(error.toVerifierError()) + + # This indicates that no `BlockRef` is available for the `parent_root`. + # However, the block may still be available in local storage. On startup, + # only the canonical branch is imported into `blockchain_dag`, while + # non-canonical branches are re-discovered with sync/request managers. + # Data from non-canonical branches that has already been verified during + # a previous run of the beacon node is already stored in the database but + # only lacks a `BlockRef`. Loading the branch from the database saves a + # lot of time, especially when a non-canonical branch has non-trivial + # depth. Note that if it turns out that a non-canonical branch eventually + # becomes canonical, it is vital to import it as quickly as possible. + self.enqueueFromDb(blck.message.parent_root) + + when sidecarsOpt is Opt[BlobSidecars]: + if sidecarsOpt.isSome: + self.blobQuarantine[].put(blockRoot, sidecarsOpt.get) + elif sidecarsOpt is Opt[fulu.DataColumnSidecars]: + if sidecarsOpt.isSome: + self.dataColumnQuarantine[].put(blockRoot, sidecarsOpt.get) + elif sidecarsOpt is Opt[gloas.DataColumnSidecars]: + if sidecarsOpt.isSome: + debugGloasComment "" + elif sidecarsOpt is NoSidecars: + discard else: - when sidecarsOpt is Opt[BlobSidecars]: - if sidecarsOpt.isSome: - self.blobQuarantine[].put(blockRoot, sidecarsOpt.get) - elif sidecarsOpt is Opt[fulu.DataColumnSidecars]: - if sidecarsOpt.isSome: - self.dataColumnQuarantine[].put(blockRoot, sidecarsOpt.get) - elif sidecarsOpt is Opt[gloas.DataColumnSidecars]: - if sidecarsOpt.isSome: - debugGloasComment "" - elif sidecarsOpt is NoSidecars: - discard - else: - {.error.} + {.error.} + + debug "Block quarantined", + blck = shortLog(blck), signature = shortLog(blck.signature) - debug "Block quarantined", - blck = shortLog(blck), signature = shortLog(blck.signature) + err(res.error()) of VerifierError.UnviableFork: # Track unviables so that descendants can be discarded promptly - # TODO Invalid and unviable should be treated separately, to correctly - # respond when a descendant of an invalid block is validated - # TODO re-add VeriferError.Invalid handling - self.consensusManager.quarantine[].addUnviable(blockRoot) - else: - discard - - res.mapConvert(void) + err( + self.consensusManager.quarantine[] + .addUnviable(blockRoot, UnviableKind.UnviableFork) + .toVerifierError() + ) + of VerifierError.Invalid: + # TODO track invalid blocks once we can differentiate between invalid + # proposer signature and other errors + # TODO fix https://github.com/status-im/nimbus-eth2/issues/7583 + # before marking as Invalid here to allow retrying with a fresh + # state + # err( + # self.consensusManager.quarantine[] + # .addUnviable(blockRoot, UnviableKind.Invalid) + # .toVerifierError() + # ) + err(res.error()) + of VerifierError.Duplicate: + err(res.error()) diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 12eba89a10..91933c0184 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -53,6 +53,38 @@ template errIgnore*(msg: cstring): untyped = template errReject*(msg: cstring): untyped = err((ValidationResult.Reject, msg)) +template addMissingValid( + quarantine: var Quarantine, root: Eth2Digest, prefix: static string +): untyped = + # Add the given root that is required to be valid, returning a reject if it + # turns out it is not + let missing = quarantine.addMissing(root) + if missing.isOk: + errIgnore(cstring(prefix & " not found")) + else: + case missing.error + of UnviableKind.UnviableFork: + errIgnore(cstring(prefix & " from unviable fork")) + of UnviableKind.Invalid: + errReject(cstring(prefix & " invalid")) + +template addMissingValid( + quarantine: var Quarantine, root, descendant: Eth2Digest, prefix: static string +): untyped = + # Add the given root that is required to be valid, returning a reject if it + # turns out it is not - descendant inherits the viability of the parent + let missing = quarantine.addMissing(root) + if missing.isOk: + errIgnore(cstring(prefix & " not found")) + else: + # The descendant is unviable the same way as the parent! + discard quarantine.addUnviable(descendant, missing.error) + case missing.error + of UnviableKind.UnviableFork: + errIgnore(cstring(prefix & " from unviable fork")) + of UnviableKind.Invalid: + errReject(cstring(prefix & " invalid")) + # Internal checks # ---------------------------------------------------------------- @@ -147,8 +179,9 @@ proc check_beacon_and_target_block( # We rely on the chain DAG to have been validated, so check for the existence # of the block in the pool. let blck = pool.dag.getBlockRef(data.beacon_block_root).valueOr: - pool.quarantine[].addMissing(data.beacon_block_root) - return errIgnore("Attestation block unknown") + return pool.quarantine[].addMissingValid( + data.beacon_block_root, "AttestationData: block" + ) # Not in spec - check that rewinding to the state is sane ? check_attestation_block(pool, data.slot, blck) @@ -356,7 +389,7 @@ template validateBeaconBlockBellatrix( forkyState.data, signed_beacon_block.message.slot) if not (signed_beacon_block.message.body.execution_payload.timestamp == timestampAtSlot): - quarantine[].addUnviable(signed_beacon_block.root) + discard quarantine[].addUnviable(signed_beacon_block.root, UnviableKind.Invalid) return dag.checkedReject( "BeaconBlock: mismatched execution payload timestamp") @@ -403,6 +436,8 @@ proc validateBlobSidecar*( # perform the cheap checks first - in particular, we want to avoid loading # an `EpochRef` and checking signatures. This reordering might lead to # different IGNORE/REJECT results in turn affecting gossip scores. + # If the header is invalid, so is the block that shares its block_root -> + # we can mark those blocks invalid without further processing template block_header: untyped = blob_sidecar.signed_block_header.message # [REJECT] The sidecar's index is consistent with `MAX_BLOBS_PER_BLOCK` @@ -509,18 +544,14 @@ proc validateBlobSidecar*( # [REJECT] The sidecar's block's parent (defined by # `block_header.parent_root`) passes validation. let parent = dag.getBlockRef(block_header.parent_root).valueOr: - if block_header.parent_root in quarantine[].unviable: - # If the parent was unviable, this block is unviable for the same reason - quarantine[].addUnviable(block_root) - # TODO keep track of unviable invalid - return errIgnore("BlobSidecar: parent from unviable fork") - - quarantine[].addMissing(block_header.parent_root) - return errIgnore("BlobSidecar: parent not found") + return quarantine[].addMissingValid( + block_header.parent_root, block_root, "BlobSidecar: parent" + ) # [REJECT] The sidecar is from a higher slot than the sidecar's # block's parent (defined by `block_header.parent_root`). if not (block_header.slot > parent.bid.slot): + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject("BlobSidecar: slot lower than parents'") # [REJECT] The current finalized_checkpoint is an ancestor of the sidecar's @@ -538,7 +569,7 @@ proc validateBlobSidecar*( if not ( finalized_checkpoint.root == ancestor.root or finalized_checkpoint.root.isZero): - quarantine[].addUnviable(block_root) + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject( "BlobSidecar: Finalized checkpoint not an ancestor") @@ -555,6 +586,8 @@ proc validateBlobSidecar*( parent, block_header.slot, block_header.proposer_index, block_root, blob_sidecar.signed_block_header.signature, ).isOkOr: + if error.invalid: + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject(error.msg) # [REJECT] The sidecar's blob is valid as verified by `verify_blob_kzg_proof( @@ -589,6 +622,8 @@ proc validateDataColumnSidecar*( wallTime: BeaconTime, subnet_id: uint64): Result[void, ValidationError] = + # If the header is invalid, so is the block that shares its block_root -> + # we can mark those blocks invalid without further processing template block_header: untyped = data_column_sidecar.signed_block_header.message # [REJECT] The sidecar is valid as verified by verify_data_column_sidecar(sidecar) block: @@ -638,18 +673,14 @@ proc validateDataColumnSidecar*( # [REJECT] The sidecar's block's parent (defined by # `block_header.parent_root`) passes validation. let parent = dag.getBlockRef(block_header.parent_root).valueOr: - if block_header.parent_root in quarantine[].unviable: - # If the parent was unviable, this block is unviable for the same reason - quarantine[].addUnviable(block_root) - # TODO keep track of unviable invalid - return errIgnore("DataColumnSidecar: parent from unviable fork") - - quarantine[].addMissing(block_header.parent_root) - return errIgnore("DataColumnSidecar: parent not found") + return quarantine[].addMissingValid( + block_header.parent_root, block_root, "DataColumnSidecar: parent" + ) # [REJECT] The sidecar is from a higher slot than the sidecar's # block's parent (defined by `block_header.parent_root`). if not (block_header.slot > parent.bid.slot): + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject("DataColumnSidecar: slot lower than parents'") # [REJECT] The current finalized_checkpoint is an ancestor of the sidecar's @@ -667,7 +698,7 @@ proc validateDataColumnSidecar*( if not ( finalized_checkpoint.root == ancestor.root or finalized_checkpoint.root.isZero): - quarantine[].addUnviable(block_root) + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject( "DataColumnSidecar: Finalized checkpoint not an ancestor") @@ -685,6 +716,8 @@ proc validateDataColumnSidecar*( parent, block_header.slot, block_header.proposer_index, block_root, data_column_sidecar.signed_block_header.signature, ).isOkOr: + if error.invalid: + discard quarantine[].addUnviable(block_root, UnviableKind.Invalid) return dag.checkedReject(error.msg) # [REJECT] The sidecar's column data is valid as @@ -842,58 +875,51 @@ proc validateBeaconBlock*( # [REJECT] The block's parent (defined by block.parent_root) # passes validation. let parent = dag.getBlockRef(signed_beacon_block.message.parent_root).valueOr: - if signed_beacon_block.message.parent_root in quarantine[].unviable: - # If the parent was unviable, this block is unviable for the same reason - quarantine[].addUnviable(signed_beacon_block.root) - - # https://github.com/ethereum/consensus-specs/blob/v1.3.0/specs/bellatrix/p2p-interface.md#beacon_block - # `is_execution_enabled(state, block.body)` check, but unlike in - # validateBeaconBlockBellatrix() don't have parent BlockRef. - if signed_beacon_block.message.is_execution_block: - # Blocks with execution enabled will be permitted to propagate - # regardless of the validity of the execution payload. This prevents - # network segregation between optimistic and non-optimistic nodes. - # - # If execution_payload verification of block's parent by an execution - # node is not complete: - # - # - [REJECT] The block's parent (defined by `block.parent_root`) passes - # all validation (excluding execution node verification of the - # `block.body.execution_payload`). - # - # otherwise: - # - # - [IGNORE] The block's parent (defined by `block.parent_root`) passes - # all validation (including execution node verification of the - # `block.body.execution_payload`). - - # Implementation restrictions: - # - # - We know that the parent was marked unviable, but don't know - # whether it was marked unviable due to consensus (REJECT) or - # execution (IGNORE) verification failure. We err on the IGNORE side. - return errIgnore("BeaconBlock: ignored, parent from unviable fork") - else: - # For non-execution blocks, we also don't keep track of unviable forks - # or invalid blocks - # TODO keep track of unviable invalid - return errIgnore("BeaconBlock: ignored, parent from unviable fork") - - # When the parent is missing, we can't validate the block - we'll queue it - # in the quarantine for later processing - if ( - let r = quarantine[].addOrphan(dag.finalizedHead.slot, 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() - else: - debug "Block quarantined", - blockRoot = shortLog(signed_beacon_block.root), - blck = shortLog(signed_beacon_block.message), - signature = shortLog(signed_beacon_block.signature) + # When the parent is missing, we can't validate the block and instead queue + # it for later processing + quarantine[].addOrphan(dag.finalizedHead.slot, signed_beacon_block).isOkOr: + # Queueing failed because the parent was unviable - this means this block + # is unviable as well, for the same reason + return + case error + of UnviableKind.Invalid: + if signed_beacon_block.message.is_execution_block: + # https://github.com/ethereum/consensus-specs/blob/v1.3.0/specs/bellatrix/p2p-interface.md#beacon_block + # + # Blocks with execution enabled will be permitted to propagate + # regardless of the validity of the execution payload. This prevents + # network segregation between optimistic and non-optimistic nodes. + # + # If execution_payload verification of block's parent by an execution + # node is not complete: + # + # - [REJECT] The block's parent (defined by `block.parent_root`) passes + # all validation (excluding execution node verification of the + # `block.body.execution_payload`). + # + # otherwise: + # + # - [IGNORE] The block's parent (defined by `block.parent_root`) passes + # all validation (including execution node verification of the + # `block.body.execution_payload`). + + # Implementation restrictions: + # + # - We know that the parent was marked unviable, but don't know + # whether it was marked unviable due to consensus (REJECT) or + # execution (IGNORE) verification failure. We err on the IGNORE side. + # TODO track this as a separate UnviableKind + errIgnore("BeaconBlock: parent invalid") + else: + errReject("BeaconBlock: parent invalid") + of UnviableKind.UnviableFork: + errIgnore("BeaconBlock: parent from unviable fork") + + debug "Block quarantined", + blockRoot = shortLog(signed_beacon_block.root), + blck = shortLog(signed_beacon_block.message), + signature = shortLog(signed_beacon_block.signature) + return errIgnore("BeaconBlock: parent not found") # Continues block parent validity checking in optimistic case, where it does @@ -924,7 +950,7 @@ proc validateBeaconBlock*( if not ( finalized_checkpoint.root == ancestor.root or finalized_checkpoint.root.isZero): - quarantine[].addUnviable(signed_beacon_block.root) + discard quarantine[].addUnviable(signed_beacon_block.root, UnviableKind.Invalid) return dag.checkedReject( "BeaconBlock: Finalized checkpoint not an ancestor") @@ -941,6 +967,8 @@ proc validateBeaconBlock*( signed_beacon_block.message.proposer_index, signed_beacon_block.root, signed_beacon_block.signature, ).isOkOr: + if error.invalid: + discard quarantine[].addUnviable(signed_beacon_block.root, UnviableKind.Invalid) return dag.checkedReject(error.msg) ok() @@ -1726,11 +1754,7 @@ proc validateSyncCommitteeMessage*( let blockRoot = msg.beacon_block_root blck = dag.getBlockRef(blockRoot).valueOr: - if blockRoot in quarantine[].unviable: - # TODO keep track of unviable invalid blocks - return errIgnore("SyncCommitteeMessage: target from unviable fork") - quarantine[].addMissing(blockRoot) - return errIgnore("SyncCommitteeMessage: target not found") + return quarantine[].addMissingValid(blockRoot, "SyncCommitteeMessage: target") block: # [IGNORE] There has been no other valid sync committee message for the @@ -1852,12 +1876,7 @@ proc validateContribution*( let blockRoot = msg.message.contribution.beacon_block_root blck = dag.getBlockRef(blockRoot).valueOr: - if blockRoot in quarantine[].unviable: - # TODO keep track of unviable invalid blocks - return errIgnore("Contribution: target from unviable fork") - - quarantine[].addMissing(blockRoot) - return errIgnore("Contribution: target not found") + return quarantine[].addMissingValid(blockRoot, "Contribution: target") # [IGNORE] A valid sync committee contribution with equal `slot`, # `beacon_block_root` and `subcommittee_index` whose `aggregation_bits` diff --git a/tests/test_block_processor.nim b/tests/test_block_processor.nim index 5a178b90a5..87caedd41c 100644 --- a/tests/test_block_processor.nim +++ b/tests/test_block_processor.nim @@ -15,7 +15,6 @@ import taskpools, ../beacon_chain/conf, ../beacon_chain/spec/[beaconstate, forks, helpers, state_transition], - ../beacon_chain/spec/datatypes/[deneb, fulu], ../beacon_chain/gossip_processing/block_processor, ../beacon_chain/consensus_object_pools/[ attestation_pool, blockchain_dag, blob_quarantine, block_quarantine, diff --git a/tests/test_block_quarantine.nim b/tests/test_block_quarantine.nim index c19213ed96..d10c3ecb97 100644 --- a/tests/test_block_quarantine.nim +++ b/tests/test_block_quarantine.nim @@ -9,22 +9,22 @@ {.used.} import + chronicles, unittest2, ../beacon_chain/spec/[forks, presets], ../beacon_chain/spec/datatypes/[phase0, deneb], ../beacon_chain/consensus_object_pools/block_quarantine func makeBlock(slot: Slot, parent: Eth2Digest): phase0.SignedBeaconBlock = - var - b = phase0.SignedBeaconBlock( - message: phase0.BeaconBlock(slot: slot, parent_root: parent)) + var b = phase0.SignedBeaconBlock( + message: phase0.BeaconBlock(slot: slot, parent_root: parent) + ) b.root = hash_tree_root(b.message) b func makeBlobbyBlock(slot: Slot, parent: Eth2Digest): deneb.SignedBeaconBlock = - var - b = deneb.SignedBeaconBlock( - message: deneb.BeaconBlock(slot: slot, parent_root: parent)) + var b = + deneb.SignedBeaconBlock(message: deneb.BeaconBlock(slot: slot, parent_root: parent)) b.root = hash_tree_root(b.message) b @@ -41,8 +41,9 @@ suite "Block quarantine": var quarantine = Quarantine.init(defaultRuntimeConfig) - quarantine.addMissing(b1.root) check: + quarantine.addMissing(b1.root).isOk() + FetchRecord(root: b1.root) in quarantine.checkMissing(32) quarantine.addOrphan(Slot 0, b1).isOk @@ -60,7 +61,7 @@ suite "Block quarantine": b5.root in quarantine.sidecarless b6.root in quarantine.sidecarless - quarantine.addUnviable(b4.root) + discard quarantine.addUnviable(b4.root, UnviableKind.UnviableFork) check: (b4.root, ValidatorSig()) notin quarantine.orphans @@ -68,7 +69,7 @@ suite "Block quarantine": b5.root in quarantine.sidecarless b6.root notin quarantine.sidecarless - quarantine.addUnviable(b1.root) + discard quarantine.addUnviable(b1.root, UnviableKind.UnviableFork) check: (b1.root, ValidatorSig()) notin quarantine.orphans @@ -122,16 +123,16 @@ suite "Block quarantine": test "Keep downloading parent chain even if we hit missing limit": var quarantine = Quarantine.init(defaultRuntimeConfig) var blocks = @[makeBlock(Slot 0, ZERO_HASH)] - for i in 0.. Date: Thu, 30 Oct 2025 17:17:30 +0100 Subject: [PATCH 2/3] warning --- beacon_chain/gossip_processing/gossip_validation.nim | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 91933c0184..ebd9995d75 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -49,7 +49,7 @@ type ValidationError* = (ValidationResult, cstring) template errIgnore*(msg: cstring): untyped = - err((ValidationResult.Ignore, cstring msg)) + err((ValidationResult.Ignore, msg)) template errReject*(msg: cstring): untyped = err((ValidationResult.Reject, msg)) From fcbeb18cbc0b59191835ec03cfe143b590938c28 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Sat, 1 Nov 2025 12:01:43 +0100 Subject: [PATCH 3/3] test, cleanups --- AllTests-mainnet.md | 1 + beacon_chain/consensus_object_pools/blob_quarantine.nim | 7 +------ beacon_chain/gossip_processing/eth2_processor.nim | 9 +++++---- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/AllTests-mainnet.md b/AllTests-mainnet.md index 28e85b9fa5..adb1cb912f 100644 --- a/AllTests-mainnet.md +++ b/AllTests-mainnet.md @@ -141,6 +141,7 @@ AllTests-mainnet ```diff + Don't re-download unviable blocks OK + Keep downloading parent chain even if we hit missing limit OK ++ No new missing/orphans while processing OK + Recursive missing parent OK + Unviable smoke test OK ``` diff --git a/beacon_chain/consensus_object_pools/blob_quarantine.nim b/beacon_chain/consensus_object_pools/blob_quarantine.nim index a6a9e62f72..a40845b681 100644 --- a/beacon_chain/consensus_object_pools/blob_quarantine.nim +++ b/beacon_chain/consensus_object_pools/blob_quarantine.nim @@ -496,10 +496,6 @@ proc popSidecars*( ## If some of the blob sidecars are missing Opt.none() is returned. ## If block do not have any blob sidecars Opt.some([]) is returned. - when blck is gloas.SignedBeaconBlock: - quarantine.remove(blockRoot) - return Opt.some(default(seq[ref BlobSidecar])) - let sidecarsCount = len(blck.message.body.blob_kzg_commitments) if sidecarsCount == 0: # Block does not have any blob sidecars. @@ -596,8 +592,7 @@ proc popSidecars*( proc popSidecars*( quarantine: var BlobQuarantine, - blck: deneb.SignedBeaconBlock | electra.SignedBeaconBlock | - fulu.SignedBeaconBlock | gloas.SignedBeaconBlock + blck: deneb.SignedBeaconBlock | electra.SignedBeaconBlock ): Opt[seq[ref BlobSidecar]] = ## Alias for `popSidecars()`. popSidecars(quarantine, blck.root, blck) diff --git a/beacon_chain/gossip_processing/eth2_processor.nim b/beacon_chain/gossip_processing/eth2_processor.nim index b23d1e8520..15ba784a5b 100644 --- a/beacon_chain/gossip_processing/eth2_processor.nim +++ b/beacon_chain/gossip_processing/eth2_processor.nim @@ -338,7 +338,7 @@ proc processBlobSidecar*( else: self.quarantine[].addSidecarless(forkyBlck) else: - raiseAssert "Could not be added as blobless" + raiseAssert "Wrong fork for blob: " & $consensusFork blob_sidecars_received.inc() blob_sidecar_delay.observe(delay.toFloatSeconds()) @@ -382,15 +382,16 @@ proc processDataColumnSidecar*( debug "Data column validated, putting data column in quarantine" self.dataColumnQuarantine[].put(block_root, newClone(dataColumnSidecar)) - if self.quarantine[].sidecarless.hasKey(block_root): + if block_root in self.quarantine[].sidecarless: let cres = self.dataColumnQuarantine[].popSidecars(block_root) if cres.isSome(): - let blck = self.quarantine[].popSidecarless(block_root).valueOr: - raiseAssert "Block should be present at this moment" + let blck = self.quarantine[].popSidecarless(block_root).expect("checked above") withBlck(blck): when (consensusFork >= ConsensusFork.Fulu) and (consensusFork < ConsensusFork.Gloas): self.blockProcessor.enqueueBlock(MsgSource.gossip, forkyBlck, cres) + else: + raiseAssert "Wrong fork for columns: " & $consensusFork data_column_sidecars_received.inc() data_column_sidecar_delay.observe(delay.toFloatSeconds())