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

@@ -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)

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,

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(

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