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.

Sign up for GitHub

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

Pass the nodeID downstream of acp118 verifier #3606

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
+42 −25
Open

Pass the nodeID downstream of acp118 verifier #3606

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2ce94f4
update
tsachiherman Dec 17, 2024
39e29e3
update interface
tsachiherman Dec 17, 2024
030fd30
update
tsachiherman Dec 17, 2024
7694421
update
tsachiherman Dec 17, 2024
1668c78
Merge branch 'master' into tsachi/acp118-nodeid
tsachiherman Dec 17, 2024
643db30
reorder
tsachiherman Dec 18, 2024
259f773
update
tsachiherman Dec 18, 2024
172b466
Merge branch 'master' into tsachi/acp118-nodeid
tsachiherman Dec 18, 2024
ad80975
update unit test per feedback.
tsachiherman Dec 18, 2024
c3494e8
update
tsachiherman Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ require (
github.com/DataDog/zstd v1.5.2
github.com/NYTimes/gziphandler v1.1.1
github.com/antithesishq/antithesis-sdk-go v0.3.8
github.com/ava-labs/coreth v0.13.9-rc.2-encapsulate-signer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a corresponding PR to coreth for this change? Wondering what this diff is for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite minimal. I can't really create a final version until this one is merged since I'd need to tag it.. but these are the expected changes : ava-labs/coreth#710

github.com/ava-labs/coreth v0.13.9-rc.3-acp118-nodeid3
github.com/ava-labs/ledger-avalanche/go v0.0.0-20241009183145-e6f90a8a1a60
github.com/btcsuite/btcd/btcutil v1.1.3
github.com/cockroachdb/pebble v0.0.0-20230928194634-aa077af62593
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ github.com/antithesishq/antithesis-sdk-go v0.3.8/go.mod h1:IUpT2DPAKh6i/YhSbt6Gl
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
github.com/ava-labs/coreth v0.13.9-rc.2-encapsulate-signer h1:mRB03tLPUvgNko4nP4VwWQdiHeHaLHtdwsnqwxrsGec=
github.com/ava-labs/coreth v0.13.9-rc.2-encapsulate-signer/go.mod h1:tqRAe+7bGLo2Rq/Ph4iYMSch72ag/Jn0DiDMDz1Xa9E=
github.com/ava-labs/coreth v0.13.9-rc.3-acp118-nodeid3 h1:Ltk4OaR51hhY7dIBkRwyUz9JyMg1UedYwyZOeYQmbZ0=
github.com/ava-labs/coreth v0.13.9-rc.3-acp118-nodeid3/go.mod h1:tqRAe+7bGLo2Rq/Ph4iYMSch72ag/Jn0DiDMDz1Xa9E=
github.com/ava-labs/ledger-avalanche/go v0.0.0-20241009183145-e6f90a8a1a60 h1:EL66gtXOAwR/4KYBjOV03LTWgkEXvLePribLlJNu4g0=
github.com/ava-labs/ledger-avalanche/go v0.0.0-20241009183145-e6f90a8a1a60/go.mod h1:/7qKobTfbzBu7eSTVaXMTr56yTYk4j2Px6/8G+idxHo=
github.com/aymerick/raymond v2.0.3-0.20180322193309-b565731e1464+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g=
Expand Down
5 changes: 3 additions & 2 deletions network/p2p/acp118/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ var _ p2p.Handler = (*Handler)(nil)
type Verifier interface {
Verify(
ctx context.Context,
nodeID ids.NodeID,
message *warp.UnsignedMessage,
justification []byte,
) *common.AppError
Expand Down Expand Up @@ -65,7 +66,7 @@ type Handler struct {

func (h *Handler) AppRequest(
ctx context.Context,
_ ids.NodeID,
nodeID ids.NodeID,
_ time.Time,
requestBytes []byte,
) ([]byte, *common.AppError) {
Expand All @@ -90,7 +91,7 @@ func (h *Handler) AppRequest(
return signatureToResponse(signatureBytes)
}

if err := h.verifier.Verify(ctx, msg, request.Justification); err != nil {
if err := h.verifier.Verify(ctx, nodeID, msg, request.Justification); err != nil {
return nil, err
}

Expand Down
50 changes: 30 additions & 20 deletions network/p2p/acp118/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,23 @@
tests := []struct {
name string
cacher cache.Cacher[ids.ID, []byte]
verifier Verifier
verifierErrs []*common.AppError
expectedErrs []error
}{
{
name: "signature fails verification",
cacher: &cache.Empty[ids.ID, []byte]{},
verifier: &testVerifier{
Errs: []*common.AppError{
{Code: 123},
},
verifierErrs: []*common.AppError{
{Code: 123},
},
expectedErrs: []error{
&common.AppError{Code: 123},
},
},
{
name: "signature signed",
cacher: &cache.Empty[ids.ID, []byte]{},
verifier: &testVerifier{},
name: "signature signed",
cacher: &cache.Empty[ids.ID, []byte]{},
verifierErrs: nil,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be deleted entirely iirc because the zero value of a slice is safe to use

expectedErrs: []error{
nil,
},
Expand All @@ -54,11 +52,9 @@
cacher: &cache.LRU[ids.ID, []byte]{
Size: 1,
},
verifier: &testVerifier{
Errs: []*common.AppError{
nil,
{Code: 123}, // The valid response should be cached
},
verifierErrs: []*common.AppError{
nil,
{Code: 123}, // The valid response should be cached
},
expectedErrs: []error{
nil,
Expand All @@ -78,9 +74,17 @@
networkID := uint32(123)
chainID := ids.GenerateTestID()
signer := warp.NewSigner(sk, networkID, chainID)
h := NewCachedHandler(tt.cacher, tt.verifier, signer)
clientNodeID := ids.GenerateTestNodeID()
serverNodeID := ids.GenerateTestNodeID()
justification := []byte("justification")
testVerifier := &testVerifier{
T: t,
Errs: tt.verifierErrs,
ClientNodeID: clientNodeID,
ExpectedJustification: justification,
}
h := NewCachedHandler(tt.cacher, testVerifier, signer)

c := p2ptest.NewClient(
t,
ctx,
Expand All @@ -98,7 +102,7 @@

request := &sdk.SignatureRequest{
Message: unsignedMessage.Bytes(),
Justification: []byte("justification"),
Justification: justification,
}

requestBytes, err := proto.Marshal(request)
Expand All @@ -108,7 +112,7 @@
expectedErr error
handled = make(chan struct{})
)
onResponse := func(_ context.Context, _ ids.NodeID, responseBytes []byte, appErr error) {
onResponse := func(_ context.Context, nodeID ids.NodeID, responseBytes []byte, appErr error) {

Check failure on line 115 in network/p2p/acp118/handler_test.go

View workflow job for this annotation

GitHub Actions / Lint 

unused-parameter: parameter 'nodeID' seems to be unused, consider removing or renaming it as _ (revive)
defer func() {
handled <- struct{}{}
}()
Expand Down Expand Up @@ -143,14 +147,20 @@

// The zero value of testVerifier allows signing
type testVerifier struct {
Errs []*common.AppError
T *testing.T
Errs []*common.AppError
ClientNodeID ids.NodeID
ExpectedJustification []byte
}

func (t *testVerifier) Verify(
context.Context,
*warp.UnsignedMessage,
[]byte,
_ context.Context,
nodeID ids.NodeID,
_ *warp.UnsignedMessage,
justification []byte,
) *common.AppError {
require.Equal(t.T, t.ClientNodeID, nodeID)
require.Equal(t.T, t.ExpectedJustification, justification)
if len(t.Errs) == 0 {
return nil
}
Expand Down
3 changes: 2 additions & 1 deletion vms/example/xsvm/warp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package xsvm
import (
"context"

"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/network/p2p/acp118"
"github.com/ava-labs/avalanchego/snow/engine/common"
"github.com/ava-labs/avalanchego/vms/platformvm/warp"
Expand All @@ -16,6 +17,6 @@ var _ acp118.Verifier = (*acp118Verifier)(nil)
// acp118Verifier allows signing all warp messages
type acp118Verifier struct{}

func (acp118Verifier) Verify(context.Context, *warp.UnsignedMessage, []byte) *common.AppError {
func (acp118Verifier) Verify(context.Context, ids.NodeID, *warp.UnsignedMessage, []byte) *common.AppError {
return nil
}
1 change: 1 addition & 0 deletions vms/platformvm/network/warp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ type signatureRequestVerifier struct {

func (s signatureRequestVerifier) Verify(
_ context.Context,
_ ids.NodeID,
unsignedMessage *warp.UnsignedMessage,
justification []byte,
) *common.AppError {
Expand Down
5 changes: 5 additions & 0 deletions vms/platformvm/network/warp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func TestSignatureRequestVerify(t *testing.T) {
s := signatureRequestVerifier{}
err := s.Verify(
context.Background(),
ids.EmptyNodeID,
must[*warp.UnsignedMessage](t)(warp.NewUnsignedMessage(
constants.UnitTestID,
constants.PlatformChainID,
Expand Down Expand Up @@ -159,6 +160,7 @@ func TestSignatureRequestVerifySubnetToL1Conversion(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
err := s.Verify(
context.Background(),
ids.EmptyNodeID,
must[*warp.UnsignedMessage](t)(warp.NewUnsignedMessage(
constants.UnitTestID,
constants.PlatformChainID,
Expand Down Expand Up @@ -218,6 +220,7 @@ func TestSignatureRequestVerifyL1ValidatorRegistrationRegistered(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
err := s.Verify(
context.Background(),
ids.EmptyNodeID,
must[*warp.UnsignedMessage](t)(warp.NewUnsignedMessage(
constants.UnitTestID,
constants.PlatformChainID,
Expand Down Expand Up @@ -519,6 +522,7 @@ func TestSignatureRequestVerifyL1ValidatorRegistrationNotRegistered(t *testing.T
t.Run(test.name, func(t *testing.T) {
err := s.Verify(
context.Background(),
ids.EmptyNodeID,
must[*warp.UnsignedMessage](t)(warp.NewUnsignedMessage(
constants.UnitTestID,
constants.PlatformChainID,
Expand Down Expand Up @@ -620,6 +624,7 @@ func TestSignatureRequestVerifyL1ValidatorWeight(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
err := s.Verify(
context.Background(),
ids.EmptyNodeID,
must[*warp.UnsignedMessage](t)(warp.NewUnsignedMessage(
constants.UnitTestID,
constants.PlatformChainID,
Expand Down
Loading