Skip to content
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

Merged
merged 37 commits into from
Dec 3, 2024
Merged

Generate Warp Signatures #1818

merged 37 commits into from
Dec 3, 2024

Conversation

joshua-kim
Copy link
Contributor

@joshua-kim joshua-kim commented Nov 25, 2024

  • Generate warp chunk signatures through acp 118
  • Updates avalanchego dependency to use a patch version instead of master

Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Comment on lines -19 to -26
message GetChunkSignatureRequest {
bytes chunk = 1;
}

message GetChunkSignatureResponse {
bytes signature = 1;
}

Copy link
Contributor Author

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.go Fixed Show fixed Hide fixed
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
x/dsmr/certificate.go Outdated Show resolved Hide resolved
x/dsmr/storage.go Show resolved Hide resolved
@@ -1478,6 +1668,120 @@ func getSignerBitSet(t *testing.T, pChain validators.State, nodeIDs ...ids.NodeI
return signerBitSet
}

func Test_Execute(t *testing.T) {
Copy link
Contributor Author

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).

Copy link
Collaborator

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>
x/dsmr/certificate.go Fixed Show fixed Hide fixed
@joshua-kim joshua-kim marked this pull request as ready for review November 26, 2024 17:02
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
x/dsmr/certificate.go Outdated Show resolved Hide resolved
x/dsmr/certificate.go Outdated Show resolved Hide resolved
x/dsmr/node.go Show resolved Hide resolved
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 {
Copy link
Collaborator

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.

Copy link
Contributor Author

@joshua-kim joshua-kim Nov 27, 2024

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.go Show resolved Hide resolved
x/dsmr/storage.go Show resolved Hide resolved
@@ -121,10 +122,10 @@ func TestStoreAndSaveValidChunk(t *testing.T) {
chunkCerts := storage.GatherChunkCerts()
require.Empty(chunkCerts)

chunkCert := &ChunkCertificate{
chunkCert := &ChunkCertificate[tx]{
Copy link
Collaborator

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?

Copy link
Contributor Author

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

s.lock.RLock()
defer s.lock.RUnlock()

chunkCerts := make([]*ChunkCertificate, 0, len(s.chunkMap))
chunkCerts := make([]*ChunkCertificate[T], 0, len(s.chunkMap))
Copy link
Collaborator

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?

Copy link
Contributor Author

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...

Comment on lines 1782 to 1783
r.NoError(node2.Accept(context.Background(), blk))
r.NoError(node2.Execute(context.Background(), blk))
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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:

  1. Execute the block executes the state transition (Verify)
  2. 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:

  1. Execute the DSMR block (Verify)
  2. Accept the DSMR block (this assembles and executes the internal block and also marks it as accepted)

@@ -1478,6 +1668,120 @@ func getSignerBitSet(t *testing.T, pChain validators.State, nodeIDs ...ids.NodeI
return signerBitSet
}

func Test_Execute(t *testing.T) {
Copy link
Collaborator

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 Show resolved Hide resolved
x/dsmr/node.go Outdated Show resolved Hide resolved
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
x/dsmr/certificate.go Outdated Show resolved Hide resolved
x/dsmr/storage.go Show resolved Hide resolved
x/dsmr/storage.go Show resolved Hide resolved
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
@joshua-kim joshua-kim self-assigned this Dec 2, 2024
aaronbuchwald
aaronbuchwald previously approved these changes Dec 2, 2024
Copy link
Collaborator

@aaronbuchwald aaronbuchwald left a 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.

x/dsmr/node.go Outdated Show resolved Hide resolved
x/dsmr/node.go Outdated Show resolved Hide resolved
x/dsmr/certificate.go Show resolved Hide resolved
Comment on lines +1367 to +1377
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
Copy link
Collaborator

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

x/dsmr/node_test.go Show resolved Hide resolved
x/dsmr/node_test.go Outdated Show resolved Hide resolved
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
x/dsmr/node.go Outdated Show resolved Hide resolved
x/dsmr/node.go Outdated Show resolved Hide resolved
x/dsmr/node_test.go Outdated Show resolved Hide resolved
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
@aaronbuchwald aaronbuchwald merged commit 7c1f744 into main Dec 3, 2024
17 checks passed
@aaronbuchwald aaronbuchwald deleted the acp118 branch December 3, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants