From ea16edd886f2005a844c80c56a9399768d2cf2e6 Mon Sep 17 00:00:00 2001
From: Etan Kissling <etan@status.im>
Date: Sat, 27 Jul 2024 01:31:05 +0200
Subject: [PATCH] add test for shuffled attestation signatures (#6459)

Followup of #3212 to test proper signature verification. Also document
possible further optimization based on blst `v0.3.13`.
---
 .../gossip_processing/batch_validation.nim    | 27 ++++---------------
 tests/test_gossip_validation.nim              | 22 +++++++++++++++
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/beacon_chain/gossip_processing/batch_validation.nim b/beacon_chain/gossip_processing/batch_validation.nim
index 39e0743a9..5d8445e64 100644
--- a/beacon_chain/gossip_processing/batch_validation.nim
+++ b/beacon_chain/gossip_processing/batch_validation.nim
@@ -208,16 +208,6 @@ proc complete(batchCrypto: var BatchCrypto, batch: var Batch, ok: bool) =
 
     reset(batchCrypto.counts)
 
-func combine(a: var Signature, b: Signature) =
-  var tmp = AggregateSignature.init(CookedSig(a))
-  tmp.aggregate(b)
-  a = Signature(tmp.finish())
-
-func combine(a: var PublicKey, b: PublicKey) =
-  var tmp = AggregatePublicKey.init(CookedPubKey(a))
-  tmp.aggregate(b)
-  a = PublicKey(tmp.finish())
-
 proc batchVerifyTask(task: ptr BatchTask) {.nimcall.} =
   # Task suitable for running in taskpools - look, no GC!
   let
@@ -366,18 +356,11 @@ proc verifySoon(
     batch = batchCrypto[].getBatch()
     fut = newFuture[BatchResult](name)
 
-  var found = false
-  # Find existing signature sets with the same message - if we can verify an
-  # aggregate instead of several signatures, that is _much_ faster
-  for item in batch[].sigsets.mitems():
-    if item.message == sigset.message:
-      item.signature.combine(sigset.signature)
-      item.pubkey.combine(sigset.pubkey)
-      found = true
-      break
-
-  if not found:
-    batch[].sigsets.add sigset
+  # TODO If there is a signature set `item in batch[].sigsets.mitems()`
+  # with `item.message == sigset.message`, further performance could be gained
+  # by implementing Pippenger multi-scalar multiplication in `nim-blscurve`.
+  # https://gist.github.com/wemeetagain/d52fc4b077f80db6e423935244c2afb2
+  batch[].sigsets.add sigset
 
   # We need to keep the "original" sigset to allow verifying each signature
   # one by one in the case the combined operation fails
diff --git a/tests/test_gossip_validation.nim b/tests/test_gossip_validation.nim
index 9c167db5c..c6084fd06 100644
--- a/tests/test_gossip_validation.nim
+++ b/tests/test_gossip_validation.nim
@@ -181,6 +181,28 @@ suite "Gossip validation " & preset():
         fut_1_0.waitFor().error()[0] == ValidationResult.Reject
         fut_1_1.waitFor().isOk()
 
+    block:
+      pool[].nextAttestationEpoch.setLen(0) # reset for test
+      check:
+        att_1_0.data == att_1_1.data
+        beacon_committee[0] != beacon_committee[1]  # Different validator
+      var
+        broken_1_0 = att_1_0
+        broken_1_1 = att_1_1
+      broken_1_0.signature = att_1_1.signature
+      broken_1_1.signature = att_1_0.signature
+      # The signatures were swapped and no longer match their pubkeys;
+      # the individual attestations are invalid but their aggregate validates!
+      let
+        fut_1_0 = validateAttestation(
+          pool, batchCrypto, broken_1_0, beaconTime, subnet, true)
+        fut_1_1 = validateAttestation(
+          pool, batchCrypto, broken_1_1, beaconTime, subnet, true)
+
+      check:
+        fut_1_0.waitFor().error()[0] == ValidationResult.Reject
+        fut_1_1.waitFor().error()[0] == ValidationResult.Reject
+
 suite "Gossip validation - Altair":
   let cfg = block:
     var res = defaultRuntimeConfig