Implement skip_randao_verification for blinded blocks (#4435)

* Implement skip_randao_verification for blinded blocks

* fix redundant randao verification on block replay (5% faster)
* check randao in REST instead of internally
* avoid redundant copies when making blocks
* cleanup leftover randao skipping code

* fix test summary

AllTests-mainnet.md

@@ -75,13 +75,12 @@ OK: 1/1 Fail: 0/1 Skip: 0/1
 ## Block pool processing [Preset: mainnet]
 ```diff
 + Adding the same block twice returns a Duplicate error [Preset: mainnet]                    OK
-+ Randao skip and non-skip                                                                   OK
 + Simple block add&get [Preset: mainnet]                                                     OK
 + basic ops                                                                                  OK
 + updateHead updates head and headState [Preset: mainnet]                                    OK
 + updateState sanity [Preset: mainnet]                                                       OK
 ```
-OK: 6/6 Fail: 0/6 Skip: 0/6
+OK: 5/5 Fail: 0/5 Skip: 0/5
 ## Block processor [Preset: mainnet]
 ```diff
 + Reverse order block add & get [Preset: mainnet]                                            OK
@@ -615,4 +614,4 @@ OK: 2/2 Fail: 0/2 Skip: 0/2
 OK: 9/9 Fail: 0/9 Skip: 0/9
 
 ---TOTAL---
-OK: 344/349 Fail: 0/349 Skip: 5/349
+OK: 343/348 Fail: 0/348 Skip: 5/348

beacon_chain/extras.nim

@@ -27,9 +27,6 @@ type
     ## Skip verification of BLS signatures in block processing.
     ## Predominantly intended for use in testing, e.g. to allow extra coverage.
     ## Also useful to avoid unnecessary work when replaying known, good blocks.
-    skipRandaoVerification ##\
-    ## Skip verification of the proposer's randao reveal in block processing, but do ensure
-    ## that they set the randao reveal to the point at infinity.
     skipStateRootValidation ##\
     ## Skip verification of block state root.
     strictVerification ##\

beacon_chain/rpc/rest_utils.nim

@@ -308,3 +308,22 @@ const
   jsonMediaType* = MediaType.init("application/json")
   sszMediaType* = MediaType.init("application/octet-stream")
   textEventStreamMediaType* = MediaType.init("text/event-stream")
+
+proc verifyRandao*(
+    node: BeaconNode, slot: Slot, proposer: ValidatorIndex,
+    randao: ValidatorSig, skip_randao_verification: bool): bool =
+  let
+    proposer_pubkey = node.dag.validatorKey(proposer)
+  if proposer_pubkey.isNone():
+    return false
+
+  if skip_randao_verification:
+    randao == ValidatorSig.infinity()
+  else:
+    let
+      fork = node.dag.forkAtEpoch(slot.epoch)
+      genesis_validators_root = node.dag.genesis_validators_root
+
+    verify_epoch_signature(
+      fork, genesis_validators_root, slot.epoch, proposer_pubkey.get(),
+      randao)

beacon_chain/rpc/rest_validator_api.nim

@@ -308,8 +308,9 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
 
   # https://ethereum.github.io/beacon-APIs/#/Validator/produceBlockV2
   router.api(MethodGet, "/eth/v2/validator/blocks/{slot}") do (
-    slot: Slot, randao_reveal: Option[ValidatorSig],
-    graffiti: Option[GraffitiBytes], skip_randao_verification: Option[string]) -> RestApiResponse:
+      slot: Slot, randao_reveal: Option[ValidatorSig],
+      graffiti: Option[GraffitiBytes],
+      skip_randao_verification: Option[string]) -> RestApiResponse:
     let message =
       block:
         let qslot = block:
@@ -363,13 +364,18 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
               return RestApiResponse.jsonError(Http503, BeaconNodeInSyncError,
                                                $res.error())
             res.get()
-        let proposer = node.dag.getProposer(qhead, qslot)
+        let
+          proposer = node.dag.getProposer(qhead, qslot)
         if proposer.isNone():
           return RestApiResponse.jsonError(Http400, ProposerNotFoundError)
+
+        if not node.verifyRandao(
+            qslot, proposer.get(), qrandao, qskip_randao_verification):
+          return RestApiResponse.jsonError(Http400, InvalidRandaoRevealValue)
+
         let res =
           await makeBeaconBlockForHeadAndSlot[bellatrix.ExecutionPayload](
-            node, qrandao, proposer.get(), qgraffiti, qhead, qslot,
-            qskip_randao_verification)
+            node, qrandao, proposer.get(), qgraffiti, qhead, qslot)
         if res.isErr():
           return RestApiResponse.jsonError(Http400, res.error())
         res.get()
@@ -378,8 +384,9 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
   # https://ethereum.github.io/beacon-APIs/#/Validator/produceBlindedBlock
   # https://github.com/ethereum/beacon-APIs/blob/v2.3.0/apis/validator/blinded_block.yaml
   router.api(MethodGet, "/eth/v1/validator/blinded_blocks/{slot}") do (
-    slot: Slot, randao_reveal: Option[ValidatorSig],
-    graffiti: Option[GraffitiBytes]) -> RestApiResponse:
+      slot: Slot, randao_reveal: Option[ValidatorSig],
+      graffiti: Option[GraffitiBytes],
+      skip_randao_verification: Option[string]) -> RestApiResponse:
     ## Requests a beacon node to produce a valid blinded block, which can then
     ## be signed by a validator. A blinded block is a block with only a
     ## transactions root, rather than a full transactions list.
@@ -408,6 +415,15 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
         return RestApiResponse.jsonError(Http400, InvalidSlotValueError,
                                          "Slot cannot be in the future")
       res
+    let qskip_randao_verification =
+      if skip_randao_verification.isNone():
+        false
+      else:
+        let res = skip_randao_verification.get()
+        if res.isErr() or res.get() != "":
+          return RestApiResponse.jsonError(Http400,
+                                            InvalidSkipRandaoVerificationValue)
+        true
     let qrandao =
       if randao_reveal.isNone():
         return RestApiResponse.jsonError(Http400, MissingRandaoRevealValue)
@@ -439,6 +455,10 @@ proc installValidatorApiHandlers*(router: var RestRouter, node: BeaconNode) =
     if proposer.isNone():
       return RestApiResponse.jsonError(Http400, ProposerNotFoundError)
 
+    if not node.verifyRandao(
+        qslot, proposer.get(), qrandao, qskip_randao_verification):
+      return RestApiResponse.jsonError(Http400, InvalidRandaoRevealValue)
+
     template responsePlain(response: untyped): untyped =
       if contentType == sszMediaType:
         RestApiResponse.sszResponse(response)

beacon_chain/spec/state_transition_block.nim

@@ -92,21 +92,15 @@ proc process_randao(
   # Verify RANDAO reveal
   let epoch = state.get_current_epoch()
 
-  if skipRandaoVerification in flags:
-    if body.randao_reveal.toRaw != ValidatorSig.infinity.toRaw:
-      return err("process_randao: expected point-at-infinity for skipRandaoVerification")
-  elif skipBlsValidation notin flags:
+  if skipBlsValidation notin flags and body.randao_reveal isnot TrustedSig:
     let proposer_pubkey = state.validators.item(proposer_index.get).pubkey
 
     # `state_transition.makeBeaconBlock` ensures this is run with a trusted
     # signature, but unless the full skipBlsValidation is specified, RANDAO
     # epoch signatures still have to be verified.
     if not verify_epoch_signature(
         state.fork, state.genesis_validators_root, epoch, proposer_pubkey,
-        when body.randao_reveal is ValidatorSig:
-          body.randao_reveal
-        else:
-          isomorphicCast[ValidatorSig](body.randao_reveal)):
+        body.randao_reveal):
 
       return err("process_randao: invalid epoch signature")
 

beacon_chain/validators/validator_duties.nim

@@ -432,7 +432,6 @@ proc makeBeaconBlockForHeadAndSlot*[EP](
     node: BeaconNode, randao_reveal: ValidatorSig,
     validator_index: ValidatorIndex, graffiti: GraffitiBytes, head: BlockRef,
     slot: Slot,
-    skip_randao_verification_bool: bool,
     execution_payload: Opt[EP],
     transactions_root: Opt[Eth2Digest],
     execution_payload_root: Opt[Eth2Digest]):
@@ -455,9 +454,9 @@ proc makeBeaconBlockForHeadAndSlot*[EP](
   let
     state = maybeState.get
     payloadFut =
-      if executionPayload.isSome:
+      if execution_payload.isSome:
         let fut = newFuture[Opt[EP]]("given-payload")
-        fut.complete(executionPayload)
+        fut.complete(execution_payload)
         fut
       elif slot.epoch < node.dag.cfg.BELLATRIX_FORK_EPOCH or
             not (
@@ -510,10 +509,7 @@ proc makeBeaconBlockForHeadAndSlot*[EP](
       (static(default(SignedBLSToExecutionChangeList))),
       noRollback, # Temporary state - no need for rollback
       cache,
-      # makeBeaconBlock doesn't verify BLS at all, but does have special case
-      # for skipRandaoVerification separately
-      verificationFlags =
-        if skip_randao_verification_bool: {skipRandaoVerification} else: {},
+      verificationFlags = {},
       transactions_root = transactions_root,
       execution_payload_root = execution_payload_root).mapErr do (error: cstring) -> string:
     # This is almost certainly a bug, but it's complex enough that there's a
@@ -530,21 +526,10 @@ proc makeBeaconBlockForHeadAndSlot*[EP](
     node: BeaconNode, randao_reveal: ValidatorSig,
     validator_index: ValidatorIndex, graffiti: GraffitiBytes, head: BlockRef,
     slot: Slot):
-    Future[ForkedBlockResult] {.async.} =
-  return await makeBeaconBlockForHeadAndSlot[EP](
-    node, randao_reveal, validator_index, graffiti, head, slot,
-    false, execution_payload = Opt.none(EP),
-    transactions_root = Opt.none(Eth2Digest),
-    execution_payload_root = Opt.none(Eth2Digest))
-
-proc makeBeaconBlockForHeadAndSlot*[EP](
-    node: BeaconNode, randao_reveal: ValidatorSig,
-    validator_index: ValidatorIndex, graffiti: GraffitiBytes, head: BlockRef,
-    slot: Slot, skip_randao_verification: bool):
-    Future[ForkedBlockResult] {.async.} =
-  return await makeBeaconBlockForHeadAndSlot[EP](
+    Future[ForkedBlockResult] =
+  return makeBeaconBlockForHeadAndSlot[EP](
     node, randao_reveal, validator_index, graffiti, head, slot,
-    skip_randao_verification, execution_payload = Opt.none(EP),
+    execution_payload = Opt.none(EP),
     transactions_root = Opt.none(Eth2Digest),
     execution_payload_root = Opt.none(Eth2Digest))
 
@@ -695,7 +680,6 @@ proc getBlindedBlockParts(
 
   let newBlock = await makeBeaconBlockForHeadAndSlot[bellatrix.ExecutionPayload](
     node, randao, validator_index, graffiti, head, slot,
-    skip_randao_verification_bool = false,
     execution_payload = Opt.some shimExecutionPayload,
     transactions_root = Opt.some executionPayloadHeader.get.transactions_root,
     execution_payload_root =

tests/test_blockchain_dag.nim

@@ -206,74 +206,6 @@ suite "Block pool processing" & preset():
       # Getting an EpochRef should not result in states being stored
       db.getStateRoot(stateCheckpoint.bid.root, stateCheckpoint.slot).isOk()
 
-  test "Randao skip and non-skip":
-    process_slots(
-      dag.cfg, state[], getStateField(
-        state[], slot) + 1, cache, info, {}).expect("can advance 1")
-
-    let
-      proposer_index = get_beacon_proposer_index(
-        state[], cache, getStateField(state[], slot))
-      privKey = MockPrivKeys[proposer_index.get]
-      randao_reveal = get_epoch_signature(
-        getStateField(state[], fork),
-        getStateField(state[], genesis_validators_root),
-        getStateField(state[], slot).epoch, privKey).toValidatorSig()
-    var bad_randao_reveal = randao_reveal
-    bad_randao_reveal.blob[5] += 1
-
-    block: # bad randao + no skip = bad
-      let
-        tmpState = assignClone(state[])
-        message = makeBeaconBlock(
-          dag.cfg, tmpState[], proposer_index.get(),
-          bad_randao_reveal,
-          getStateField(tmpState[], eth1_data),
-          default(GraffitiBytes), @[], @[], BeaconBlockExits(),
-          default(SyncAggregate), default(ExecutionPayload),
-          default(SignedBLSToExecutionChangeList),
-          noRollback, cache)
-      check: message.isErr
-
-    block: # bad randao + skip = bad
-      let
-        tmpState = assignClone(state[])
-        message = makeBeaconBlock(
-          dag.cfg, tmpState[], proposer_index.get(),
-          bad_randao_reveal,
-          getStateField(tmpState[], eth1_data),
-          default(GraffitiBytes), @[], @[], BeaconBlockExits(),
-          default(SyncAggregate), default(ExecutionPayload),
-          default(SignedBLSToExecutionChangeList),
-          noRollback, cache, {skipRandaoVerification})
-      check: message.isErr
-
-    block: # infinity + no skip = bad
-      let
-        tmpState = assignClone(state[])
-        message = makeBeaconBlock(
-          dag.cfg, tmpState[], proposer_index.get(),
-          ValidatorSig.infinity(),
-          getStateField(tmpState[], eth1_data),
-          default(GraffitiBytes), @[], @[], BeaconBlockExits(),
-          default(SyncAggregate), default(ExecutionPayload),
-          default(SignedBLSToExecutionChangeList),
-          noRollback, cache, {})
-      check: message.isErr
-
-    block: # Infinity + skip = ok!
-      let
-        tmpState = assignClone(state[])
-        message = makeBeaconBlock(
-          dag.cfg, tmpState[], proposer_index.get(),
-          ValidatorSig.infinity(),
-          getStateField(tmpState[], eth1_data),
-          default(GraffitiBytes), @[], @[], BeaconBlockExits(),
-          default(SyncAggregate), default(ExecutionPayload),
-          default(SignedBLSToExecutionChangeList),
-          noRollback, cache, {skipRandaoVerification})
-      check: message.isOk
-
   test "Adding the same block twice returns a Duplicate error" & preset():
     let
       b10 = dag.addHeadBlock(verifier, b1, nilPhase0Callback)