Add Aggregator interface and implement fake aggregator #35
Conversation
ranchalp
force-pushed
the
aggregator
branch
2 times, most recently
from
afb26f9 to
e81ff83
Compare
f3/api.go
Outdated
| // Aggregates signatures from a participant to an existing signature. | ||
| Aggregate(sig []byte, aggSignature []byte) []byte | ||
| // VerifyAggregate verifies an aggregate signature. | ||
| VerifyAggregate(payload, aggSig []byte, signers map[ActorID]struct{}) bool |
There was a problem hiding this comment.
This API seems to assume that the implementor knows how to get a public key for an ActorID somewhere. It works for the fake impl, but I don't think will for the real one. I think instead signers should just be a list of public keys, so that it becomes a pure function.
There was a problem hiding this comment.
Actually, I think it would be most natural for aggregate signature itself to encode the list of signers (e.g., as a bitmask with respect to the publicly known power table).
There was a problem hiding this comment.
f3 will need to know about public keys either way, as it needs to store them in the power table/state. Yes, we could abstract pubkeys out in this interface, but then it means that the implementer of the API has to have access to the context-sensitive (per granite instance) power table
There was a problem hiding this comment.
Implemented the verification of the proper public keys as listed suggested in a previous PR
|
Thanks again! addressed the comment |
ranchalp
force-pushed
the
aggregator
branch
7 times, most recently
from
2ebfadd to
11fe486
Compare
There was a problem hiding this comment.
Let's merge the Signer.Verify method into the Aggregator interface (but rename it to SignatureVerifier), and then the SIgner interface only does actual signing (in non-fake this will delegate all the way out to Lotus).
Change the signing scheme to be concat the pubkey to the message, and verification (which should take the pubkey as a parameter instead of ActorID) to check that concatenation.
For aggregation, output payload ++ pubkeys (you can read the pubkey out of the input signature). Verify the new signature is unique before aggregation (error if not). Then verifying the aggregate is easy because the parameters are exactly payload and pubkeys.
There is coupling between the signing algorithm and verification, but that's the case for real signatures too!
f3/api.go
Outdated
| @@ -51,11 +51,19 @@ type Signer interface { | |||
| Verify(sender ActorID, msg, sig []byte) bool | |||
| } | |||
|
|
|||
| type Aggregator interface { | |||
| // Aggregates signatures from a participant to an existing signature. | |||
| Aggregate(sig []byte, aggSignature []byte) []byte | |||
There was a problem hiding this comment.
I suggest having this interface take a vector of signatures to be aggregated into the output all at once. The implementation can do the loop over one-at-a-time if that's the only way to implement it, but if faster methods exist for batch operations, it can do them too.
Allow the accumulator aggSignature to be an empty slice.
f3/api.go
Outdated
| // Aggregates signatures from a participant to an existing signature. | ||
| Aggregate(sig []byte, aggSignature []byte) []byte | ||
| // VerifyAggregate verifies an aggregate signature. | ||
| VerifyAggregate(payload, aggSig []byte, signers [][]byte) bool |
There was a problem hiding this comment.
Looks good, but let's define a type PubKey []byte to distinguish what the different byte slices are.
ranchalp
force-pushed
the
aggregator
branch
4 times, most recently
from
b676a04 to
6ee56ae
Compare
|
@Kubuxu new commits in the PR removed code from previous commits. Probably much better to check files changed instead of reviewing commit by commit. |
sim/network.go
Outdated
| } | ||
|
|
||
| for i := 0; i < len(sigs); i++ { | ||
| if !reflect.DeepEqual(msg, sigs[i][pubKeyLen:]) { |
| // Fake implementation. | ||
| // Just appends signature to aggregate signature. | ||
| // This fake aggregation is not commutative (order matters) | ||
| // But determinism is preserved by sorting by weight both here and in VerifyAggregate |
There was a problem hiding this comment.
IMO: the deterministic sorting should happen outside the interface to match real signature schemes which can also have this behaviour.
There was a problem hiding this comment.
But that would mean forcing outside the interface a sorting that may not be required by all signature schemes. Would it not be better to have it inside the signature schemes that need it only?
There was a problem hiding this comment.
(I get optimizing if the real BLS is not commutative, and coupling in that particular instantiation of the interface, but I also mean in particular in the case of the sorting required by this general-purpose (or so-intending) fake aggregation and verification scheme, that just happens to need to sort)
There was a problem hiding this comment.
I see your point, and I don't know which approach is better at this moment. I mentioned it because this requirement was unclear to me, and I immediately highlighted it as a possible difference between test/sim and reality.
We can keep order independence internal, but then we should highlight it in the interface documentation.
There was a problem hiding this comment.
We can keep order independence internal, but then we should highlight it in the interface documentation.
Sounds good, I suggest going back to this when we discuss interfaces in #42 not to duplicate discussions.
| Verify(sender ActorID, msg, sig []byte) bool | ||
| Verify(pubKey PubKey, msg, sig []byte) bool | ||
| // Aggregates signatures from a participant to an existing signature. | ||
| Aggregate(sig [][]byte, aggSignature []byte) []byte |
There was a problem hiding this comment.
I see that you are assuming that aggregate signatures can be combined. I think the conclusion is that it is not true, but we can address that later.
|
Just rebased right now. |
Builds upon #32, which in turn builds upon #34, so merge those first.