From e243ba2c0bf77853a9c08da3af2df6e79f3741f0 Mon Sep 17 00:00:00 2001 From: Etan Kissling Date: Wed, 29 Sep 2021 14:10:44 +0200 Subject: [PATCH] revise `makeBeaconBlock` overloads (#2879) The phase0 and altair overloads of `makeBeaconBlock` slightly differ in their signatures which makes using them unnecessarily verbose. - A placeholder `sync_aggregate` argument similar to `executionPayload` is added to the phase0 overload to match the altair signature. - A wrapper operating on `ForkedHashedBeaconState` is introduced. --- beacon_chain/spec/state_transition.nim | 152 +++++++++++++++---- beacon_chain/validators/validator_duties.nim | 80 ++++------ research/block_sim.nim | 77 ++++------ tests/testblockutil.nim | 76 +++------- 4 files changed, 206 insertions(+), 179 deletions(-) diff --git a/beacon_chain/spec/state_transition.nim b/beacon_chain/spec/state_transition.nim index df7236a7b..16db19a75 100644 --- a/beacon_chain/spec/state_transition.nim +++ b/beacon_chain/spec/state_transition.nim @@ -338,7 +338,7 @@ proc state_transition*( cfg, state, signedBlock, cache, flags, rollback) # https://github.com/ethereum/eth2.0-specs/blob/v1.0.1/specs/phase0/validator.md#preparing-for-a-beaconblock -proc makeBeaconBlock*( +template partialBeaconBlock( cfg: RuntimeConfig, state: var phase0.HashedBeaconState, proposer_index: ValidatorIndex, @@ -351,17 +351,9 @@ proc makeBeaconBlock*( proposerSlashings: seq[ProposerSlashing], attesterSlashings: seq[AttesterSlashing], voluntaryExits: seq[SignedVoluntaryExit], - executionPayload: ExecutionPayload, - rollback: RollbackHashedProc, - cache: var StateCache): Result[phase0.BeaconBlock, string] = - ## Create a block for the given state. The last block applied to it must be - ## the one identified by parent_root and process_slots must be called up to - ## the slot for which a block is to be created. - - # To create a block, we'll first apply a partial block to the state, skipping - # some validations. - - var blck = phase0.BeaconBlock( + sync_aggregate: SyncAggregate, + executionPayload: ExecutionPayload): phase0.BeaconBlock = + phase0.BeaconBlock( slot: state.data.slot, proposer_index: proposer_index.uint64, parent_root: parent_root, @@ -378,6 +370,35 @@ proc makeBeaconBlock*( voluntary_exits: List[SignedVoluntaryExit, Limit MAX_VOLUNTARY_EXITS](voluntaryExits))) +proc makeBeaconBlock*( + cfg: RuntimeConfig, + state: var phase0.HashedBeaconState, + proposer_index: ValidatorIndex, + parent_root: Eth2Digest, + randao_reveal: ValidatorSig, + eth1_data: Eth1Data, + graffiti: GraffitiBytes, + attestations: seq[Attestation], + deposits: seq[Deposit], + proposerSlashings: seq[ProposerSlashing], + attesterSlashings: seq[AttesterSlashing], + voluntaryExits: seq[SignedVoluntaryExit], + sync_aggregate: SyncAggregate, + executionPayload: ExecutionPayload, + rollback: RollbackHashedProc, + cache: var StateCache): Result[phase0.BeaconBlock, string] = + ## Create a block for the given state. The last block applied to it must be + ## the one identified by parent_root and process_slots must be called up to + ## the slot for which a block is to be created. + + # To create a block, we'll first apply a partial block to the state, skipping + # some validations. + + var blck = partialBeaconBlock(cfg, state, proposer_index, parent_root, + randao_reveal, eth1_data, graffiti, attestations, deposits, + proposerSlashings, attesterSlashings, voluntaryExits, + sync_aggregate, executionPayload) + let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache) if res.isErr: @@ -395,6 +416,39 @@ proc makeBeaconBlock*( ok(blck) # https://github.com/ethereum/eth2.0-specs/blob/v1.1.0-alpha.7/specs/altair/validator.md#preparing-a-beaconblock +template partialBeaconBlock( + cfg: RuntimeConfig, + state: var altair.HashedBeaconState, + proposer_index: ValidatorIndex, + parent_root: Eth2Digest, + randao_reveal: ValidatorSig, + eth1_data: Eth1Data, + graffiti: GraffitiBytes, + attestations: seq[Attestation], + deposits: seq[Deposit], + proposerSlashings: seq[ProposerSlashing], + attesterSlashings: seq[AttesterSlashing], + voluntaryExits: seq[SignedVoluntaryExit], + sync_aggregate: SyncAggregate, + executionPayload: ExecutionPayload): altair.BeaconBlock = + altair.BeaconBlock( + slot: state.data.slot, + proposer_index: proposer_index.uint64, + parent_root: parent_root, + body: altair.BeaconBlockBody( + randao_reveal: randao_reveal, + eth1_data: eth1data, + graffiti: graffiti, + proposer_slashings: List[ProposerSlashing, Limit MAX_PROPOSER_SLASHINGS]( + proposerSlashings), + attester_slashings: List[AttesterSlashing, Limit MAX_ATTESTER_SLASHINGS]( + attesterSlashings), + attestations: List[Attestation, Limit MAX_ATTESTATIONS](attestations), + deposits: List[Deposit, Limit MAX_DEPOSITS](deposits), + voluntary_exits: + List[SignedVoluntaryExit, Limit MAX_VOLUNTARY_EXITS](voluntaryExits), + sync_aggregate: sync_aggregate)) + proc makeBeaconBlock*( cfg: RuntimeConfig, state: var altair.HashedBeaconState, @@ -419,23 +473,10 @@ proc makeBeaconBlock*( # To create a block, we'll first apply a partial block to the state, skipping # some validations. - var blck = altair.BeaconBlock( - slot: state.data.slot, - proposer_index: proposer_index.uint64, - parent_root: parent_root, - body: altair.BeaconBlockBody( - randao_reveal: randao_reveal, - eth1_data: eth1data, - graffiti: graffiti, - proposer_slashings: List[ProposerSlashing, Limit MAX_PROPOSER_SLASHINGS]( - proposerSlashings), - attester_slashings: List[AttesterSlashing, Limit MAX_ATTESTER_SLASHINGS]( - attesterSlashings), - attestations: List[Attestation, Limit MAX_ATTESTATIONS](attestations), - deposits: List[Deposit, Limit MAX_DEPOSITS](deposits), - voluntary_exits: - List[SignedVoluntaryExit, Limit MAX_VOLUNTARY_EXITS](voluntaryExits), - sync_aggregate: sync_aggregate)) + var blck = partialBeaconBlock(cfg, state, proposer_index, parent_root, + randao_reveal, eth1_data, graffiti, attestations, deposits, + proposerSlashings, attesterSlashings, voluntaryExits, + sync_aggregate, executionPayload) let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache) @@ -452,3 +493,56 @@ proc makeBeaconBlock*( state.root = hash_tree_root(state.data) blck.state_root = state.root ok(blck) + +proc makeBeaconBlock*( + cfg: RuntimeConfig, + state: var ForkedHashedBeaconState, + proposer_index: ValidatorIndex, + parent_root: Eth2Digest, + randao_reveal: ValidatorSig, + eth1_data: Eth1Data, + graffiti: GraffitiBytes, + attestations: seq[Attestation], + deposits: seq[Deposit], + proposerSlashings: seq[ProposerSlashing], + attesterSlashings: seq[AttesterSlashing], + voluntaryExits: seq[SignedVoluntaryExit], + sync_aggregate: SyncAggregate, + executionPayload: ExecutionPayload, + rollback: RollbackForkedHashedProc, + cache: var StateCache): Result[ForkedBeaconBlock, string] = + ## Create a block for the given state. The last block applied to it must be + ## the one identified by parent_root and process_slots must be called up to + ## the slot for which a block is to be created. + + template makeBeaconBlock(kind: untyped): Result[ForkedBeaconBlock, string] = + # To create a block, we'll first apply a partial block to the state, skipping + # some validations. + + var blck = + ForkedBeaconBlock.init( + partialBeaconBlock(cfg, state.`hbs kind`, proposer_index, parent_root, + randao_reveal, eth1_data, graffiti, attestations, deposits, + proposerSlashings, attesterSlashings, voluntaryExits, + sync_aggregate, executionPayload)) + + let res = process_block(cfg, state.`hbs kind`.data, blck.`kind Block`, + {skipBlsValidation}, cache) + + if res.isErr: + warn "Unable to apply new block to state", + blck = shortLog(blck), + slot = state.`hbs kind`.data.slot, + eth1_deposit_index = state.`hbs kind`.data.eth1_deposit_index, + deposit_root = shortLog(state.`hbs kind`.data.eth1_data.deposit_root), + error = res.error + rollback(state) + return err("Unable to apply new block to state: " & $res.error()) + + state.`hbs kind`.root = hash_tree_root(state.`hbs kind`.data) + blck.`kind Block`.state_root = state.`hbs kind`.root + ok(blck) + + case state.beaconStateFork + of forkPhase0: makeBeaconBlock(phase0) + of forkAltair, forkMerge: makeBeaconBlock(altair) diff --git a/beacon_chain/validators/validator_duties.nim b/beacon_chain/validators/validator_duties.nim index c4d3a15f3..639c7aa7e 100644 --- a/beacon_chain/validators/validator_duties.nim +++ b/beacon_chain/validators/validator_duties.nim @@ -425,58 +425,40 @@ proc makeBeaconBlockForHeadAndSlot*(node: BeaconNode, error "Eth1 deposits not available. Skipping block proposal", slot return ForkedBlockResult.err("Eth1 deposits not available") + func restore(v: var ForkedHashedBeaconState) = + # TODO address this ugly workaround - there should probably be a + # `state_transition` that takes a `StateData` instead and updates + # the block as well + doAssert v.addr == addr proposalStateAddr.data + assign(proposalStateAddr[], poolPtr.headState) + + template makeBeaconBlock(kind: untyped): untyped = + makeBeaconBlock( + node.dag.cfg, + stateData.data, + validator_index, + head.root, + randao_reveal, + eth1Proposal.vote, + graffiti, + node.attestationPool[].getAttestationsForBlock( + stateData.data.`hbs kind`, cache), + eth1Proposal.deposits, + node.exitPool[].getProposerSlashingsForBlock(), + node.exitPool[].getAttesterSlashingsForBlock(), + node.exitPool[].getVoluntaryExitsForBlock(), + sync_aggregate, + default(ExecutionPayload), + restore, + cache) + let doPhase0 = slot.epoch < node.dag.cfg.ALTAIR_FORK_EPOCH return if doPhase0: - func restore(v: var phase0.HashedBeaconState) = - # TODO address this ugly workaround - there should probably be a - # `state_transition` that takes a `StateData` instead and updates - # the block as well - doAssert v.addr == addr proposalStateAddr.data.hbsPhase0 - assign(proposalStateAddr[], poolPtr.headState) - - makeBeaconBlock( - node.dag.cfg, - stateData.data.hbsPhase0, - validator_index, - head.root, - randao_reveal, - eth1Proposal.vote, - graffiti, - node.attestationPool[].getAttestationsForBlock( - stateData.data.hbsPhase0, cache), - eth1Proposal.deposits, - node.exitPool[].getProposerSlashingsForBlock(), - node.exitPool[].getAttesterSlashingsForBlock(), - node.exitPool[].getVoluntaryExitsForBlock(), - default(ExecutionPayload), - restore, - cache).map(proc (t: auto): auto = ForkedBeaconBlock.init(t)) + let sync_aggregate = SyncAggregate(sync_committee_signature: ValidatorSig.infinity) + makeBeaconBlock(phase0) else: - func restore(v: var altair.HashedBeaconState) = - # TODO address this ugly workaround - there should probably be a - # `state_transition` that takes a `StateData` instead and updates - # the block as well - doAssert v.addr == addr proposalStateAddr.data.hbsAltair - assign(proposalStateAddr[], poolPtr.headState) - - makeBeaconBlock( - node.dag.cfg, - stateData.data.hbsAltair, - validator_index, - head.root, - randao_reveal, - eth1Proposal.vote, - graffiti, - node.attestationPool[].getAttestationsForBlock( - stateData.data.hbsAltair, cache), - eth1Proposal.deposits, - node.exitPool[].getProposerSlashingsForBlock(), - node.exitPool[].getAttesterSlashingsForBlock(), - node.exitPool[].getVoluntaryExitsForBlock(), - node.sync_committee_msg_pool[].produceSyncAggregate(head.root), - default(ExecutionPayload), - restore, - cache).map(proc (t: auto): auto = ForkedBeaconBlock.init(t)) + let sync_aggregate = node.sync_committee_msg_pool[].produceSyncAggregate(head.root) + makeBeaconBlock(altair) proc proposeSignedBlock*(node: BeaconNode, head: BlockRef, diff --git a/research/block_sim.nim b/research/block_sim.nim index 9ec2e0975..69ed627b2 100644 --- a/research/block_sim.nim +++ b/research/block_sim.nim @@ -235,6 +235,13 @@ cli do(slots = SLOTS_PER_EPOCH * 6, stateData.data, finalizedEpochRef.eth1_data, finalizedEpochRef.eth1_deposit_index) + sync_aggregate = + when T is phase0.SignedBeaconBlock: + SyncAggregate(sync_committee_signature: ValidatorSig.infinity) + elif T is altair.SignedBeaconBlock: + syncCommitteePool[].produceSyncAggregate(dag.head.root) + else: + static: doAssert false hashedState = when T is phase0.SignedBeaconBlock: addr stateData.data.hbsPhase0 @@ -242,56 +249,26 @@ cli do(slots = SLOTS_PER_EPOCH * 6, addr stateData.data.hbsAltair else: static: doAssert false - - # TODO this is ugly, to need to almost-but-not-quite-identical calls to - # makeBeaconBlock. Add a quasi-dummy SyncAggregate param to the phase 0 - # makeBeaconBlock, to avoid code duplication. - # - # One could combine these "when"s, but this "when" should disappear. - message = - when T is phase0.SignedBeaconBlock: - makeBeaconBlock( - cfg, - hashedState[], - proposerIdx, - dag.head.root, - privKey.genRandaoReveal( - getStateField(stateData.data, fork), - getStateField(stateData.data, genesis_validators_root), - slot).toValidatorSig(), - eth1ProposalData.vote, - default(GraffitiBytes), - attPool.getAttestationsForTestBlock(stateData, cache), - eth1ProposalData.deposits, - @[], - @[], - @[], - ExecutionPayload(), - noRollback, - cache) - elif T is altair.SignedBeaconBlock: - makeBeaconBlock( - cfg, - hashedState[], - proposerIdx, - dag.head.root, - privKey.genRandaoReveal( - getStateField(stateData.data, fork), - getStateField(stateData.data, genesis_validators_root), - slot).toValidatorSig(), - eth1ProposalData.vote, - default(GraffitiBytes), - attPool.getAttestationsForTestBlock(stateData, cache), - eth1ProposalData.deposits, - @[], - @[], - @[], - syncCommitteePool[].produceSyncAggregate(dag.head.root), - ExecutionPayload(), - noRollback, - cache) - else: - static: doAssert false + message = makeBeaconBlock( + cfg, + hashedState[], + proposerIdx, + dag.head.root, + privKey.genRandaoReveal( + getStateField(stateData.data, fork), + getStateField(stateData.data, genesis_validators_root), + slot).toValidatorSig(), + eth1ProposalData.vote, + default(GraffitiBytes), + attPool.getAttestationsForTestBlock(stateData, cache), + eth1ProposalData.deposits, + @[], + @[], + @[], + sync_aggregate, + default(ExecutionPayload), + noRollback, + cache) var newBlock = T( diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim index 0891b2dd0..7dd657827 100644 --- a/tests/testblockutil.nim +++ b/tests/testblockutil.nim @@ -102,61 +102,35 @@ proc addTestBlock*( else: ValidatorSig() - let message = - case state.beaconStateFork - of forkPhase0: - let res = makeBeaconBlock( - cfg, - state.hbsPhase0, - proposer_index.get(), - parent_root, - randao_reveal, - # Keep deposit counts internally consistent. - Eth1Data( - deposit_root: eth1_data.deposit_root, - deposit_count: getStateField(state, eth1_deposit_index) + deposits.lenu64, - block_hash: eth1_data.block_hash), - graffiti, - attestations, - deposits, - @[], - @[], - @[], - default(ExecutionPayload), - noRollback, - cache) - doAssert res.isOk(), "Should have created a valid block!" - ForkedBeaconBlock.init(res.get()) - of forkAltair, forkMerge: - let res = makeBeaconBlock( - cfg, - state.hbsAltair, - proposer_index.get(), - parent_root, - randao_reveal, - # Keep deposit counts internally consistent. - Eth1Data( - deposit_root: eth1_data.deposit_root, - deposit_count: getStateField(state, eth1_deposit_index) + deposits.lenu64, - block_hash: eth1_data.block_hash), - graffiti, - attestations, - deposits, - @[], - @[], - @[], - SyncAggregate( - sync_committee_signature: ValidatorSig.infinity), - default(ExecutionPayload), - noRollback, - cache) - doAssert res.isOk(), "Should have created a valid block!" - ForkedBeaconBlock.init(res.get()) + let + message = makeBeaconBlock( + cfg, + state, + proposer_index.get(), + parent_root, + randao_reveal, + # Keep deposit counts internally consistent. + Eth1Data( + deposit_root: eth1_data.deposit_root, + deposit_count: getStateField(state, eth1_deposit_index) + deposits.lenu64, + block_hash: eth1_data.block_hash), + graffiti, + attestations, + deposits, + @[], + @[], + @[], + SyncAggregate(sync_committee_signature: ValidatorSig.infinity), + default(ExecutionPayload), + noRollback, + cache) + + doAssert message.isOk(), "Should have created a valid block!" let new_block = signBlock( getStateField(state, fork), - getStateField(state, genesis_validators_root), message, privKey, + getStateField(state, genesis_validators_root), message.get(), privKey, flags) new_block