diff --git a/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool.ts b/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool.ts index f0beddedea15..e98c4810b25b 100644 --- a/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool.ts +++ b/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool.ts @@ -434,7 +434,8 @@ export class AttestationPool { const ownKey = this.getSlotSignerKey(slotNumber, address); const payloadHash = attestation.getPayloadHash(); - await this.attestationPerSlotAndSigner.set(ownKey, attestation.toBuffer()); + // Store the signature in canonical (v ∈ {27, 28}) form; see tryAddCheckpointAttestation. + await this.attestationPerSlotAndSigner.set(ownKey, attestation.withNormalizedSignature().toBuffer()); this.metrics.trackMempoolItemAdded(ownKey); // Track our own payload hash so that an equivocating attestation from another @@ -614,7 +615,10 @@ export class AttestationPool { // equivocations are detected via the multimap but their bytes are not retained. const alreadyHasStored = await this.attestationPerSlotAndSigner.hasAsync(slotSignerKey); if (!alreadyHasStored) { - await this.attestationPerSlotAndSigner.set(slotSignerKey, attestation.toBuffer()); + // Store the signature in canonical (v ∈ {27, 28}) form. `sender` recovered above, so the + // signature is non-empty and safe to normalize; this keeps a yParity-encoded (v = 0/1) variant + // from a malicious peer or an in-flight byte mutation out of the L1 bundle downstream. + await this.attestationPerSlotAndSigner.set(slotSignerKey, attestation.withNormalizedSignature().toBuffer()); this.metrics.trackMempoolItemAdded(slotSignerKey); } diff --git a/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool_test_suite.ts b/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool_test_suite.ts index 19180d9d156d..a568f1805dbc 100644 --- a/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool_test_suite.ts +++ b/yarn-project/p2p/src/mem_pools/attestation_pool/attestation_pool_test_suite.ts @@ -1,7 +1,9 @@ import { IndexWithinCheckpoint, SlotNumber } from '@aztec/foundation/branded-types'; import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; import { Fr } from '@aztec/foundation/curves/bn254'; -import type { BlockProposal, CheckpointAttestation, CheckpointProposalCore } from '@aztec/stdlib/p2p'; +import { Signature } from '@aztec/foundation/eth-signature'; +import type { BlockProposal, CheckpointProposalCore } from '@aztec/stdlib/p2p'; +import { CheckpointAttestation } from '@aztec/stdlib/p2p'; import { CheckpointHeader } from '@aztec/stdlib/rollup'; import { makeBlockHeader, @@ -62,7 +64,44 @@ export function describeAttestationPool(getAttestationPool: () => AttestationPoo expect(a1Buffer).toEqual(expect.arrayContaining(a2Buffer)); }; + /** + * Rewrites the attester signature into yParity form (27 -> 0, 28 -> 1), keeping (r, s). The recovery + * bit is unchanged so it still recovers to the same signer, mimicking a peer that gossips (or mutates + * in flight) a signature with a non-canonical recovery byte. + */ + const toYParityForm = (attestation: CheckpointAttestation): CheckpointAttestation => { + const sig = attestation.signature; + const yParityV = sig.v === 27 ? 0 : sig.v === 28 ? 1 : sig.v; + return new CheckpointAttestation( + attestation.payload, + new Signature(sig.r, sig.s, yParityV), + attestation.proposerSignature, + ); + }; + describe('CheckpointAttestation', () => { + it('normalizes yParity (v=0/v=1) attester signatures on ingress from both gossip and own keys', async () => { + const slotNumber = 421; + const canonical = createCheckpointAttestationsForSlot(slotNumber); + const payloadHash = canonical[0].getPayloadHash(); + const expectedSenders = new Set([canonical[0].getSender()!.toString(), canonical[1].getSender()!.toString()]); + + const gossiped = toYParityForm(canonical[0]); + expect([0, 1]).toContain(gossiped.signature.v); + await ap.tryAddCheckpointAttestation(gossiped); + + const own = toYParityForm(canonical[1]); + expect([0, 1]).toContain(own.signature.v); + await ap.addOwnCheckpointAttestations([own]); + + const retrieved = await ap.getCheckpointAttestationsForSlotAndProposal(SlotNumber(slotNumber), payloadHash); + expect(retrieved.length).toBe(2); + for (const attestation of retrieved) { + expect([27, 28]).toContain(attestation.signature.v); + expect(expectedSenders.has(attestation.getSender()!.toString())).toBe(true); + } + }); + it('should add attestations to pool', async () => { const slotNumber = 420; const archive = Fr.random(); diff --git a/yarn-project/stdlib/src/block/proposal/attestations_and_signers.test.ts b/yarn-project/stdlib/src/block/proposal/attestations_and_signers.test.ts new file mode 100644 index 000000000000..a53d78cc23d6 --- /dev/null +++ b/yarn-project/stdlib/src/block/proposal/attestations_and_signers.test.ts @@ -0,0 +1,61 @@ +import { Buffer32 } from '@aztec/foundation/buffer'; +import { EthAddress } from '@aztec/foundation/eth-address'; +import { Signature } from '@aztec/foundation/eth-signature'; + +import { TEST_COORDINATION_SIGNATURE_CONTEXT } from '../../tests/mocks.js'; +import { CommitteeAttestationsAndSigners } from './attestations_and_signers.js'; +import { CommitteeAttestation } from './committee_attestation.js'; + +function popcount(hex: `0x${string}`): number { + let count = 0; + for (const byte of Buffer.from(hex.slice(2), 'hex')) { + let b = byte; + while (b) { + count += b & 1; + b >>= 1; + } + } + return count; +} + +/** A signing attestation with the given recovery byte and non-zero (r, s). */ +function signing(v: number): CommitteeAttestation { + return new CommitteeAttestation(EthAddress.random(), new Signature(Buffer32.random(), Buffer32.random(), v)); +} + +describe('CommitteeAttestationsAndSigners.packAttestations', () => { + it('keeps the bitmap popcount consistent with getSigners() for yParity (v=0/v=1) signatures', () => { + const attestations = [ + signing(27), + signing(0), // yParity form of a v=27 signature + CommitteeAttestation.fromAddress(EthAddress.random()), // non-signing member (empty signature) + signing(1), // yParity form of a v=28 signature + signing(28), + ]; + const bundle = new CommitteeAttestationsAndSigners(attestations, TEST_COORDINATION_SIGNATURE_CONTEXT); + + const packed = bundle.getPackedAttestations(); + // Four signing members, one empty slot. + expect(bundle.getSigners().length).toEqual(4); + expect(popcount(packed.signatureIndices)).toEqual(bundle.getSigners().length); + }); + + it('packs every signature slot with a canonical v so L1 ECDSA.recover accepts it at proving time', () => { + const attestations = [signing(0), signing(1), signing(27), signing(28)]; + const bundle = new CommitteeAttestationsAndSigners(attestations, TEST_COORDINATION_SIGNATURE_CONTEXT); + + const unpacked = CommitteeAttestation.fromPacked(bundle.getPackedAttestations(), attestations.length); + for (const attestation of unpacked) { + expect([27, 28]).toContain(attestation.signature.v); + } + }); + + it('still packs a genuinely empty signature as an address-only slot', () => { + const address = EthAddress.random(); + const attestations = [signing(27), CommitteeAttestation.fromAddress(address)]; + const bundle = new CommitteeAttestationsAndSigners(attestations, TEST_COORDINATION_SIGNATURE_CONTEXT); + + expect(bundle.getSigners().map(s => s.toString())).not.toContain(address.toString()); + expect(popcount(bundle.getPackedAttestations().signatureIndices)).toEqual(1); + }); +}); diff --git a/yarn-project/stdlib/src/block/proposal/attestations_and_signers.ts b/yarn-project/stdlib/src/block/proposal/attestations_and_signers.ts index 4213fc6abfb5..c6ee4a5f8dc9 100644 --- a/yarn-project/stdlib/src/block/proposal/attestations_and_signers.ts +++ b/yarn-project/stdlib/src/block/proposal/attestations_and_signers.ts @@ -76,8 +76,10 @@ export class CommitteeAttestationsAndSigners implements Signable { let totalDataSize = 0; for (const attestation of viemAttestations) { const signature = attestation.signature; - // Check if signature is empty (v = 0) - const isEmpty = signature.v === 0; + // A slot is empty (a non-signing member, packed as its address) only when r, s and v are all zero + // — matching Signature.isEmpty() and getSigners(), so the bitmap popcount and the signers list can + // never disagree (which would revert propose() with SignersSizeMismatch). + const isEmpty = signature.v === 0 && BigInt(signature.r) === 0n && BigInt(signature.s) === 0n; if (!isEmpty) { totalDataSize += 65; // v (1) + r (32) + s (32) @@ -93,8 +95,8 @@ export class CommitteeAttestationsAndSigners implements Signable { for (const [i, attestation] of viemAttestations.entries()) { const signature = attestation.signature; - // Check if signature is empty - const isEmpty = signature.v === 0; + // Empty iff r, s and v are all zero (see the size-tally loop above). + const isEmpty = signature.v === 0 && BigInt(signature.r) === 0n && BigInt(signature.s) === 0n; if (!isEmpty) { // Set bit in bitmap (bit 7-0 in each byte, left to right) @@ -102,8 +104,10 @@ export class CommitteeAttestationsAndSigners implements Signable { const bitIndex = 7 - (i % 8); signatureIndices[byteIndex] = (signatureIndices[byteIndex] ?? 0) | (1 << bitIndex); - // Pack signature: v + r + s - signaturesOrAddresses[dataIndex] = signature.v; + // Pack signature: v + r + s. Canonicalize a yParity recovery byte (v = 0/1) to 27/28 — it + // recovers to the same signer, but L1 ECDSA.recover only accepts 27/28. Any other value is left + // as-is so a genuinely malformed signature still fails on L1 rather than being silently rewritten. + signaturesOrAddresses[dataIndex] = signature.v === 0 || signature.v === 1 ? signature.v + 27 : signature.v; dataIndex++; // Pack r (32 bytes) diff --git a/yarn-project/stdlib/src/p2p/attestation_utils.test.ts b/yarn-project/stdlib/src/p2p/attestation_utils.test.ts index 29d3dee63a95..457396baf7f5 100644 --- a/yarn-project/stdlib/src/p2p/attestation_utils.test.ts +++ b/yarn-project/stdlib/src/p2p/attestation_utils.test.ts @@ -1,12 +1,14 @@ import { SlotNumber } from '@aztec/foundation/branded-types'; import { Secp256k1Signer } from '@aztec/foundation/crypto/secp256k1-signer'; import { Fr } from '@aztec/foundation/curves/bn254'; +import { Signature } from '@aztec/foundation/eth-signature'; import { jest } from '@jest/globals'; +import { CommitteeAttestationsAndSigners } from '../block/proposal/attestations_and_signers.js'; import { CheckpointHeader } from '../rollup/index.js'; import { TEST_COORDINATION_SIGNATURE_CONTEXT } from '../tests/mocks.js'; -import { trimAttestations } from './attestation_utils.js'; +import { orderAttestations, trimAttestations } from './attestation_utils.js'; import { CheckpointAttestation } from './checkpoint_attestation.js'; import { CheckpointProposal } from './checkpoint_proposal.js'; import { ConsensusPayload } from './consensus_payload.js'; @@ -32,6 +34,86 @@ function makeSignerAndAttestation() { return { signer, attestation: makeAttestation(signer), address: signer.address }; } +/** + * Rewrites the attester signature's recovery byte into yParity form (27 -> 0, 28 -> 1) while keeping + * (r, s). Since the recovery bit is unchanged, the signature still recovers to the same signer, but the + * non-canonical `v` is what a malicious committee member (or a peer mutating the byte in flight) can + * emit to make `packAttestations` and `getSigners` disagree. + */ +function toYParityForm(attestation: CheckpointAttestation): CheckpointAttestation { + const sig = attestation.signature; + const yParityV = sig.v === 27 ? 0 : sig.v === 28 ? 1 : sig.v; + return new CheckpointAttestation( + attestation.payload, + new Signature(sig.r, sig.s, yParityV), + attestation.proposerSignature, + ); +} + +function popcount(hex: `0x${string}`): number { + let count = 0; + for (const byte of Buffer.from(hex.slice(2), 'hex')) { + let b = byte; + while (b) { + count += b & 1; + b >>= 1; + } + } + return count; +} + +describe('orderAttestations', () => { + it('normalizes yParity (v=0/v=1) attester signatures to canonical v', () => { + // Generate enough signers that both recovery bits (v=27 -> 0 and v=28 -> 1) are exercised. + const items = Array.from({ length: 8 }, () => makeSignerAndAttestation()); + const committee = items.map(i => i.address); + const yParityAttestations = items.map(i => toYParityForm(i.attestation)); + + // Sanity check that the fixture actually produced non-canonical signatures for both recovery bits. + const emittedVs = new Set(yParityAttestations.map(a => a.signature.v)); + expect([...emittedVs].every(v => v === 0 || v === 1)).toBe(true); + + const ordered = orderAttestations(yParityAttestations, committee); + + for (const [i, attestation] of ordered.entries()) { + expect([27, 28]).toContain(attestation.signature.v); + expect(attestation.address.toString()).toEqual(committee[i].toString()); + } + }); + + it('produces an L1-consistent bundle (popcount === signers.length) for yParity signatures', () => { + const items = Array.from({ length: 8 }, () => makeSignerAndAttestation()); + const committee = items.map(i => i.address); + const yParityAttestations = items.map(i => toYParityForm(i.attestation)); + + const ordered = orderAttestations(yParityAttestations, committee); + const bundle = new CommitteeAttestationsAndSigners(ordered, TEST_COORDINATION_SIGNATURE_CONTEXT); + + const packed = bundle.getPackedAttestations(); + expect(popcount(packed.signatureIndices)).toEqual(bundle.getSigners().length); + // Every signing slot must carry a canonical v so L1 ECDSA.recover (used at proving time) accepts it. + for (const attestation of ordered) { + if (!attestation.signature.isEmpty()) { + expect([27, 28]).toContain(attestation.signature.v); + } + } + }); + + it('leaves non-signing committee members as empty address-only slots', () => { + const items = Array.from({ length: 3 }, () => makeSignerAndAttestation()); + const missing = Secp256k1Signer.random().address; + const committee = [...items.map(i => i.address), missing]; + + const ordered = orderAttestations( + items.map(i => toYParityForm(i.attestation)), + committee, + ); + + expect(ordered[3].address.toString()).toEqual(missing.toString()); + expect(ordered[3].signature.isEmpty()).toBe(true); + }); +}); + describe('trimAttestations', () => { it('returns attestations unchanged when count <= required', () => { const items = Array.from({ length: 3 }, () => makeSignerAndAttestation()); diff --git a/yarn-project/stdlib/src/p2p/attestation_utils.ts b/yarn-project/stdlib/src/p2p/attestation_utils.ts index 33fb150909af..4a48964b88a5 100644 --- a/yarn-project/stdlib/src/p2p/attestation_utils.ts +++ b/yarn-project/stdlib/src/p2p/attestation_utils.ts @@ -1,3 +1,4 @@ +import { normalizeSignature } from '@aztec/foundation/crypto/secp256k1-signer'; import type { EthAddress } from '@aztec/foundation/eth-address'; import { CommitteeAttestation } from '../block/index.js'; @@ -18,9 +19,15 @@ export function orderAttestations( for (const attestation of attestations) { const sender = attestation.getSender(); if (sender) { + // Canonicalize the recovery byte to v ∈ {27, 28} before it reaches the L1 bundle. A committee + // member (or a peer mutating the byte in flight) can emit an equivalent signature in yParity form + // (v = 0/1); left as-is it makes `packAttestations` (empty iff v === 0) and `getSigners` (empty iff + // r,s,v all zero) disagree, reverting `propose()` with SignersSizeMismatch, and a v = 1 byte would + // later revert epoch proving in ECDSA.recover. `sender` recovered, so the signature is non-empty + // and low-s, and normalizeSignature preserves the recovered address. attestationMap.set( sender.toString(), - CommitteeAttestation.fromAddressAndSignature(sender, attestation.signature), + CommitteeAttestation.fromAddressAndSignature(sender, normalizeSignature(attestation.signature)), ); } } diff --git a/yarn-project/stdlib/src/p2p/checkpoint_attestation.ts b/yarn-project/stdlib/src/p2p/checkpoint_attestation.ts index c054a6c5dc40..7383baad0a52 100644 --- a/yarn-project/stdlib/src/p2p/checkpoint_attestation.ts +++ b/yarn-project/stdlib/src/p2p/checkpoint_attestation.ts @@ -1,5 +1,6 @@ import { CheckpointProposalHash, type SlotNumber } from '@aztec/foundation/branded-types'; import { type BaseBuffer32, Buffer32 } from '@aztec/foundation/buffer'; +import { normalizeSignature } from '@aztec/foundation/crypto/secp256k1-signer'; import type { Fr } from '@aztec/foundation/curves/bn254'; import type { EthAddress } from '@aztec/foundation/eth-address'; import { Signature } from '@aztec/foundation/eth-signature'; @@ -97,6 +98,17 @@ export class CheckpointAttestation extends Gossipable { return this.cachedProposer ?? undefined; } + /** + * Returns a copy with the attester signature canonicalized to v ∈ {27, 28}. Callers store attestations + * received from gossip verbatim; normalizing on ingress keeps a signature emitted in yParity form + * (v = 0/1) — whether by a malicious committee member or a peer mutating the byte in flight — from + * reaching the L1 bundle in a non-canonical form. Must only be called once the signature has recovered + * a sender (so it is non-empty and low-s); `normalizeSignature` throws on an all-zero signature. + */ + withNormalizedSignature(): CheckpointAttestation { + return new CheckpointAttestation(this.payload, normalizeSignature(this.signature), this.proposerSignature); + } + getPayload(): Buffer { return this.payload.getPayloadToSign(); }