BFT-457: Simplify batch signature

[aakoshh]

What ❔

Replaces the AggregateSignature types in attester with a MultiSig using secp256k1 keys and signatures.

Why ❔

Because sepc256k1::AggregateSignature required O(n^2) work in verification, due to losing the information in which order the signatures were added and who they belong to. Since the ability to aggregate signatures over different messages isn't needed for attesters, storing signatures together with the corresponding public key simplifies verification and will make it easier to submit the data to Solidity as well.

[brunoffranca]

Approved! Not relevant for now, but just want to remember that after EIP-2537 gets merged (maybe early next year) attester keys will use BLS and we will have aggregate signatures again.

[pompon0]

@aakoshh can we have keep the AggregatedSignature, so that moving between bls and ecdsa is just swapping implementations? I don't think it is a good idea to remove an abstraction layer which we will reintroduce later, especially since it affects the proto layout.

[aakoshh]

I assume you wish to keep the attester::AggregateSignature, but the secp256k1::Aggregatedignature can go, right? For example there is no ed25519::AggregatedSignature either.

We can keep it, sure. I'm wondering if the bitmap could be moved into it, though, so that it's a bit more self contained? So you would have e.g. bls12_381::AggregateSignature which is just a signature, but unlike how it is in validator::AggregateSignature, the attester::AggregateSignature would contain the Signers bitvector. That way the Batch could have 1 field for this in both cases.

I am guessing you don't want this kind of dissonance though, so, never mind.

[aakoshh]

I'm asking because "just swapping" implementation isn't exactly true if the signers field has to be added back to the Batch, which means further changes in its methods to count weights, etc. Perhaps I can create wrappers around the BTreeMap and move more functionality there, to achieve functional parity (e.g. the aforementioned weight sum), but it would mean the protobuf will not go back to its original format either.

[aakoshh]

I'd also not keep the ability to verify different messages on the attester::AggregateSignature as it's not a real use case for them, if that's okay.

[pompon0]

I mean, yeah, protobuf will contain different data for different signature schemes.
Yes, I meant the attesters::AggregatedSignature, not the secp one, where it doesn't make sense.
Yes, having all signatures for the same message is a way to go.
Signers cannot really go into bls12_381::AggregateSignature because that would require Genesis to be leaked (in some form) to the crypto crate. All that I'm asking is to keep the attesters::AggregatedSignature type (with guts replaced as needed) and keep the API of that type roughly the same as it would be with the bls aggregation.

[aakoshh]

I added the following:

• MultiSig for the current thing
• AggregateSignature wrapping a bls12_381 (at least as a placeholder; EIP-2537 says this is the one)
• AggregateMultiSig pairing up Signers and AggregateSignature
• Both of the aggregates only verify a single message

This doesn't follow the pattern of e.g. CommitQC where the Signers and the AggregateSignature are next to the thing they signed, but I think it makes sense to call this a BLS multi-sig. Easy to drop anyway.

As a side note: since the aggregate_signature module uses Batch, it might as well use Committee, and then the responsibility of checking whether the keys are present in the committee and the sum-of-weights could be moved into the multisig implementations, to unify their API. In practice I expect the simple MultiSig to be dropped when BLS is ready, so it doesn't matter that much.