Generate Warp Signatures #1818
Generate Warp Signatures #1818
Conversation
joshua-kim
commented
Nov 25, 2024
•
joshua-kim
commented
- Generate warp chunk signatures through acp 118
- Updates avalanchego dependency to use a patch version instead of master
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
| message GetChunkSignatureRequest { | ||
| bytes chunk = 1; | ||
| } | ||
|
|
||
| message GetChunkSignatureResponse { | ||
| bytes signature = 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't know why these were added to begin with since we use acp118 signature requests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
x/dsmr/node_test.go
Outdated
| @@ -1478,6 +1668,120 @@ func getSignerBitSet(t *testing.T, pChain validators.State, nodeIDs ...ids.NodeI | |||
| return signerBitSet | |||
| } | |||
|
|
|||
| func Test_Execute(t *testing.T) { | |||
There was a problem hiding this comment.
Looks like we weren't previously testing Execute. In hindsight though - I'm not sure how we want to test failure cases for this because I don't think we should be failing Execute if Accept doesn't return an error (and we shouldn't be calling Execute on a block we haven't accepted).
There was a problem hiding this comment.
Execute should fail if a an invalid signature, expiry, metadata (parent hash, height, timestamp), or duplicate is included.
Accept should execute the internal block, which should be treated as a fatal error, so we can only really test that it correctly propagates a fatal error.
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
x/dsmr/node.go
Outdated
| } | ||
|
|
||
| blk.blkBytes = packer.Bytes | ||
| blk.blkID = utils.ToID(blk.blkBytes) | ||
| return blk, nil | ||
| } | ||
|
|
||
| func (*Node[T]) Execute(ctx context.Context, _ Block, block Block) error { | ||
| func (n *Node[T]) Execute(ctx context.Context, block Block[T]) error { |
There was a problem hiding this comment.
(optional for this PR) - we can remove the unused parent parameter or check the header fields ie. height, parent hash, timestamp, etc.
There was a problem hiding this comment.
I can re-add this parameter for now to not scope creep
x/dsmr/storage_test.go
Outdated
| @@ -121,10 +122,10 @@ func TestStoreAndSaveValidChunk(t *testing.T) { | |||
| chunkCerts := storage.GatherChunkCerts() | |||
| require.Empty(chunkCerts) | |||
|
|
|||
| chunkCert := &ChunkCertificate{ | |||
| chunkCert := &ChunkCertificate[tx]{ | |||
There was a problem hiding this comment.
Should we modify these tests to account for when the signature should be present or would you prefer that's tested through the node tests?
There was a problem hiding this comment.
We're now exporting the ChunkStorage type outside of this package, but i don't think there's a use-case where people should be manually playing with the internals of the database. I think I would prefer that we test this through the Node api
x/dsmr/storage.go
Outdated
| s.lock.RLock() | ||
| defer s.lock.RUnlock() | ||
|
|
||
| chunkCerts := make([]*ChunkCertificate, 0, len(s.chunkMap)) | ||
| chunkCerts := make([]*ChunkCertificate[T], 0, len(s.chunkMap)) |
There was a problem hiding this comment.
(unrelated to this PR) Can we remove the below nil check if the cert will always be populated when it's in the map?
There was a problem hiding this comment.
Yeah I'm not sure why this check exists here either. It looks like this should be safe to remove...
x/dsmr/node_test.go
Outdated
| r.NoError(node2.Accept(context.Background(), blk)) | ||
| r.NoError(node2.Execute(context.Background(), blk)) |
There was a problem hiding this comment.
Should this order be reversed?
There was a problem hiding this comment.
For some reason I thought Execute ran after Accept... swapped the ordering here.
There was a problem hiding this comment.
You're right, it's confusing haha. There is the DSMR block and then the internal block.
This should support the same flow as the chain/ package is used for in the snowman.Block implementation. The current flow is:
- Execute the block executes the state transition (Verify)
- Accept marks the already executed block as accepted (Accept)
In DSMR, we have the added requirement that we assemble and execute an internal block. So the flow is:
- Execute the DSMR block (Verify)
- Accept the DSMR block (this assembles and executes the internal block and also marks it as accepted)
x/dsmr/node_test.go
Outdated
| @@ -1478,6 +1668,120 @@ func getSignerBitSet(t *testing.T, pChain validators.State, nodeIDs ...ids.NodeI | |||
| return signerBitSet | |||
| } | |||
|
|
|||
| func Test_Execute(t *testing.T) { | |||
There was a problem hiding this comment.
Execute should fail if a an invalid signature, expiry, metadata (parent hash, height, timestamp), or duplicate is included.
Accept should execute the internal block, which should be treated as a fatal error, so we can only really test that it correctly propagates a fatal error.
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
| result := make([]*Node[tx], 0, n) | ||
| for i, n := range nodes { | ||
| getChunkPeers := make(map[ids.NodeID]p2p.Handler) | ||
| chunkSignaturePeers := make(map[ids.NodeID]p2p.Handler) | ||
| chunkCertGossipPeers := make(map[ids.NodeID]p2p.Handler) | ||
| for j := range nodes { | ||
| if i == j { | ||
| continue | ||
| } | ||
|
|
||
| getChunkPeers[validators[j].NodeID] = nodes[j].GetChunkHandler |
There was a problem hiding this comment.
This setup is really helpful, separate from this PR, I wonder if we could set up a standard way to do this for anything that is using the p2p networking code and needs to connect clients/servers this way
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
