Bugfix: Invalid blocks were produced in the presence of invalid deposits (#3639)

Since we were not verifying BLS signature in blocks that we produce,
we were failing to notice that some deposits need to be ignored (due
to having an invalid signature). Processing these deposits resulted
in a different ending state after the state transition which caused
our blocks to be rejected by the network.

Author: zah

beacon_chain/spec/state_transition.nim

@@ -329,7 +329,11 @@ proc makeBeaconBlock*(
     sync_aggregate: SyncAggregate,
     execution_payload: ExecutionPayload,
     rollback: RollbackHashedProc[phase0.HashedBeaconState],
-    cache: var StateCache): Result[phase0.BeaconBlock, cstring] =
+    cache: var StateCache,
+    # TODO:
+    # `verificationFlags` is needed only in tests and can be
+    # removed if we don't use invalid signatures there
+    verificationFlags: UpdateFlags = {}): Result[phase0.BeaconBlock, cstring] =
   ## Create a block for the given state. The latest block applied to it will
   ## be used for the parent_root value, and the slot will be take from
   ## state.slot meaning process_slots must be called up to the slot for which
@@ -342,7 +346,7 @@ proc makeBeaconBlock*(
                                 randao_reveal, eth1_data, graffiti, attestations, deposits,
                                 exits, sync_aggregate, execution_payload)
 
-  let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache)
+  let res = process_block(cfg, state.data, blck, verificationFlags, cache)
 
   if res.isErr:
     rollback(state)
@@ -394,7 +398,11 @@ proc makeBeaconBlock*(
     sync_aggregate: SyncAggregate,
     execution_payload: ExecutionPayload,
     rollback: RollbackHashedProc[altair.HashedBeaconState],
-    cache: var StateCache): Result[altair.BeaconBlock, cstring] =
+    cache: var StateCache,
+    # TODO:
+    # `verificationFlags` is needed only in tests and can be
+    # removed if we don't use invalid signatures there
+    verificationFlags: UpdateFlags = {}): Result[altair.BeaconBlock, cstring] =
   ## Create a block for the given state. The latest block applied to it will
   ## be used for the parent_root value, and the slot will be take from
   ## state.slot meaning process_slots must be called up to the slot for which
@@ -407,7 +415,7 @@ proc makeBeaconBlock*(
                                 randao_reveal, eth1_data, graffiti, attestations, deposits,
                                 exits, sync_aggregate, execution_payload)
 
-  let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache)
+  let res = process_block(cfg, state.data, blck, verificationFlags, cache)
 
   if res.isErr:
     rollback(state)
@@ -460,7 +468,11 @@ proc makeBeaconBlock*(
     sync_aggregate: SyncAggregate,
     execution_payload: ExecutionPayload,
     rollback: RollbackHashedProc[bellatrix.HashedBeaconState],
-    cache: var StateCache): Result[bellatrix.BeaconBlock, cstring] =
+    cache: var StateCache,
+    # TODO:
+    # `verificationFlags` is needed only in tests and can be
+    # removed if we don't use invalid signatures there
+    verificationFlags: UpdateFlags = {}): Result[bellatrix.BeaconBlock, cstring] =
   ## Create a block for the given state. The latest block applied to it will
   ## be used for the parent_root value, and the slot will be take from
   ## state.slot meaning process_slots must be called up to the slot for which
@@ -473,7 +485,7 @@ proc makeBeaconBlock*(
                                 randao_reveal, eth1_data, graffiti, attestations, deposits,
                                 exits, sync_aggregate, execution_payload)
 
-  let res = process_block(cfg, state.data, blck, {skipBlsValidation}, cache)
+  let res = process_block(cfg, state.data, blck, verificationFlags, cache)
 
   if res.isErr:
     rollback(state)
@@ -497,7 +509,11 @@ proc makeBeaconBlock*(
     sync_aggregate: SyncAggregate,
     executionPayload: ExecutionPayload,
     rollback: RollbackForkedHashedProc,
-    cache: var StateCache): Result[ForkedBeaconBlock, cstring] =
+    cache: var StateCache,
+    # TODO:
+    # `verificationFlags` is needed only in tests and can be
+    # removed if we don't use invalid signatures there
+    verificationFlags: UpdateFlags = {}): Result[ForkedBeaconBlock, cstring] =
   ## Create a block for the given state. The latest block applied to it will
   ## be used for the parent_root value, and the slot will be take from
   ## state.slot meaning process_slots must be called up to the slot for which
@@ -514,8 +530,7 @@ proc makeBeaconBlock*(
                            exits, sync_aggregate, executionPayload))
 
     let res = process_block(cfg, state.`kind Data`.data, blck.`kind Data`,
-                            {skipBlsValidation}, cache)
-
+                            verificationFlags, cache)
     if res.isErr:
       rollback(state)
       return err(res.error())

beacon_chain/spec/state_transition_block.nim

@@ -296,7 +296,7 @@ proc process_deposit*(cfg: RuntimeConfig,
   else:
     # Verify the deposit signature (proof of possession) which is not checked
     # by the deposit contract
-    if skipBlsValidation in flags or verify_deposit_signature(cfg, deposit.data):
+    if verify_deposit_signature(cfg, deposit.data):
       # New validator! Add validator and balance entries
       if not state.validators.add(get_validator_from_deposit(deposit.data)):
         return err("process_deposit: too many validators")

ncli/resttest-rules.json

@@ -2972,18 +2972,6 @@
       "status": {"operator": "equals", "value": "400"}
     }
   },
-  {
-    "topics": ["validator", "blocks"],
-    "request": {
-      "url": "/eth/v1/validator/blocks/1?randao_reveal=0x97897b5e8526b4d0f808e7b60bcd1942935b124720bd5156da54c54adc25fe458ef7c934b4e5018afe4659978b06e6510797e5cc7fc31f329035ec6a46889ee9aea375d57b22be71dd4ff181b7f1a07b9199e73c2b80e39e04ba904596d9e4db",
-      "headers": {"Accept": "application/json"}
-    },
-    "response": {
-      "status": {"operator": "equals", "value": "200"},
-      "headers": [{"key": "Content-Type", "value": "application/json", "operator": "equals"}],
-      "body": [{"operator": "jstructcmps", "start": ["data"], "value": {"slot": "", "proposer_index": "", "parent_root": "", "state_root": "", "body": {"randao_reveal": "", "eth1_data": {"deposit_root": "", "deposit_count": "", "block_hash": ""}, "graffiti": "", "proposer_slashings": [], "attester_slashings": [], "attestations": [], "deposits": [], "voluntary_exits": []}}}]
-    }
-  },
   {
     "topics": ["validator", "blocksV2"],
     "request": {
@@ -2994,18 +2982,6 @@
       "status": {"operator": "equals", "value": "400"}
     }
   },
-  {
-    "topics": ["validator", "blocksV2"],
-    "request": {
-      "url": "/eth/v2/validator/blocks/1?randao_reveal=0x97897b5e8526b4d0f808e7b60bcd1942935b124720bd5156da54c54adc25fe458ef7c934b4e5018afe4659978b06e6510797e5cc7fc31f329035ec6a46889ee9aea375d57b22be71dd4ff181b7f1a07b9199e73c2b80e39e04ba904596d9e4db",
-      "headers": {"Accept": "application/json"}
-    },
-    "response": {
-      "status": {"operator": "equals", "value": "200"},
-      "headers": [{"key": "Content-Type", "value": "application/json", "operator": "equals"}],
-      "body": [{"operator": "jstructcmps", "start": ["data"], "value": {"slot": "", "proposer_index": "", "parent_root": "", "state_root": "", "body": {"randao_reveal": "", "eth1_data": {"deposit_root": "", "deposit_count": "", "block_hash": ""}, "graffiti": "", "proposer_slashings": [], "attester_slashings": [], "attestations": [], "deposits": [], "voluntary_exits": []}}}]
-    }
-  },
   {
     "topics": ["validator", "attestation_data"],
     "request": {

tests/testblockutil.nim

@@ -124,9 +124,11 @@ proc addTestBlock*(
       sync_aggregate,
       default(ExecutionPayload),
       noRollback,
-      cache)
+      cache,
+      verificationFlags = {skipBlsValidation})
 
-  doAssert message.isOk(), "Should have created a valid block!"
+  if message.isErr:
+    raiseAssert "Failed to create a block: " & $message.error
 
   let
     new_block = signBlock(