From 07e2df4dd1122b2bd1b55b128bc25b3cda4e432f Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Mon, 7 Mar 2022 21:52:58 +0100 Subject: [PATCH] relax `BlockRef` database assumptions * remove `getForkedBlock(BlockRef)` which assumes block data exists but doesn't support archive/backfilled blocks * fix REST `/eth/v1/beacon/headers` request not returning archive/backfilled blocks * avoid re-encoding in REST block SSZ requests (using `getBlockSSZ`) --- .../attestation_pool.nim | 5 +- .../consensus_object_pools/block_dag.nim | 2 +- .../consensus_object_pools/blockchain_dag.nim | 7 -- .../gossip_processing/gossip_validation.nim | 41 ++++++----- beacon_chain/rpc/rest_beacon_api.nim | 69 +++++++++++-------- beacon_chain/rpc/rest_utils.nim | 36 ++-------- beacon_chain/rpc/rpc_beacon_api.nim | 9 ++- ncli/ncli_db.nim | 2 +- tests/test_block_dag.nim | 6 +- tests/test_blockchain_dag.nim | 2 +- 10 files changed, 89 insertions(+), 90 deletions(-) diff --git a/beacon_chain/consensus_object_pools/attestation_pool.nim b/beacon_chain/consensus_object_pools/attestation_pool.nim index ad5379a1c1..f010734e45 100644 --- a/beacon_chain/consensus_object_pools/attestation_pool.nim +++ b/beacon_chain/consensus_object_pools/attestation_pool.nim @@ -138,8 +138,9 @@ proc init*(T: type AttestationPool, dag: ChainDAGRef, else: epochRef = dag.getEpochRef(blckRef, blckRef.slot.epoch, false).expect( "Getting an EpochRef should always work for non-finalized blocks") - - withBlck(dag.getForkedBlock(blckRef)): + let blck = dag.getForkedBlock(blckRef.bid).expect( + "Should be able to load initial fork choice blocks") + withBlck(blck): forkChoice.process_block( dag, epochRef, blckRef, blck.message, blckRef.slot.start_beacon_time) diff --git a/beacon_chain/consensus_object_pools/block_dag.nim b/beacon_chain/consensus_object_pools/block_dag.nim index 9aa798a081..3ebf155d70 100644 --- a/beacon_chain/consensus_object_pools/block_dag.nim +++ b/beacon_chain/consensus_object_pools/block_dag.nim @@ -198,7 +198,7 @@ func toBlockSlotId*(bs: BlockSlot): BlockSlotId = func isProposed*(bid: BlockId, slot: Slot): bool = ## Return true if `bid` was proposed in the given slot - bid.slot == slot + bid.slot == slot and not bid.root.isZero func isProposed*(blck: BlockRef, slot: Slot): bool = ## Return true if `blck` was proposed in the given slot diff --git a/beacon_chain/consensus_object_pools/blockchain_dag.nim b/beacon_chain/consensus_object_pools/blockchain_dag.nim index 65d500f8f1..ba0941f511 100644 --- a/beacon_chain/consensus_object_pools/blockchain_dag.nim +++ b/beacon_chain/consensus_object_pools/blockchain_dag.nim @@ -68,7 +68,6 @@ template withStateVars*( template stateRoot(): Eth2Digest {.inject, used.} = getStateRoot(stateDataInternal.data) template blck(): BlockRef {.inject, used.} = stateDataInternal.blck - template root(): Eth2Digest {.inject, used.} = stateDataInternal.data.root body @@ -429,11 +428,6 @@ proc getForkedBlock*( result.err() return -proc getForkedBlock*( - dag: ChainDAGRef, blck: BlockRef): ForkedTrustedSignedBeaconBlock = - dag.getForkedBlock(blck.bid).expect( - "BlockRef block should always load, database corrupt?") - proc getForkedBlock*( dag: ChainDAGRef, root: Eth2Digest): Opt[ForkedTrustedSignedBeaconBlock] = let bid = dag.getBlockId(root) @@ -1220,7 +1214,6 @@ proc updateStateData*( found, assignDur, replayDur - elif ancestors.len > 0: debug "State replayed", blocks = ancestors.len, diff --git a/beacon_chain/gossip_processing/gossip_validation.nim b/beacon_chain/gossip_processing/gossip_validation.nim index 5e6311838b..d18fad79bf 100644 --- a/beacon_chain/gossip_processing/gossip_validation.nim +++ b/beacon_chain/gossip_processing/gossip_validation.nim @@ -205,19 +205,24 @@ template validateBeaconBlockBellatrix( # mostly relevant around merge transition epochs. It's possible that # the previous block is phase 0 or Altair, if this is the transition # block itself. - let blockData = dag.getForkedBlock(parent) - case blockData.kind: - of BeaconBlockFork.Phase0: + let blockData = dag.getForkedBlock(parent.bid) + if blockData.isOk(): + case blockData.get().kind: + of BeaconBlockFork.Phase0: + false + of BeaconBlockFork.Altair: + false + of BeaconBlockFork.Bellatrix: + # https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/bellatrix/beacon-chain.md#process_execution_payload + # shows how this gets folded into the state each block; checking this + # is equivalent, without ever requiring state replay or any similarly + # expensive computation. + blockData.get().bellatrixData.message.body.execution_payload != + default(ExecutionPayload) + else: + warn "Cannot load block parent, assuming execution is disabled", + parent = shortLog(parent) false - of BeaconBlockFork.Altair: - false - of BeaconBlockFork.Bellatrix: - # https://github.com/ethereum/consensus-specs/blob/v1.1.10/specs/bellatrix/beacon-chain.md#process_execution_payload - # shows how this gets folded into the state each block; checking this - # is equivalent, without ever requiring state replay or any similarly - # expensive computation. - blockData.bellatrixData.message.body.execution_payload != - default(ExecutionPayload) if executionEnabled: # [REJECT] The block's execution payload timestamp is correct with respect @@ -294,11 +299,13 @@ proc validateBeaconBlock*( if slotBlock.isProposed() and slotBlock.blck.slot == signed_beacon_block.message.slot: - let data = dag.getForkedBlock(slotBlock.blck) - if getForkedBlockField(data, proposer_index) == - signed_beacon_block.message.proposer_index and - data.signature.toRaw() != signed_beacon_block.signature.toRaw(): - return errIgnore("BeaconBlock: already proposed in the same slot") + let curBlock = dag.getForkedBlock(slotBlock.blck.bid) + if curBlock.isOk(): + let data = curBlock.get() + if getForkedBlockField(data, proposer_index) == + signed_beacon_block.message.proposer_index and + data.signature.toRaw() != signed_beacon_block.signature.toRaw(): + return errIgnore("BeaconBlock: already proposed in the same slot") # [IGNORE] The block's parent (defined by block.parent_root) has been seen # (via both gossip and non-gossip sources) (a client MAY queue blocks for diff --git a/beacon_chain/rpc/rest_beacon_api.nim b/beacon_chain/rpc/rest_beacon_api.nim index d15e6788cb..ab18469862 100644 --- a/beacon_chain/rpc/rest_beacon_api.nim +++ b/beacon_chain/rpc/rest_beacon_api.nim @@ -682,15 +682,9 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = $rroot.error()) return RestApiResponse.jsonError(Http500, NoImplementationError) - let blck = - block: - let res = node.getCurrentBlock(qslot) - if res.isErr(): - return RestApiResponse.jsonError(Http404, BlockNotFoundError, - $res.error()) - res.get() + let bdata = node.getForkedBlock(BlockIdent.init(qslot)).valueOr: + return RestApiResponse.jsonError(Http404, BlockNotFoundError) - let bdata = node.dag.getForkedBlock(blck) return withBlck(bdata): RestApiResponse.jsonResponse( @@ -765,13 +759,16 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = router.api(MethodGet, "/eth/v1/beacon/blocks/{block_id}") do ( block_id: BlockIdent) -> RestApiResponse: let - bid = block_id.valueOr: + blockIdent = block_id.valueOr: return RestApiResponse.jsonError(Http400, InvalidBlockIdValueError, $error) - - bdata = node.getForkedBlock(bid).valueOr: + bid = node.getBlockId(blockIdent).valueOr: return RestApiResponse.jsonError(Http404, BlockNotFoundError) + if node.dag.cfg.blockForkAtEpoch(bid.slot.epoch) != BeaconBlockFork.Phase0: + return RestApiResponse.jsonError( + Http404, BlockNotFoundError, "v1 API supports only phase 0 blocks") + let contentType = block: let res = preferredContentType(jsonMediaType, @@ -779,28 +776,38 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = if res.isErr(): return RestApiResponse.jsonError(Http406, ContentNotAcceptableError) res.get() + return - case bdata.kind - of BeaconBlockFork.Phase0: - if contentType == sszMediaType: - RestApiResponse.sszResponse(bdata.phase0Data) - elif contentType == jsonMediaType: - RestApiResponse.jsonResponse(bdata.phase0Data) + if contentType == sszMediaType: + var data: seq[byte] + if not node.dag.getBlockSSZ(bid, data): + return RestApiResponse.jsonError(Http404, BlockNotFoundError) + + RestApiResponse.response(data, Http200, $sszMediaType) + elif contentType == jsonMediaType: + let bdata = node.dag.getForkedBlock(bid).valueOr: + return RestApiResponse.jsonError(Http404, BlockNotFoundError) + + if bdata.kind == BeaconBlockFork.Phase0: + RestApiResponse.jsonResponse(bdata.phase0Data.asSigned()) else: - RestApiResponse.jsonError(Http500, InvalidAcceptError) - of BeaconBlockFork.Altair, BeaconBlockFork.Bellatrix: - RestApiResponse.jsonError(Http404, BlockNotFoundError) + # Shouldn't happen, but in case there's some weird block database + # issue.. + RestApiResponse.jsonError( + Http404, BlockNotFoundError, "v1 API supports only phase 0 blocks") + else: + RestApiResponse.jsonError(Http500, InvalidAcceptError) # https://ethereum.github.io/beacon-APIs/#/Beacon/getBlockV2 router.api(MethodGet, "/eth/v2/beacon/blocks/{block_id}") do ( block_id: BlockIdent) -> RestApiResponse: let - bid = block_id.valueOr: + blockIdent = block_id.valueOr: return RestApiResponse.jsonError(Http400, InvalidBlockIdValueError, $error) - - bdata = node.getForkedBlock(bid).valueOr: + bid = node.getBlockId(blockIdent).valueOr: return RestApiResponse.jsonError(Http404, BlockNotFoundError) + let contentType = block: let res = preferredContentType(jsonMediaType, @@ -810,9 +817,15 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = res.get() return if contentType == sszMediaType: - withBlck(bdata): - RestApiResponse.sszResponse(blck) + var data: seq[byte] + if not node.dag.getBlockSSZ(bid, data): + return RestApiResponse.jsonError(Http404, BlockNotFoundError) + + RestApiResponse.response(data, Http200, $sszMediaType) elif contentType == jsonMediaType: + let bdata = node.dag.getForkedBlock(bid).valueOr: + return RestApiResponse.jsonError(Http404, BlockNotFoundError) + RestApiResponse.jsonResponsePlain(bdata.asSigned()) else: RestApiResponse.jsonError(Http500, InvalidAcceptError) @@ -821,14 +834,14 @@ proc installBeaconApiHandlers*(router: var RestRouter, node: BeaconNode) = router.api(MethodGet, "/eth/v1/beacon/blocks/{block_id}/root") do ( block_id: BlockIdent) -> RestApiResponse: let - bid = block_id.valueOr: + blockIdent = block_id.valueOr: return RestApiResponse.jsonError(Http400, InvalidBlockIdValueError, $error) - blck = node.getBlockId(bid).valueOr: + bid = node.getBlockId(blockIdent).valueOr: return RestApiResponse.jsonError(Http404, BlockNotFoundError) - return RestApiResponse.jsonResponse((root: blck.root)) + return RestApiResponse.jsonResponse((root: bid.root)) # https://ethereum.github.io/beacon-APIs/#/Beacon/getBlockAttestations router.api(MethodGet, diff --git a/beacon_chain/rpc/rest_utils.nim b/beacon_chain/rpc/rest_utils.nim index 4035fcbc62..9a0d116d8c 100644 --- a/beacon_chain/rpc/rest_utils.nim +++ b/beacon_chain/rpc/rest_utils.nim @@ -46,14 +46,6 @@ func getCurrentSlot*(node: BeaconNode, slot: Slot): else: err("Requesting slot too far ahead of the current head") -func getCurrentBlock*(node: BeaconNode, slot: Slot): - Result[BlockRef, cstring] = - let bs = node.dag.getBlockAtSlot(? node.getCurrentSlot(slot)) - if bs.isProposed(): - ok(bs.blck) - else: - err("Block not found") - proc getCurrentHead*(node: BeaconNode, slot: Slot): Result[BlockRef, cstring] = let res = node.dag.head # if not(node.isSynced(res)): @@ -95,7 +87,7 @@ proc getBlockSlot*(node: BeaconNode, ok(node.dag.head.atEpochStart(getStateField( node.dag.headState.data, current_justified_checkpoint).epoch)) -proc getBlockId*(node: BeaconNode, id: BlockIdent): Result[BlockId, cstring] = +proc getBlockId*(node: BeaconNode, id: BlockIdent): Opt[BlockId] = case id.kind of BlockQueryKind.Named: case id.value @@ -106,33 +98,19 @@ proc getBlockId*(node: BeaconNode, id: BlockIdent): Result[BlockId, cstring] = of BlockIdentType.Finalized: ok(node.dag.finalizedHead.blck.bid) of BlockQueryKind.Root: - node.dag.getBlockId(id.root).orErr(cstring("Block not found")) + node.dag.getBlockId(id.root) of BlockQueryKind.Slot: let bsid = node.dag.getBlockIdAtSlot(id.slot) if bsid.isProposed(): ok bsid.bid else: - err("Block not found") + err() proc getForkedBlock*(node: BeaconNode, id: BlockIdent): - Result[ForkedTrustedSignedBeaconBlock, cstring] = - case id.kind - of BlockQueryKind.Named: - case id.value - of BlockIdentType.Head: - ok(node.dag.getForkedBlock(node.dag.head)) - of BlockIdentType.Genesis: - ok(node.dag.getForkedBlock(node.dag.genesis)) - of BlockIdentType.Finalized: - ok(node.dag.getForkedBlock(node.dag.finalizedHead.blck)) - of BlockQueryKind.Root: - node.dag.getForkedBlock(id.root).orErr(cstring("Block not found")) - of BlockQueryKind.Slot: - let bsid = node.dag.getBlockIdAtSlot(id.slot) - if bsid.isProposed(): - node.dag.getForkedBlock(bsid.bid).orErr(cstring("Block not found")) - else: - err("Block not found") + Opt[ForkedTrustedSignedBeaconBlock] = + let bid = ? node.getBlockId(id) + + node.dag.getForkedBlock(bid) proc disallowInterruptionsAux(body: NimNode) = for n in body: diff --git a/beacon_chain/rpc/rpc_beacon_api.nim b/beacon_chain/rpc/rpc_beacon_api.nim index 0117446d44..70539b1588 100644 --- a/beacon_chain/rpc/rpc_beacon_api.nim +++ b/beacon_chain/rpc/rpc_beacon_api.nim @@ -159,11 +159,14 @@ proc getForkedBlockFromBlockId( raises: [Defect, CatchableError].} = case blockId: of "head": - node.dag.getForkedBlock(node.dag.head) + node.dag.getForkedBlock(node.dag.head.bid).valueOr: + raise newException(CatchableError, "Block not found") of "genesis": - node.dag.getForkedBlock(node.dag.genesis) + node.dag.getForkedBlock(node.dag.genesis.bid).valueOr: + raise newException(CatchableError, "Block not found") of "finalized": - node.dag.getForkedBlock(node.dag.finalizedHead.blck) + node.dag.getForkedBlock(node.dag.finalizedHead.blck.bid).valueOr: + raise newException(CatchableError, "Block not found") else: if blockId.startsWith("0x"): let diff --git a/ncli/ncli_db.nim b/ncli/ncli_db.nim index 1852437275..a701b7a665 100644 --- a/ncli/ncli_db.nim +++ b/ncli/ncli_db.nim @@ -956,7 +956,7 @@ proc cmdValidatorDb(conf: DbConf, cfg: RuntimeConfig) = clear cache for bi in 0 ..< blockRefs.len: - let forkedBlock = dag.getForkedBlock(blockRefs[blockRefs.len - bi - 1]) + let forkedBlock = dag.getForkedBlock(blockRefs[blockRefs.len - bi - 1].bid).get() withBlck(forkedBlock): processSlots(blck.message.slot, {skipLastStateRootCalculation}) diff --git a/tests/test_block_dag.nim b/tests/test_block_dag.nim index 70ed9e5393..b9e6779f30 100644 --- a/tests/test_block_dag.nim +++ b/tests/test_block_dag.nim @@ -83,10 +83,14 @@ suite "BlockSlot and helpers": test "parent sanity": let + root = block: + var d: Eth2Digest + d.data[0] = 1 + d s0 = BlockRef(bid: BlockId(slot: Slot(0))) s00 = BlockSlot(blck: s0, slot: Slot(0)) s01 = BlockSlot(blck: s0, slot: Slot(1)) - s2 = BlockRef(bid: BlockId(slot: Slot(2)), parent: s0) + s2 = BlockRef(bid: BlockId(slot: Slot(2), root: root), parent: s0) s22 = BlockSlot(blck: s2, slot: Slot(2)) s24 = BlockSlot(blck: s2, slot: Slot(4)) diff --git a/tests/test_blockchain_dag.nim b/tests/test_blockchain_dag.nim index e0e36db74e..28f84bc431 100644 --- a/tests/test_blockchain_dag.nim +++ b/tests/test_blockchain_dag.nim @@ -541,7 +541,7 @@ suite "chain DAG finalization tests" & preset(): assign(tmpStateData[], dag.headState) check: dag.updateStateData(tmpStateData[], cur.atSlot(cur.slot), false, cache) - dag.getForkedBlock(cur).phase0Data.message.state_root == + dag.getForkedBlock(cur.bid).get().phase0Data.message.state_root == getStateRoot(tmpStateData[].data) getStateRoot(tmpStateData[].data) == hash_tree_root( tmpStateData[].data.phase0Data.data)