From e3d4ad5d39a9ff9488139affe1e144eeebe2b2e9 Mon Sep 17 00:00:00 2001
From: tersec <tersec@users.noreply.github.com>
Date: Sun, 9 Jun 2024 17:37:41 +0000
Subject: [PATCH] properly (re)factor consolidation signature checking (#6334)

---
 beacon_chain/el/el_manager.nim               |  1 -
 beacon_chain/spec/signatures.nim             | 20 ++++++++++++++++++++
 beacon_chain/spec/state_transition_block.nim | 20 +++++---------------
 tests/testblockutil.nim                      |  2 --
 4 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/beacon_chain/el/el_manager.nim b/beacon_chain/el/el_manager.nim
index 5081772f4..e204f68db 100644
--- a/beacon_chain/el/el_manager.nim
+++ b/beacon_chain/el/el_manager.nim
@@ -1956,7 +1956,6 @@ proc startExchangeTransitionConfigurationLoop(
 
   while true:
     # https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.3/src/engine/paris.md#specification-3
-    debug "Exchange transition configuration tick"
     await m.exchangeTransitionConfiguration()
     await sleepAsync(60.seconds)
 
diff --git a/beacon_chain/spec/signatures.nim b/beacon_chain/spec/signatures.nim
index 12f2a0c72..1fcb6ca9f 100644
--- a/beacon_chain/spec/signatures.nim
+++ b/beacon_chain/spec/signatures.nim
@@ -421,3 +421,23 @@ proc verify_bls_to_execution_change_signature*(
   let signing_root = compute_bls_to_execution_change_signing_root(
     genesisFork, genesis_validators_root, msg.message)
   blsVerify(pubkey, signing_root.data, signature)
+
+func compute_consolidation_signing_root(
+    genesisFork: Fork, genesis_validators_root: Eth2Digest,
+    msg: Consolidation): Eth2Digest =
+  # Uses genesis fork version regardless
+  doAssert genesisFork.current_version == genesisFork.previous_version
+
+  let domain = compute_domain(
+    DOMAIN_CONSOLIDATION, genesisFork.current_version,
+    genesis_validators_root=genesis_validators_root)
+  compute_signing_root(msg, domain)
+
+proc verify_consolidation_signature*(
+    genesisFork: Fork, genesis_validators_root: Eth2Digest,
+    msg: SignedConsolidation | TrustedSignedConsolidation,
+    pubkeys: openArray[ValidatorPubKey]): bool =
+  withTrust(msg.signature):
+    let signing_root = compute_consolidation_signing_root(
+      genesisFork, genesis_validators_root, msg.message)
+    blsFastAggregateVerify(pubkeys, signing_root.data, msg.signature)
diff --git a/beacon_chain/spec/state_transition_block.nim b/beacon_chain/spec/state_transition_block.nim
index 8025331c5..91947856f 100644
--- a/beacon_chain/spec/state_transition_block.nim
+++ b/beacon_chain/spec/state_transition_block.nim
@@ -644,20 +644,11 @@ proc process_consolidation*(
       target_validator.withdrawal_credentials.data.toOpenArray(12, 31)):
     return err("Consolidation: source and target don't have same withdrawal address")
 
-  debugComment "this is per spec, near-verbatim, but Nimbus generally factors this out into spec/signatures.nim. so, create verify_consolidation_signature infra there, call here"
   # Verify consolidation is signed by the source and the target
-  let
-    domain = compute_domain(
-      DOMAIN_CONSOLIDATION, cfg.GENESIS_FORK_VERSION,
-      genesis_validators_root=state.genesis_validators_root)
-    signing_root = compute_signing_root(consolidation, domain)
-    pubkeys = [source_validator[].pubkey, target_validator.pubkey]
-
-  debugComment "as a good example, this trustedsig hack typically/should live in spec/signatures.nim"
-  when not (signed_consolidation.signature is TrustedSig):
-    if not blsFastAggregateVerify(
-        pubkeys, signing_root.data, signed_consolidation.signature):
-      return err("Consolidation: invalid signature")
+  if not verify_consolidation_signature(
+      cfg.genesisFork, state.genesis_validators_root, signed_consolidation,
+      [source_validator[].pubkey, target_validator.pubkey]):
+    return err("Consolidation: invalid signature")
 
   # Initiate source validator exit and append pending consolidation
   source_validator[].exit_epoch = compute_consolidation_epoch_and_update_churn(
@@ -667,8 +658,7 @@ proc process_consolidation*(
   debugComment "check HashList add return value"
   discard state.pending_consolidations.add(PendingConsolidation(
     source_index: consolidation.source_index,
-    target_index: consolidation.target_index
-  ))
+    target_index: consolidation.target_index))
 
   ok()
 
diff --git a/tests/testblockutil.nim b/tests/testblockutil.nim
index 894faee58..c69a7c4b7 100644
--- a/tests/testblockutil.nim
+++ b/tests/testblockutil.nim
@@ -208,8 +208,6 @@ proc addTestBlock*(
       else:
         default(bellatrix.ExecutionPayloadForSigning)
 
-    debugComment "addTestBlock Electra attestation support"
-
     makeBeaconBlock(
       cfg,
       state,