POC Signature aggregation API #368
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
I don't think so. For messages from P-chain -> Subnet, the |
…-api resolved conflicts to make the code compile
Co-authored-by: Michael Kaplan <55204436+michaelkaplan13@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
before this, it was being rendered as: INFO [07-29|19:29:09.151] Starting the signature aggregator with config at :%s /home/gene/tmp/signature-aggregator-config.json61842304=<nil> LOG_ERROR="Normalized odd number of arguments by adding nil"
"Starting the signature-aggregator executable" message already exists elsewhere. This differentiates this log entry from that one.
config/config.go
Outdated
| return c.PChainAPI | ||
| } | ||
|
|
||
| // Config implempents the peers.Config interface |
There was a problem hiding this comment.
makes sense and is a nit, but can we still add it? We have it in other places of the repo, and believe it's a standard go practice
| &cfg, | ||
| ) | ||
| network.InitializeConnectionsAndCheckStake(&cfg) |
There was a problem hiding this comment.
can we remove the pass by ref here, want to avoid it being unintentionally altered
signature-aggregator/api/api.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| RawMessageAPIPath = "/aggregate-signatures/by-raw-message" |
There was a problem hiding this comment.
good with signatures/aggregate
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Co-authored-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com> Co-authored-by: minghinmatthewlam <matthew.lam@avalabs.org> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
| type AggregateSignatureResponse struct { | ||
| // hex encoding of the signature | ||
| SignedMessage string `json:"signed-message"` | ||
| } |
There was a problem hiding this comment.
We should consider returning the P-Chain height corresponding to the signature's canonical validator bit vector. Without it, there's an implicit "expiration" on responses from the API. With it, the caller would be better able to implement checks against the current state of the P-Chain before sending a tx to deliver the Warp message.
There was a problem hiding this comment.
I'm happy to add this to follow up. Since we are already changing interfaces there for ACP-118. But I do think that the implicit P-chain height is current height and the general workflow is request it and send it immediately. Failure in that case is extremely unlikely but a simple retry of the full flow should be an easy fix.
signature-aggregator/README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| ## Sample workflow | ||
| If you want to manually test a locally running service pointed to the Fuji testnet you can do so with the following steps. | ||
|
|
||
| Note that this might fail for older messages if there has been enough validator churn, and less then threhold weight of stake of validators have seen the message when it originated. In this case try picking a more recent message. |
There was a problem hiding this comment.
| Note that this might fail for older messages if there has been enough validator churn, and less then threhold weight of stake of validators have seen the message when it originated. In this case try picking a more recent message. | |
| Note that this might fail for older messages if there has been enough validator churn, and less then the threshold weight of stake of validators have seen the message when it originated. In this case try picking a more recent message. |
There was a problem hiding this comment.
Expanding this to 1) explain that the Warp protocol does not support resigning, but 2) protocols on top of Warp such as Teleporter do (and link to the relevant docs)
There was a problem hiding this comment.
I just left a comment on your PR but I will link to the doc section once they are merged in main. I don't really want to over-elaborate here beyond linking since this is just a sample testing flow to make sure that the service works as expected.
Co-authored-by: cam-schultz <78878559+cam-schultz@users.noreply.github.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
| return signedMsg, true, nil | ||
| } | ||
| // Not enough signatures, continue processing messages | ||
| return nil, true, nil |
There was a problem hiding this comment.
Yeah, I think I prefer explicitly returning nil
Co-authored-by: Geoff Stuart <geoff.vball@gmail.com> Signed-off-by: Ian Suvak <ian.suvak@avalabs.org>
iansuvak
force-pushed
the
signature-aggregation-api
branch
from
d8653a6 to
1477133
Compare
Why this should be merged
This will close #361 and represents a minimal mergeable implementation of the Warp Signature aggregation API. It maintains all functionality of the existing relayer implementation and adds the new binary running the signature aggregation portion.
How this works
application_relayer.goto it's own file insignature-aggregator/aggregator.gowhich is imported by the relayer.peerspackage includingAppRequestNetworkbut slightly modifies it to be able to accept the simplified config from signature-aggregatorCode organization
signature-aggregatordirectory containing all files specific to the new API including it's config and network implementation.libfolder containing some common interfaces and structs used by both configs and networks. Some of the network code in particular should be deduplicated.awm-relayernetwork implementation to be able to share interfaces for signature aggregation in the future.How this was tested
Existing e2e tests pass and a new e2e test was added. Note that currently it will only work for messages originating on the P-Chain until avalanche go 1.11.11 (and matching coreth/subnet-evm) is released as a required networking fix to bypass trackesSubnets check has only recently been merged to
masterbranch.How is this documented
README.md with overview and example manual testing flow was added.