-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate Warp Signatures #1818
Generate Warp Signatures #1818
Conversation
joshua-kim
commented
Nov 25, 2024
•
edited
Loading
edited
- Generate warp chunk signatures through acp 118
- Updates avalanchego dependency to use a patch version instead of master
message GetChunkSignatureRequest { | ||
bytes chunk = 1; | ||
} | ||
|
||
message GetChunkSignatureResponse { | ||
bytes signature = 1; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why these were added to begin with since we use acp118 signature requests
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this order be reversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I thought Execute
ran after Accept
... swapped the ordering here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <20001595+joshua-kim@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments and I think some of the test cases may be unreachable in production. I could be wrong, otherwise we can either keep them as defensive or remove.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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