From 7c6d0a70c6a66838e837b517202a49fd4c4e9256 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 11 Jun 2024 13:43:14 -0400 Subject: [PATCH 01/62] implement acp-118 signature aggregation Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 229 ++++++++++++++++++++++++++ network/p2p/acp118/aggregator_test.go | 205 +++++++++++++++++++++++ network/p2p/error.go | 33 ---- 3 files changed, 434 insertions(+), 33 deletions(-) create mode 100644 network/p2p/acp118/aggregator.go create mode 100644 network/p2p/acp118/aggregator_test.go delete mode 100644 network/p2p/error.go diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go new file mode 100644 index 000000000000..9cbc10a9442c --- /dev/null +++ b/network/p2p/acp118/aggregator.go @@ -0,0 +1,229 @@ +// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package acp118 + +import ( + "context" + "errors" + "fmt" + "sync" + + "go.uber.org/zap" + "golang.org/x/sync/semaphore" + "google.golang.org/protobuf/proto" + + "github.com/ava-labs/avalanchego/ids" + "github.com/ava-labs/avalanchego/network/p2p" + "github.com/ava-labs/avalanchego/proto/pb/sdk" + "github.com/ava-labs/avalanchego/utils/crypto/bls" + "github.com/ava-labs/avalanchego/utils/logging" + "github.com/ava-labs/avalanchego/utils/set" + "github.com/ava-labs/avalanchego/vms/platformvm/warp" +) + +var ( + ErrDuplicateValidator = errors.New("duplicate validator") + ErrInsufficientSignatures = errors.New("failed to aggregate sufficient stake weight of signatures") +) + +type result struct { + message *warp.Message + err error +} + +type Validator struct { + NodeID ids.NodeID + PublicKey *bls.PublicKey + Weight uint64 +} + +type indexedValidator struct { + Validator + I int +} + +// NewSignatureAggregator returns an instance of SignatureAggregator +func NewSignatureAggregator( + log logging.Logger, + client *p2p.Client, + maxPending int, +) *SignatureAggregator { + return &SignatureAggregator{ + log: log, + client: client, + maxPending: int64(maxPending), + } +} + +// SignatureAggregator aggregates validator signatures for warp messages +type SignatureAggregator struct { + log logging.Logger + client *p2p.Client + maxPending int64 +} + +// AggregateSignatures blocks until stakeWeightThreshold of validators signs the +// provided message. Validators are issued requests in the caller-specified +// order. +func (s *SignatureAggregator) AggregateSignatures( + parentCtx context.Context, + message *warp.UnsignedMessage, + justification []byte, + validators []Validator, + stakeWeightThreshold uint64, +) (*warp.Message, error) { + ctx, cancel := context.WithCancel(parentCtx) + defer cancel() + + request := &sdk.SignatureRequest{ + Message: message.Bytes(), + Justification: justification, + } + + requestBytes, err := proto.Marshal(request) + if err != nil { + return nil, fmt.Errorf("failed to marshal signature request: %w", err) + } + + done := make(chan result) + pendingRequests := semaphore.NewWeighted(s.maxPending) + lock := &sync.Mutex{} + aggregatedStakeWeight := uint64(0) + attemptedStakeWeight := uint64(0) + totalStakeWeight := uint64(0) + signatures := make([]*bls.Signature, 0) + signerBitSet := set.NewBits() + + nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) + for i, v := range validators { + totalStakeWeight += v.Weight + + // Sanity check the validator set provided by the caller + if _, ok := nodeIDsToValidator[v.NodeID]; ok { + return nil, fmt.Errorf("%w: %s", ErrDuplicateValidator, v.NodeID) + } + + nodeIDsToValidator[v.NodeID] = indexedValidator{ + I: i, + Validator: v, + } + } + + onResponse := func( + _ context.Context, + nodeID ids.NodeID, + responseBytes []byte, + err error, + ) { + // We are guaranteed a response from a node in the validator set + validator := nodeIDsToValidator[nodeID] + + defer func() { + lock.Lock() + attemptedStakeWeight += validator.Weight + remainingStakeWeight := totalStakeWeight - attemptedStakeWeight + failed := remainingStakeWeight < stakeWeightThreshold + lock.Unlock() + + if failed { + done <- result{err: ErrInsufficientSignatures} + } + + pendingRequests.Release(1) + }() + + if err != nil { + s.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.Error(err), + ) + return + } + + response := &sdk.SignatureResponse{} + if err := proto.Unmarshal(responseBytes, response); err != nil { + s.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.Error(err), + ) + return + } + + signature, err := bls.SignatureFromBytes(response.Signature) + if err != nil { + s.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.String("reason", "invalid signature"), + zap.Error(err), + ) + return + } + + if !bls.Verify(validator.PublicKey, signature, message.Bytes()) { + s.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.String("reason", "public key failed verification"), + ) + return + } + + lock.Lock() + signerBitSet.Add(validator.I) + signatures = append(signatures, signature) + aggregatedStakeWeight += validator.Weight + + if aggregatedStakeWeight >= stakeWeightThreshold { + aggregateSignature, err := bls.AggregateSignatures(signatures) + if err != nil { + done <- result{err: err} + lock.Unlock() + return + } + + bitSetSignature := &warp.BitSetSignature{ + Signers: signerBitSet.Bytes(), + Signature: [bls.SignatureLen]byte{}, + } + + copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) + signedMessage, err := warp.NewMessage(message, bitSetSignature) + done <- result{message: signedMessage, err: err} + lock.Unlock() + return + } + + lock.Unlock() + } + + for _, validator := range validators { + if err := pendingRequests.Acquire(ctx, 1); err != nil { + return nil, err + } + + // Avoid loop shadowing in goroutine + validatorCopy := validator + go func() { + if err := s.client.AppRequest( + ctx, + set.Of(validatorCopy.NodeID), + requestBytes, + onResponse, + ); err != nil { + done <- result{err: err} + return + } + }() + } + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case r := <-done: + return r.message, r.err + } +} diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go new file mode 100644 index 000000000000..50622fc4ac99 --- /dev/null +++ b/network/p2p/acp118/aggregator_test.go @@ -0,0 +1,205 @@ +// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package acp118 + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ava-labs/avalanchego/ids" + "github.com/ava-labs/avalanchego/network/p2p" + "github.com/ava-labs/avalanchego/network/p2p/p2ptest" + "github.com/ava-labs/avalanchego/snow/validators" + "github.com/ava-labs/avalanchego/snow/validators/validatorstest" + "github.com/ava-labs/avalanchego/utils/crypto/bls" + "github.com/ava-labs/avalanchego/utils/logging" + "github.com/ava-labs/avalanchego/vms/platformvm/warp" +) + +func TestVerifier_Verify(t *testing.T) { + nodeID0 := ids.GenerateTestNodeID() + sk0, err := bls.NewSecretKey() + require.NoError(t, err) + pk0 := bls.PublicFromSecretKey(sk0) + + nodeID1 := ids.GenerateTestNodeID() + sk1, err := bls.NewSecretKey() + require.NoError(t, err) + pk1 := bls.PublicFromSecretKey(sk1) + + networkID := uint32(123) + subnetID := ids.GenerateTestID() + chainID := ids.GenerateTestID() + signer := warp.NewSigner(sk0, networkID, chainID) + + tests := []struct { + name string + + handler p2p.Handler + + ctx context.Context + validators []Validator + threshold uint64 + + pChainState validators.State + pChainHeight uint64 + quorumNum uint64 + quorumDen uint64 + + wantAggregateSignaturesErr error + wantVerifyErr error + }{ + { + name: "passes attestation and verification", + handler: NewHandler(&testAttestor{}, signer, networkID, chainID), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + threshold: 1, + pChainState: &validatorstest.State{ + T: t, + GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { + return subnetID, nil + }, + GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { + return map[ids.NodeID]*validators.GetValidatorOutput{ + nodeID0: { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, nil + }, + }, + quorumNum: 1, + quorumDen: 1, + }, + { + name: "passes attestation and fails verification - insufficient stake", + handler: NewHandler(&testAttestor{}, signer, networkID, chainID), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + threshold: 1, + pChainState: &validatorstest.State{ + T: t, + GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { + return subnetID, nil + }, + GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { + return map[ids.NodeID]*validators.GetValidatorOutput{ + nodeID0: { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + nodeID1: { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 1, + }, + }, nil + }, + }, + quorumNum: 2, + quorumDen: 2, + wantVerifyErr: warp.ErrInsufficientWeight, + }, + { + name: "fails attestation", + handler: NewHandler( + &testAttestor{Err: errors.New("foobar")}, + signer, + networkID, + chainID, + ), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + threshold: 1, + wantAggregateSignaturesErr: ErrInsufficientSignatures, + }, + { + name: "invalid validator set", + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + wantAggregateSignaturesErr: ErrDuplicateValidator, + }, + { + name: "context canceled", + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + return ctx + }(), + wantAggregateSignaturesErr: context.Canceled, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + + ctx := context.Background() + message, err := warp.NewUnsignedMessage(networkID, chainID, []byte("payload")) + require.NoError(err) + client := p2ptest.NewClient(t, ctx, tt.handler, ids.GenerateTestNodeID(), nodeID0) + verifier := NewSignatureAggregator(logging.NoLog{}, client, 1) + + signedMessage, err := verifier.AggregateSignatures( + tt.ctx, + message, + []byte("justification"), + tt.validators, + tt.threshold, + ) + require.ErrorIs(err, tt.wantAggregateSignaturesErr) + + if signedMessage == nil { + return + } + + err = signedMessage.Signature.Verify( + ctx, + &signedMessage.UnsignedMessage, + networkID, + tt.pChainState, + 0, + tt.quorumNum, + tt.quorumDen, + ) + require.ErrorIs(err, tt.wantVerifyErr) + }) + } +} diff --git a/network/p2p/error.go b/network/p2p/error.go deleted file mode 100644 index 07207319a041..000000000000 --- a/network/p2p/error.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. -// See the file LICENSE for licensing terms. - -package p2p - -import "github.com/ava-labs/avalanchego/snow/engine/common" - -var ( - // ErrUnexpected should be used to indicate that a request failed due to a - // generic error - ErrUnexpected = &common.AppError{ - Code: -1, - Message: "unexpected error", - } - // ErrUnregisteredHandler should be used to indicate that a request failed - // due to it not matching a registered handler - ErrUnregisteredHandler = &common.AppError{ - Code: -2, - Message: "unregistered handler", - } - // ErrNotValidator should be used to indicate that a request failed due to - // the requesting peer not being a validator - ErrNotValidator = &common.AppError{ - Code: -3, - Message: "not a validator", - } - // ErrThrottled should be used to indicate that a request failed due to the - // requesting peer exceeding a rate limit - ErrThrottled = &common.AppError{ - Code: -4, - Message: "throttled", - } -) From 11ada0f373df7662098dc89a3654050e045c124a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 22 Oct 2024 15:23:24 -0400 Subject: [PATCH 02/62] undo Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/error.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 network/p2p/error.go diff --git a/network/p2p/error.go b/network/p2p/error.go new file mode 100644 index 000000000000..07207319a041 --- /dev/null +++ b/network/p2p/error.go @@ -0,0 +1,33 @@ +// Copyright (C) 2019-2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package p2p + +import "github.com/ava-labs/avalanchego/snow/engine/common" + +var ( + // ErrUnexpected should be used to indicate that a request failed due to a + // generic error + ErrUnexpected = &common.AppError{ + Code: -1, + Message: "unexpected error", + } + // ErrUnregisteredHandler should be used to indicate that a request failed + // due to it not matching a registered handler + ErrUnregisteredHandler = &common.AppError{ + Code: -2, + Message: "unregistered handler", + } + // ErrNotValidator should be used to indicate that a request failed due to + // the requesting peer not being a validator + ErrNotValidator = &common.AppError{ + Code: -3, + Message: "not a validator", + } + // ErrThrottled should be used to indicate that a request failed due to the + // requesting peer exceeding a rate limit + ErrThrottled = &common.AppError{ + Code: -4, + Message: "throttled", + } +) From e650f5d0caf2be6cb51b3949be8b53815b5be2c1 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 22 Oct 2024 16:14:13 -0400 Subject: [PATCH 03/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 4 +++- network/p2p/acp118/aggregator_test.go | 14 ++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 9cbc10a9442c..20999f225578 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -23,7 +23,9 @@ import ( ) var ( - ErrDuplicateValidator = errors.New("duplicate validator") + ErrDuplicateValidator = errors.New("duplicate validator") + // ErrInsufficientSignatures is returned if it's not possible for us to + // generate a signature due to too many unsuccessful requests to peers ErrInsufficientSignatures = errors.New("failed to aggregate sufficient stake weight of signatures") ) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 50622fc4ac99..13d40ce9297f 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -5,7 +5,6 @@ package acp118 import ( "context" - "errors" "testing" "github.com/stretchr/testify/require" @@ -13,6 +12,7 @@ import ( "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/network/p2p" "github.com/ava-labs/avalanchego/network/p2p/p2ptest" + "github.com/ava-labs/avalanchego/snow/engine/common" "github.com/ava-labs/avalanchego/snow/validators" "github.com/ava-labs/avalanchego/snow/validators/validatorstest" "github.com/ava-labs/avalanchego/utils/crypto/bls" @@ -54,8 +54,8 @@ func TestVerifier_Verify(t *testing.T) { wantVerifyErr error }{ { - name: "passes attestation and verification", - handler: NewHandler(&testAttestor{}, signer, networkID, chainID), + name: "pass - gets signatures from sufficient stake", + handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ { @@ -84,8 +84,8 @@ func TestVerifier_Verify(t *testing.T) { quorumDen: 1, }, { - name: "passes attestation and fails verification - insufficient stake", - handler: NewHandler(&testAttestor{}, signer, networkID, chainID), + name: "fail - gets signatures from insufficient stake", + handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ { @@ -122,10 +122,8 @@ func TestVerifier_Verify(t *testing.T) { { name: "fails attestation", handler: NewHandler( - &testAttestor{Err: errors.New("foobar")}, + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer, - networkID, - chainID, ), ctx: context.Background(), validators: []Validator{ From 81c9487ebb87ba0898164e1b1d89f83d1404cba7 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:19:38 -0500 Subject: [PATCH 04/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 20999f225578..b0820a701eea 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -23,6 +23,8 @@ import ( ) var ( + // ErrDuplicateValidator is returned if the provided validator set contains + // duplicate validators ErrDuplicateValidator = errors.New("duplicate validator") // ErrInsufficientSignatures is returned if it's not possible for us to // generate a signature due to too many unsuccessful requests to peers From 0760fcdd45b02abcd06785333dfd47c093f9f8fb Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:39:19 -0500 Subject: [PATCH 05/62] improve usage of context Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 5 +---- network/p2p/acp118/aggregator_test.go | 11 +++++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index b0820a701eea..02bdb28d0a9e 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -71,15 +71,12 @@ type SignatureAggregator struct { // provided message. Validators are issued requests in the caller-specified // order. func (s *SignatureAggregator) AggregateSignatures( - parentCtx context.Context, + ctx context.Context, message *warp.UnsignedMessage, justification []byte, validators []Validator, stakeWeightThreshold uint64, ) (*warp.Message, error) { - ctx, cancel := context.WithCancel(parentCtx) - defer cancel() - request := &sdk.SignatureRequest{ Message: message.Bytes(), Justification: justification, diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 13d40ce9297f..ef5404cf1887 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -54,7 +54,7 @@ func TestVerifier_Verify(t *testing.T) { wantVerifyErr error }{ { - name: "pass - gets signatures from sufficient stake", + name: "gets signatures from sufficient stake", handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ @@ -84,7 +84,7 @@ func TestVerifier_Verify(t *testing.T) { quorumDen: 1, }, { - name: "fail - gets signatures from insufficient stake", + name: "gets signatures from insufficient stake", handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ @@ -169,10 +169,9 @@ func TestVerifier_Verify(t *testing.T) { t.Run(tt.name, func(t *testing.T) { require := require.New(t) - ctx := context.Background() message, err := warp.NewUnsignedMessage(networkID, chainID, []byte("payload")) require.NoError(err) - client := p2ptest.NewClient(t, ctx, tt.handler, ids.GenerateTestNodeID(), nodeID0) + client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) verifier := NewSignatureAggregator(logging.NoLog{}, client, 1) signedMessage, err := verifier.AggregateSignatures( @@ -184,12 +183,12 @@ func TestVerifier_Verify(t *testing.T) { ) require.ErrorIs(err, tt.wantAggregateSignaturesErr) - if signedMessage == nil { + if tt.wantAggregateSignaturesErr != nil { return } err = signedMessage.Signature.Verify( - ctx, + context.Background(), &signedMessage.UnsignedMessage, networkID, tt.pChainState, From b6cfd0edfce081e248e861157cf73a7b2d22f9d1 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:45:51 -0500 Subject: [PATCH 06/62] improve doc Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 02bdb28d0a9e..c69b73452ff3 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -68,8 +68,7 @@ type SignatureAggregator struct { } // AggregateSignatures blocks until stakeWeightThreshold of validators signs the -// provided message. Validators are issued requests in the caller-specified -// order. +// provided message. func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.UnsignedMessage, From dacd131f9031ada88f6c54efc1d79647540e03ea Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 12 Nov 2024 16:47:28 -0500 Subject: [PATCH 07/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index ef5404cf1887..d622119d423f 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -32,7 +32,6 @@ func TestVerifier_Verify(t *testing.T) { pk1 := bls.PublicFromSecretKey(sk1) networkID := uint32(123) - subnetID := ids.GenerateTestID() chainID := ids.GenerateTestID() signer := warp.NewSigner(sk0, networkID, chainID) @@ -68,7 +67,7 @@ func TestVerifier_Verify(t *testing.T) { pChainState: &validatorstest.State{ T: t, GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { - return subnetID, nil + return ids.Empty, nil }, GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { return map[ids.NodeID]*validators.GetValidatorOutput{ @@ -98,7 +97,7 @@ func TestVerifier_Verify(t *testing.T) { pChainState: &validatorstest.State{ T: t, GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { - return subnetID, nil + return ids.Empty, nil }, GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { return map[ids.NodeID]*validators.GetValidatorOutput{ From 810fb559154c7b3e205f73af665f55a9c7d3facc Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 13 Nov 2024 17:16:12 -0500 Subject: [PATCH 08/62] rename i -> index Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index c69b73452ff3..e3d45da59713 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -44,7 +44,7 @@ type Validator struct { type indexedValidator struct { Validator - I int + Index int } // NewSignatureAggregator returns an instance of SignatureAggregator @@ -105,7 +105,7 @@ func (s *SignatureAggregator) AggregateSignatures( } nodeIDsToValidator[v.NodeID] = indexedValidator{ - I: i, + Index: i, Validator: v, } } From 6f6c0b54cb094743a49ac32c618fc92b279c0244 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 14 Nov 2024 03:48:28 -0500 Subject: [PATCH 09/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 76 +++++++++++---------------- network/p2p/acp118/aggregator_test.go | 48 +++++++++++++---- 2 files changed, 70 insertions(+), 54 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index e3d45da59713..c75a8bd57cbe 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -9,6 +9,7 @@ import ( "fmt" "sync" + "github.com/ava-labs/avalanchego/utils/math" "go.uber.org/zap" "golang.org/x/sync/semaphore" "google.golang.org/protobuf/proto" @@ -26,16 +27,11 @@ var ( // ErrDuplicateValidator is returned if the provided validator set contains // duplicate validators ErrDuplicateValidator = errors.New("duplicate validator") - // ErrInsufficientSignatures is returned if it's not possible for us to + // ErrFailedAggregation is returned if it's not possible for us to // generate a signature due to too many unsuccessful requests to peers - ErrInsufficientSignatures = errors.New("failed to aggregate sufficient stake weight of signatures") + ErrFailedAggregation = errors.New("failed to aggregate signatures") ) -type result struct { - message *warp.Message - err error -} - type Validator struct { NodeID ids.NodeID PublicKey *bls.PublicKey @@ -67,14 +63,12 @@ type SignatureAggregator struct { maxPending int64 } -// AggregateSignatures blocks until stakeWeightThreshold of validators signs the -// provided message. +// AggregateSignatures blocks until signatures from validators are aggregated. func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.UnsignedMessage, justification []byte, validators []Validator, - stakeWeightThreshold uint64, ) (*warp.Message, error) { request := &sdk.SignatureRequest{ Message: message.Bytes(), @@ -86,7 +80,7 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, fmt.Errorf("failed to marshal signature request: %w", err) } - done := make(chan result) + done := make(chan error) pendingRequests := semaphore.NewWeighted(s.maxPending) lock := &sync.Mutex{} aggregatedStakeWeight := uint64(0) @@ -97,7 +91,10 @@ func (s *SignatureAggregator) AggregateSignatures( nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) for i, v := range validators { - totalStakeWeight += v.Weight + totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) + if err != nil { + return nil, err + } // Sanity check the validator set provided by the caller if _, ok := nodeIDsToValidator[v.NodeID]; ok { @@ -116,18 +113,17 @@ func (s *SignatureAggregator) AggregateSignatures( responseBytes []byte, err error, ) { + lock.Lock() + // We are guaranteed a response from a node in the validator set validator := nodeIDsToValidator[nodeID] defer func() { - lock.Lock() attemptedStakeWeight += validator.Weight - remainingStakeWeight := totalStakeWeight - attemptedStakeWeight - failed := remainingStakeWeight < stakeWeightThreshold lock.Unlock() - if failed { - done <- result{err: ErrInsufficientSignatures} + if attemptedStakeWeight == totalStakeWeight { + done <- nil } pendingRequests.Release(1) @@ -172,32 +168,9 @@ func (s *SignatureAggregator) AggregateSignatures( return } - lock.Lock() - signerBitSet.Add(validator.I) + signerBitSet.Add(validator.Index) signatures = append(signatures, signature) aggregatedStakeWeight += validator.Weight - - if aggregatedStakeWeight >= stakeWeightThreshold { - aggregateSignature, err := bls.AggregateSignatures(signatures) - if err != nil { - done <- result{err: err} - lock.Unlock() - return - } - - bitSetSignature := &warp.BitSetSignature{ - Signers: signerBitSet.Bytes(), - Signature: [bls.SignatureLen]byte{}, - } - - copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) - signedMessage, err := warp.NewMessage(message, bitSetSignature) - done <- result{message: signedMessage, err: err} - lock.Unlock() - return - } - - lock.Unlock() } for _, validator := range validators { @@ -214,7 +187,7 @@ func (s *SignatureAggregator) AggregateSignatures( requestBytes, onResponse, ); err != nil { - done <- result{err: err} + done <- err return } }() @@ -223,7 +196,22 @@ func (s *SignatureAggregator) AggregateSignatures( select { case <-ctx.Done(): return nil, ctx.Err() - case r := <-done: - return r.message, r.err + case err := <-done: + if err != nil { + return nil, err + } + + aggregateSignature, err := bls.AggregateSignatures(signatures) + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrFailedAggregation, err) + } + + bitSetSignature := &warp.BitSetSignature{ + Signers: signerBitSet.Bytes(), + Signature: [bls.SignatureLen]byte{}, + } + + copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) + return warp.NewMessage(message, bitSetSignature) } } diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index d622119d423f..0393b9edb75a 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + "github.com/ava-labs/avalanchego/utils/math" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/ids" @@ -42,7 +43,6 @@ func TestVerifier_Verify(t *testing.T) { ctx context.Context validators []Validator - threshold uint64 pChainState validators.State pChainHeight uint64 @@ -63,7 +63,6 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - threshold: 1, pChainState: &validatorstest.State{ T: t, GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { @@ -93,7 +92,6 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - threshold: 1, pChainState: &validatorstest.State{ T: t, GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { @@ -106,17 +104,49 @@ func TestVerifier_Verify(t *testing.T) { PublicKey: pk0, Weight: 1, }, + }, nil + }, + }, + }, + { + name: "overflow", + handler: NewHandler(&testVerifier{}, signer), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: math.MaxUint[uint64](), + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: math.MaxUint[uint64](), + }, + }, + pChainState: &validatorstest.State{ + T: t, + GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { + return ids.Empty, nil + }, + GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { + return map[ids.NodeID]*validators.GetValidatorOutput{ + nodeID0: { + NodeID: nodeID0, + PublicKey: pk0, + Weight: math.MaxUint[uint64](), + }, nodeID1: { NodeID: nodeID1, PublicKey: pk1, - Weight: 1, + Weight: math.MaxUint[uint64](), }, }, nil }, }, - quorumNum: 2, - quorumDen: 2, - wantVerifyErr: warp.ErrInsufficientWeight, + quorumNum: 1, + quorumDen: 2, + wantAggregateSignaturesErr: math.ErrOverflow, }, { name: "fails attestation", @@ -132,8 +162,7 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - threshold: 1, - wantAggregateSignaturesErr: ErrInsufficientSignatures, + wantAggregateSignaturesErr: ErrFailedAggregation, }, { name: "invalid validator set", @@ -178,7 +207,6 @@ func TestVerifier_Verify(t *testing.T) { message, []byte("justification"), tt.validators, - tt.threshold, ) require.ErrorIs(err, tt.wantAggregateSignaturesErr) From 9c2e9eb5ce762dc5b2020d7937e98a8835d792be Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Mon, 18 Nov 2024 04:40:06 -0500 Subject: [PATCH 10/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 277 ++++++++++++++++---------- network/p2p/acp118/aggregator_test.go | 51 ++++- 2 files changed, 219 insertions(+), 109 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index c75a8bd57cbe..993918e14972 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -11,7 +11,6 @@ import ( "github.com/ava-labs/avalanchego/utils/math" "go.uber.org/zap" - "golang.org/x/sync/semaphore" "google.golang.org/protobuf/proto" "github.com/ava-labs/avalanchego/ids" @@ -28,8 +27,8 @@ var ( // duplicate validators ErrDuplicateValidator = errors.New("duplicate validator") // ErrFailedAggregation is returned if it's not possible for us to - // generate a signature due to too many unsuccessful requests to peers - ErrFailedAggregation = errors.New("failed to aggregate signatures") + // generate a signature + ErrFailedAggregation = errors.New("failed aggregation") ) type Validator struct { @@ -43,6 +42,11 @@ type indexedValidator struct { Index int } +type result struct { + Signature *warp.BitSetSignature + Err error +} + // NewSignatureAggregator returns an instance of SignatureAggregator func NewSignatureAggregator( log logging.Logger, @@ -52,7 +56,7 @@ func NewSignatureAggregator( return &SignatureAggregator{ log: log, client: client, - maxPending: int64(maxPending), + maxPending: maxPending, } } @@ -60,15 +64,20 @@ func NewSignatureAggregator( type SignatureAggregator struct { log logging.Logger client *p2p.Client - maxPending int64 + maxPending int } -// AggregateSignatures blocks until signatures from validators are aggregated. +// AggregateSignatures blocks until quorumNum/quorumDen signatures from +// validators are requested to be aggregated into a warp message. func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.UnsignedMessage, justification []byte, validators []Validator, + quorumNumMin uint64, + quorumDenMin uint64, + quorumNumMax uint64, + quorumDenMax uint64, ) (*warp.Message, error) { request := &sdk.SignatureRequest{ Message: message.Bytes(), @@ -80,15 +89,10 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, fmt.Errorf("failed to marshal signature request: %w", err) } - done := make(chan error) - pendingRequests := semaphore.NewWeighted(s.maxPending) - lock := &sync.Mutex{} - aggregatedStakeWeight := uint64(0) - attemptedStakeWeight := uint64(0) - totalStakeWeight := uint64(0) - signatures := make([]*bls.Signature, 0) - signerBitSet := set.NewBits() + done := make(chan result) + sem := make(chan struct{}, s.maxPending) + totalStakeWeight := uint64(0) nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) for i, v := range validators { totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) @@ -96,7 +100,7 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, err } - // Sanity check the validator set provided by the caller + // Check that the validator set provided is valid if _, ok := nodeIDsToValidator[v.NodeID]; ok { return nil, fmt.Errorf("%w: %s", ErrDuplicateValidator, v.NodeID) } @@ -107,111 +111,184 @@ func (s *SignatureAggregator) AggregateSignatures( } } - onResponse := func( - _ context.Context, - nodeID ids.NodeID, - responseBytes []byte, - err error, - ) { - lock.Lock() + minThreshold := (totalStakeWeight * quorumNumMin) / quorumDenMin + maxThreshold := (totalStakeWeight * quorumNumMax) / quorumDenMax - // We are guaranteed a response from a node in the validator set - validator := nodeIDsToValidator[nodeID] - - defer func() { - attemptedStakeWeight += validator.Weight - lock.Unlock() + job := aggregationJob{ + log: s.log, + client: s.client, + requestBytes: requestBytes, + messageBytes: message.Bytes(), + minThreshold: minThreshold, + maxThreshold: maxThreshold, + validators: validators, + nodeIDsToValidator: nodeIDsToValidator, + totalStakeWeight: totalStakeWeight, + signatures: make([]*bls.Signature, 0), + signerBitSet: set.NewBits(), + sem: sem, + done: done, + } - if attemptedStakeWeight == totalStakeWeight { - done <- nil + for { + select { + case <-ctx.Done(): + // Try to return whatever progress we have made if the context is + // cancelled early + signature, err := job.GetResult() + if err != nil { + return nil, err } - pendingRequests.Release(1) - }() + return warp.NewMessage(message, signature) + case result := <-done: + if result.Err != nil { + return nil, result.Err + } - if err != nil { - s.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.Error(err), - ) - return + return warp.NewMessage(message, result.Signature) + case sem <- struct{}{}: + if err := job.SendRequest(ctx); err != nil { + return nil, err + } } + } +} - response := &sdk.SignatureResponse{} - if err := proto.Unmarshal(responseBytes, response); err != nil { - s.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.Error(err), - ) - return - } +type aggregationJob struct { + log logging.Logger + client *p2p.Client + requestBytes []byte + messageBytes []byte + minThreshold uint64 + maxThreshold uint64 + validators []Validator + index int + nodeIDsToValidator map[ids.NodeID]indexedValidator + totalStakeWeight uint64 - signature, err := bls.SignatureFromBytes(response.Signature) - if err != nil { - s.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.String("reason", "invalid signature"), - zap.Error(err), - ) - return - } + lock sync.Mutex + aggregatedStakeWeight uint64 + failedStakeWeight uint64 + signatures []*bls.Signature + signerBitSet set.Bits - if !bls.Verify(validator.PublicKey, signature, message.Bytes()) { - s.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.String("reason", "public key failed verification"), - ) - return - } + sem <-chan struct{} + done chan result +} + +func (a *aggregationJob) GetResult() (*warp.BitSetSignature, error) { + a.lock.Lock() + defer a.lock.Unlock() + + return a.getResult() +} - signerBitSet.Add(validator.Index) - signatures = append(signatures, signature) - aggregatedStakeWeight += validator.Weight +func (a *aggregationJob) getResult() (*warp.BitSetSignature, error) { + aggregateSignature, err := bls.AggregateSignatures(a.signatures) + if err != nil { + return nil, fmt.Errorf("%w: %s", ErrFailedAggregation, err) } - for _, validator := range validators { - if err := pendingRequests.Acquire(ctx, 1); err != nil { - return nil, err - } + bitSetSignature := &warp.BitSetSignature{ + Signers: a.signerBitSet.Bytes(), + Signature: [bls.SignatureLen]byte{}, + } - // Avoid loop shadowing in goroutine - validatorCopy := validator - go func() { - if err := s.client.AppRequest( - ctx, - set.Of(validatorCopy.NodeID), - requestBytes, - onResponse, - ); err != nil { - done <- err - return - } - }() + copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) + return bitSetSignature, nil +} + +func (a *aggregationJob) SendRequest(ctx context.Context) error { + if len(a.validators) == 0 { + return nil } - select { - case <-ctx.Done(): - return nil, ctx.Err() - case err := <-done: - if err != nil { - return nil, err + if err := a.client.AppRequest(ctx, set.Of(a.validators[a.index].NodeID), a.requestBytes, a.HandleResponse); err != nil { + return err + } + + a.index++ + + return nil +} + +func (a *aggregationJob) HandleResponse( + _ context.Context, + nodeID ids.NodeID, + responseBytes []byte, + err error, +) { + a.lock.Lock() + // We are guaranteed a response from a node in the validator set + validator := a.nodeIDsToValidator[nodeID] + failed := true + + defer func() { + defer func() { + a.lock.Unlock() + <-a.sem + }() + + if failed { + a.failedStakeWeight += validator.Weight } - aggregateSignature, err := bls.AggregateSignatures(signatures) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrFailedAggregation, err) + // Fast-fail if it's not possible to generate a signature that meets the + // minimum threshold + if a.totalStakeWeight-a.failedStakeWeight < a.minThreshold { + a.done <- result{Err: ErrFailedAggregation} + return } - bitSetSignature := &warp.BitSetSignature{ - Signers: signerBitSet.Bytes(), - Signature: [bls.SignatureLen]byte{}, + if a.aggregatedStakeWeight >= a.maxThreshold { + signature, err := a.getResult() + a.done <- result{Signature: signature, Err: err} + return } + }() + + if err != nil { + a.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.Error(err), + ) + return + } + + response := &sdk.SignatureResponse{} + if err := proto.Unmarshal(responseBytes, response); err != nil { + a.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.Error(err), + ) + return + } - copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) - return warp.NewMessage(message, bitSetSignature) + signature, err := bls.SignatureFromBytes(response.Signature) + if err != nil { + a.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.String("reason", "invalid signature"), + zap.Error(err), + ) + return } + + if !bls.Verify(validator.PublicKey, signature, a.messageBytes) { + a.log.Debug( + "dropping response", + zap.Stringer("nodeID", nodeID), + zap.String("reason", "public key failed verification"), + ) + return + } + + failed = false + a.signerBitSet.Add(validator.Index) + a.signatures = append(a.signatures, signature) + a.aggregatedStakeWeight += validator.Weight } diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 0393b9edb75a..93d7de8adb38 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -46,8 +46,10 @@ func TestVerifier_Verify(t *testing.T) { pChainState validators.State pChainHeight uint64 - quorumNum uint64 - quorumDen uint64 + quorumNumMin uint64 + quorumDenMin uint64 + quorumNumMax uint64 + quorumDenMax uint64 wantAggregateSignaturesErr error wantVerifyErr error @@ -78,8 +80,10 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNum: 1, - quorumDen: 1, + quorumNumMin: 1, + quorumDenMin: 1, + quorumNumMax: 1, + quorumDenMax: 1, }, { name: "gets signatures from insufficient stake", @@ -107,6 +111,10 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, + quorumNumMin: 1, + quorumDenMin: 1, + quorumNumMax: 1, + quorumDenMax: 1, }, { name: "overflow", @@ -144,8 +152,10 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNum: 1, - quorumDen: 2, + quorumNumMin: 1, + quorumDenMin: 2, + quorumNumMax: 1, + quorumDenMax: 2, wantAggregateSignaturesErr: math.ErrOverflow, }, { @@ -163,6 +173,10 @@ func TestVerifier_Verify(t *testing.T) { }, }, wantAggregateSignaturesErr: ErrFailedAggregation, + quorumNumMin: 1, + quorumDenMin: 1, + quorumNumMax: 1, + quorumDenMax: 1, }, { name: "invalid validator set", @@ -180,6 +194,10 @@ func TestVerifier_Verify(t *testing.T) { }, }, wantAggregateSignaturesErr: ErrDuplicateValidator, + quorumNumMin: 1, + quorumDenMin: 1, + quorumNumMax: 1, + quorumDenMax: 1, }, { name: "context canceled", @@ -189,7 +207,18 @@ func TestVerifier_Verify(t *testing.T) { return ctx }(), - wantAggregateSignaturesErr: context.Canceled, + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + wantAggregateSignaturesErr: ErrFailedAggregation, + quorumNumMin: 1, + quorumDenMin: 1, + quorumNumMax: 1, + quorumDenMax: 1, }, } @@ -207,6 +236,10 @@ func TestVerifier_Verify(t *testing.T) { message, []byte("justification"), tt.validators, + tt.quorumNumMin, + tt.quorumDenMin, + tt.quorumNumMax, + tt.quorumDenMax, ) require.ErrorIs(err, tt.wantAggregateSignaturesErr) @@ -220,8 +253,8 @@ func TestVerifier_Verify(t *testing.T) { networkID, tt.pChainState, 0, - tt.quorumNum, - tt.quorumDen, + tt.quorumNumMin, + tt.quorumDenMin, ) require.ErrorIs(err, tt.wantVerifyErr) }) From 6589763ab42cd13317cafc45c350f5ac8febfb8d Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:11:53 -0500 Subject: [PATCH 11/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 993918e14972..8c21bcc40407 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -187,7 +187,7 @@ func (a *aggregationJob) GetResult() (*warp.BitSetSignature, error) { func (a *aggregationJob) getResult() (*warp.BitSetSignature, error) { aggregateSignature, err := bls.AggregateSignatures(a.signatures) if err != nil { - return nil, fmt.Errorf("%w: %s", ErrFailedAggregation, err) + return nil, fmt.Errorf("%w: %w", ErrFailedAggregation, err) } bitSetSignature := &warp.BitSetSignature{ From c75e929bb6420365859afceec55af738898d0533 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 02:56:39 -0500 Subject: [PATCH 12/62] int Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 120 +++++++++++++++----------- network/p2p/acp118/aggregator_test.go | 71 ++++++--------- 2 files changed, 100 insertions(+), 91 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 8c21bcc40407..74bb5aa4133b 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -43,8 +43,8 @@ type indexedValidator struct { } type result struct { - Signature *warp.BitSetSignature - Err error + Message *warp.Message + Err error } // NewSignatureAggregator returns an instance of SignatureAggregator @@ -71,16 +71,14 @@ type SignatureAggregator struct { // validators are requested to be aggregated into a warp message. func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, - message *warp.UnsignedMessage, + message *warp.Message, justification []byte, validators []Validator, - quorumNumMin uint64, - quorumDenMin uint64, - quorumNumMax uint64, - quorumDenMax uint64, + quorumNum uint64, + quorumDen uint64, ) (*warp.Message, error) { request := &sdk.SignatureRequest{ - Message: message.Bytes(), + Message: message.UnsignedMessage.Bytes(), Justification: justification, } @@ -94,6 +92,20 @@ func (s *SignatureAggregator) AggregateSignatures( totalStakeWeight := uint64(0) nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) + // TODO expose concrete type to avoid type casting + bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) + if !ok { + return nil, fmt.Errorf("invalid warp signature type") + } + + var signerBitSet set.Bits + if bitSetSignature.Signers != nil { + signerBitSet = set.BitsFromBytes(bitSetSignature.Signers) + } else { + signerBitSet = set.NewBits() + } + + aggregatedStakeWeight := uint64(0) for i, v := range validators { totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) if err != nil { @@ -109,25 +121,34 @@ func (s *SignatureAggregator) AggregateSignatures( Index: i, Validator: v, } + + if signerBitSet.Contains(i) { + aggregatedStakeWeight += v.Weight + } } - minThreshold := (totalStakeWeight * quorumNumMin) / quorumDenMin - maxThreshold := (totalStakeWeight * quorumNumMax) / quorumDenMax + signatures := make([]*bls.Signature, 0, 1) + if bitSetSignature.Signature != [bls.SignatureLen]byte{} { + blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) + if err != nil { + return nil, fmt.Errorf("failed to parse bls signature: %w", err) + } + signatures = append(signatures, blsSignature) + } job := aggregationJob{ - log: s.log, - client: s.client, - requestBytes: requestBytes, - messageBytes: message.Bytes(), - minThreshold: minThreshold, - maxThreshold: maxThreshold, - validators: validators, - nodeIDsToValidator: nodeIDsToValidator, - totalStakeWeight: totalStakeWeight, - signatures: make([]*bls.Signature, 0), - signerBitSet: set.NewBits(), - sem: sem, - done: done, + log: s.log, + client: s.client, + requestBytes: requestBytes, + message: message, + minThreshold: (totalStakeWeight * quorumNum) / quorumDen, + nodeIDsToValidator: nodeIDsToValidator, + totalStakeWeight: totalStakeWeight, + aggregatedStakeWeight: aggregatedStakeWeight, + signatures: signatures, + signerBitSet: signerBitSet, + sem: sem, + done: done, } for { @@ -135,18 +156,18 @@ func (s *SignatureAggregator) AggregateSignatures( case <-ctx.Done(): // Try to return whatever progress we have made if the context is // cancelled early - signature, err := job.GetResult() + result, err := job.GetResult() if err != nil { return nil, err } - return warp.NewMessage(message, signature) + return result, nil case result := <-done: if result.Err != nil { return nil, result.Err } - return warp.NewMessage(message, result.Signature) + return result.Message, nil case sem <- struct{}{}: if err := job.SendRequest(ctx); err != nil { return nil, err @@ -159,13 +180,10 @@ type aggregationJob struct { log logging.Logger client *p2p.Client requestBytes []byte - messageBytes []byte + message *warp.Message minThreshold uint64 - maxThreshold uint64 - validators []Validator - index int nodeIDsToValidator map[ids.NodeID]indexedValidator - totalStakeWeight uint64 + totalStakeWeight uint64 // TODO initialize this correctly lock sync.Mutex aggregatedStakeWeight uint64 @@ -177,14 +195,14 @@ type aggregationJob struct { done chan result } -func (a *aggregationJob) GetResult() (*warp.BitSetSignature, error) { +func (a *aggregationJob) GetResult() (*warp.Message, error) { a.lock.Lock() defer a.lock.Unlock() return a.getResult() } -func (a *aggregationJob) getResult() (*warp.BitSetSignature, error) { +func (a *aggregationJob) getResult() (*warp.Message, error) { aggregateSignature, err := bls.AggregateSignatures(a.signatures) if err != nil { return nil, fmt.Errorf("%w: %w", ErrFailedAggregation, err) @@ -196,20 +214,26 @@ func (a *aggregationJob) getResult() (*warp.BitSetSignature, error) { } copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) - return bitSetSignature, nil + unsignedMessage, err := warp.NewUnsignedMessage(a.message.NetworkID, a.message.SourceChainID, a.message.Payload) + if err != nil { + return nil, fmt.Errorf("failed to initialize message") + } + + return warp.NewMessage(unsignedMessage, bitSetSignature) } func (a *aggregationJob) SendRequest(ctx context.Context) error { - if len(a.validators) == 0 { - return nil - } + // TODO dont rely on map iteration order and use weighted sampling instead + for nodeID, validator := range a.nodeIDsToValidator { + if a.signerBitSet.Contains(validator.Index) { + continue + } - if err := a.client.AppRequest(ctx, set.Of(a.validators[a.index].NodeID), a.requestBytes, a.HandleResponse); err != nil { - return err + if err := a.client.AppRequest(ctx, set.Of(nodeID), a.requestBytes, a.HandleResponse); err != nil { + return err + } } - a.index++ - return nil } @@ -230,6 +254,12 @@ func (a *aggregationJob) HandleResponse( <-a.sem }() + if a.aggregatedStakeWeight >= a.minThreshold { + signature, err := a.getResult() + a.done <- result{Message: signature, Err: err} + return + } + if failed { a.failedStakeWeight += validator.Weight } @@ -240,12 +270,6 @@ func (a *aggregationJob) HandleResponse( a.done <- result{Err: ErrFailedAggregation} return } - - if a.aggregatedStakeWeight >= a.maxThreshold { - signature, err := a.getResult() - a.done <- result{Signature: signature, Err: err} - return - } }() if err != nil { @@ -278,7 +302,7 @@ func (a *aggregationJob) HandleResponse( return } - if !bls.Verify(validator.PublicKey, signature, a.messageBytes) { + if !bls.Verify(validator.PublicKey, signature, a.message.UnsignedMessage.Bytes()) { a.log.Debug( "dropping response", zap.Stringer("nodeID", nodeID), diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 93d7de8adb38..5eab3b5df064 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,9 +7,6 @@ import ( "context" "testing" - "github.com/ava-labs/avalanchego/utils/math" - "github.com/stretchr/testify/require" - "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/network/p2p" "github.com/ava-labs/avalanchego/network/p2p/p2ptest" @@ -18,7 +15,9 @@ import ( "github.com/ava-labs/avalanchego/snow/validators/validatorstest" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/logging" + "github.com/ava-labs/avalanchego/utils/math" "github.com/ava-labs/avalanchego/vms/platformvm/warp" + "github.com/stretchr/testify/require" ) func TestVerifier_Verify(t *testing.T) { @@ -46,10 +45,8 @@ func TestVerifier_Verify(t *testing.T) { pChainState validators.State pChainHeight uint64 - quorumNumMin uint64 - quorumDenMin uint64 - quorumNumMax uint64 - quorumDenMax uint64 + quorumNum uint64 + quorumDen uint64 wantAggregateSignaturesErr error wantVerifyErr error @@ -80,10 +77,8 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNumMin: 1, - quorumDenMin: 1, - quorumNumMax: 1, - quorumDenMax: 1, + quorumNum: 1, + quorumDen: 1, }, { name: "gets signatures from insufficient stake", @@ -111,10 +106,8 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNumMin: 1, - quorumDenMin: 1, - quorumNumMax: 1, - quorumDenMax: 1, + quorumNum: 1, + quorumDen: 1, }, { name: "overflow", @@ -152,10 +145,8 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNumMin: 1, - quorumDenMin: 2, - quorumNumMax: 1, - quorumDenMax: 2, + quorumNum: 1, + quorumDen: 2, wantAggregateSignaturesErr: math.ErrOverflow, }, { @@ -173,10 +164,8 @@ func TestVerifier_Verify(t *testing.T) { }, }, wantAggregateSignaturesErr: ErrFailedAggregation, - quorumNumMin: 1, - quorumDenMin: 1, - quorumNumMax: 1, - quorumDenMax: 1, + quorumNum: 1, + quorumDen: 1, }, { name: "invalid validator set", @@ -194,10 +183,8 @@ func TestVerifier_Verify(t *testing.T) { }, }, wantAggregateSignaturesErr: ErrDuplicateValidator, - quorumNumMin: 1, - quorumDenMin: 1, - quorumNumMax: 1, - quorumDenMax: 1, + quorumNum: 1, + quorumDen: 1, }, { name: "context canceled", @@ -215,10 +202,8 @@ func TestVerifier_Verify(t *testing.T) { }, }, wantAggregateSignaturesErr: ErrFailedAggregation, - quorumNumMin: 1, - quorumDenMin: 1, - quorumNumMax: 1, - quorumDenMax: 1, + quorumNum: 1, + quorumDen: 1, }, } @@ -226,20 +211,20 @@ func TestVerifier_Verify(t *testing.T) { t.Run(tt.name, func(t *testing.T) { require := require.New(t) - message, err := warp.NewUnsignedMessage(networkID, chainID, []byte("payload")) + unsignedMsg, err := warp.NewUnsignedMessage(networkID, chainID, []byte("payload")) + require.NoError(err) + msg, err := warp.NewMessage(unsignedMsg, &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}) require.NoError(err) client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) - verifier := NewSignatureAggregator(logging.NoLog{}, client, 1) + aggregator := NewSignatureAggregator(logging.NoLog{}, client, 1) - signedMessage, err := verifier.AggregateSignatures( + gotMsg, err := aggregator.AggregateSignatures( tt.ctx, - message, + msg, []byte("justification"), tt.validators, - tt.quorumNumMin, - tt.quorumDenMin, - tt.quorumNumMax, - tt.quorumDenMax, + tt.quorumNum, + tt.quorumDen, ) require.ErrorIs(err, tt.wantAggregateSignaturesErr) @@ -247,14 +232,14 @@ func TestVerifier_Verify(t *testing.T) { return } - err = signedMessage.Signature.Verify( + err = gotMsg.Signature.Verify( context.Background(), - &signedMessage.UnsignedMessage, + &gotMsg.UnsignedMessage, networkID, tt.pChainState, 0, - tt.quorumNumMin, - tt.quorumDenMin, + tt.quorumNum, + tt.quorumDen, ) require.ErrorIs(err, tt.wantVerifyErr) }) From ce284e6c25928795c6dc0d6f9bca01dab7108863 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 02:58:48 -0500 Subject: [PATCH 13/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 74bb5aa4133b..4bde04816e86 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -9,7 +9,6 @@ import ( "fmt" "sync" - "github.com/ava-labs/avalanchego/utils/math" "go.uber.org/zap" "google.golang.org/protobuf/proto" @@ -18,6 +17,7 @@ import ( "github.com/ava-labs/avalanchego/proto/pb/sdk" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/logging" + "github.com/ava-labs/avalanchego/utils/math" "github.com/ava-labs/avalanchego/utils/set" "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) From f7e2ca960d03e6bbce232dfe89c7af4a965d3805 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 02:59:05 -0500 Subject: [PATCH 14/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 5eab3b5df064..be01af5ee7a8 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,6 +7,8 @@ import ( "context" "testing" + "github.com/stretchr/testify/require" + "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/network/p2p" "github.com/ava-labs/avalanchego/network/p2p/p2ptest" @@ -17,7 +19,6 @@ import ( "github.com/ava-labs/avalanchego/utils/logging" "github.com/ava-labs/avalanchego/utils/math" "github.com/ava-labs/avalanchego/vms/platformvm/warp" - "github.com/stretchr/testify/require" ) func TestVerifier_Verify(t *testing.T) { From ef9e7ec02a0ce71b9de0ce65552b2f126adcdb18 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 03:04:56 -0500 Subject: [PATCH 15/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 4bde04816e86..fc37a06f8673 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -95,7 +95,7 @@ func (s *SignatureAggregator) AggregateSignatures( // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) if !ok { - return nil, fmt.Errorf("invalid warp signature type") + return nil, errors.New("invalid warp signature type") } var signerBitSet set.Bits From a8642a58ec3b608f1934b9087bed174263e23926 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 03:05:16 -0500 Subject: [PATCH 16/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index fc37a06f8673..5e36ba69f8a8 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -216,7 +216,7 @@ func (a *aggregationJob) getResult() (*warp.Message, error) { copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) unsignedMessage, err := warp.NewUnsignedMessage(a.message.NetworkID, a.message.SourceChainID, a.message.Payload) if err != nil { - return nil, fmt.Errorf("failed to initialize message") + return nil, fmt.Errorf("failed to initialize message: %w", err) } return warp.NewMessage(unsignedMessage, bitSetSignature) From aa5da476d70d1fb076727fc2097badce16ef208b Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 03:07:08 -0500 Subject: [PATCH 17/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index be01af5ee7a8..6370cf2c3162 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -169,8 +169,9 @@ func TestVerifier_Verify(t *testing.T) { quorumDen: 1, }, { - name: "invalid validator set", - ctx: context.Background(), + name: "invalid validator set", + handler: NewHandler(&testVerifier{}, signer), + ctx: context.Background(), validators: []Validator{ { NodeID: nodeID0, @@ -188,7 +189,8 @@ func TestVerifier_Verify(t *testing.T) { quorumDen: 1, }, { - name: "context canceled", + name: "context canceled", + handler: NewHandler(&testVerifier{}, signer), ctx: func() context.Context { ctx, cancel := context.WithCancel(context.Background()) cancel() From a44b653da9e98ffc25c7b9bc388894fca514ed4a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 14:49:54 -0500 Subject: [PATCH 18/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 137 +++++++++++++------------------ 1 file changed, 59 insertions(+), 78 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 5e36ba69f8a8..fc842d8d3bcb 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -22,14 +22,9 @@ import ( "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) -var ( - // ErrDuplicateValidator is returned if the provided validator set contains - // duplicate validators - ErrDuplicateValidator = errors.New("duplicate validator") - // ErrFailedAggregation is returned if it's not possible for us to - // generate a signature - ErrFailedAggregation = errors.New("failed aggregation") -) +// ErrFailedAggregation is returned if it's not possible for us to +// generate a signature +var ErrFailedAggregation = errors.New("failed aggregation") type Validator struct { NodeID ids.NodeID @@ -43,8 +38,10 @@ type indexedValidator struct { } type result struct { - Message *warp.Message - Err error + Message *warp.Message + AggregatedWeight uint64 + TotalWeight uint64 + Err error } // NewSignatureAggregator returns an instance of SignatureAggregator @@ -68,7 +65,10 @@ type SignatureAggregator struct { } // AggregateSignatures blocks until quorumNum/quorumDen signatures from -// validators are requested to be aggregated into a warp message. +// validators are requested to be aggregated into a warp message or the context +// is canceled. Returns the signed message and the amount of stake that signed +// the message. Caller is responsible for providing a well-formed canonical +// validator set corresponding to the signer bitset in the message. func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.Message, @@ -76,7 +76,7 @@ func (s *SignatureAggregator) AggregateSignatures( validators []Validator, quorumNum uint64, quorumDen uint64, -) (*warp.Message, error) { +) (*warp.Message, uint64, uint64, error) { request := &sdk.SignatureRequest{ Message: message.UnsignedMessage.Bytes(), Justification: justification, @@ -84,18 +84,16 @@ func (s *SignatureAggregator) AggregateSignatures( requestBytes, err := proto.Marshal(request) if err != nil { - return nil, fmt.Errorf("failed to marshal signature request: %w", err) + return nil, 0, 0, fmt.Errorf("failed to marshal signature request: %w", err) } done := make(chan result) - sem := make(chan struct{}, s.maxPending) - totalStakeWeight := uint64(0) nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) if !ok { - return nil, errors.New("invalid warp signature type") + return nil, 0, 0, errors.New("invalid warp signature type") } var signerBitSet set.Bits @@ -105,41 +103,39 @@ func (s *SignatureAggregator) AggregateSignatures( signerBitSet = set.NewBits() } + sampleable := make([]ids.NodeID, 0, len(validators)) aggregatedStakeWeight := uint64(0) for i, v := range validators { totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) if err != nil { - return nil, err + return nil, 0, 0, err } - // Check that the validator set provided is valid - if _, ok := nodeIDsToValidator[v.NodeID]; ok { - return nil, fmt.Errorf("%w: %s", ErrDuplicateValidator, v.NodeID) + // Only try to aggregate signatures from validators that are not already in + // the signer bit set + if signerBitSet.Contains(i) { + aggregatedStakeWeight += v.Weight + continue } nodeIDsToValidator[v.NodeID] = indexedValidator{ Index: i, Validator: v, } - - if signerBitSet.Contains(i) { - aggregatedStakeWeight += v.Weight - } + sampleable = append(sampleable, v.NodeID) } signatures := make([]*bls.Signature, 0, 1) if bitSetSignature.Signature != [bls.SignatureLen]byte{} { blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) if err != nil { - return nil, fmt.Errorf("failed to parse bls signature: %w", err) + return nil, 0, 0, fmt.Errorf("failed to parse bls signature: %w", err) } signatures = append(signatures, blsSignature) } job := aggregationJob{ log: s.log, - client: s.client, - requestBytes: requestBytes, message: message, minThreshold: (totalStakeWeight * quorumNum) / quorumDen, nodeIDsToValidator: nodeIDsToValidator, @@ -147,43 +143,40 @@ func (s *SignatureAggregator) AggregateSignatures( aggregatedStakeWeight: aggregatedStakeWeight, signatures: signatures, signerBitSet: signerBitSet, - sem: sem, done: done, } - for { - select { - case <-ctx.Done(): - // Try to return whatever progress we have made if the context is - // cancelled early - result, err := job.GetResult() - if err != nil { - return nil, err - } - - return result, nil - case result := <-done: - if result.Err != nil { - return nil, result.Err - } - - return result.Message, nil - case sem <- struct{}{}: - if err := job.SendRequest(ctx); err != nil { - return nil, err - } + for _, nodeID := range sampleable { + if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, job.HandleResponse); err != nil { + return nil, 0, 0, err } } + + select { + case <-ctx.Done(): + // Try to return whatever progress we have made if the context is + // cancelled early + result := job.GetResult() + if result.Err != nil { + return nil, 0, 0, result.Err + } + + return result.Message, result.AggregatedWeight, result.TotalWeight, nil + case result := <-done: + if result.Err != nil { + return nil, 0, 0, result.Err + } + + return result.Message, result.AggregatedWeight, result.TotalWeight, nil + } } type aggregationJob struct { log logging.Logger - client *p2p.Client - requestBytes []byte message *warp.Message minThreshold uint64 nodeIDsToValidator map[ids.NodeID]indexedValidator - totalStakeWeight uint64 // TODO initialize this correctly + totalStakeWeight uint64 lock sync.Mutex aggregatedStakeWeight uint64 @@ -191,21 +184,20 @@ type aggregationJob struct { signatures []*bls.Signature signerBitSet set.Bits - sem <-chan struct{} done chan result } -func (a *aggregationJob) GetResult() (*warp.Message, error) { +func (a *aggregationJob) GetResult() result { a.lock.Lock() defer a.lock.Unlock() return a.getResult() } -func (a *aggregationJob) getResult() (*warp.Message, error) { +func (a *aggregationJob) getResult() result { aggregateSignature, err := bls.AggregateSignatures(a.signatures) if err != nil { - return nil, fmt.Errorf("%w: %w", ErrFailedAggregation, err) + return result{Err: fmt.Errorf("%w: %w", ErrFailedAggregation, err)} } bitSetSignature := &warp.BitSetSignature{ @@ -216,25 +208,19 @@ func (a *aggregationJob) getResult() (*warp.Message, error) { copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) unsignedMessage, err := warp.NewUnsignedMessage(a.message.NetworkID, a.message.SourceChainID, a.message.Payload) if err != nil { - return nil, fmt.Errorf("failed to initialize message: %w", err) + return result{Err: fmt.Errorf("failed to initialize message: %w", err)} } - return warp.NewMessage(unsignedMessage, bitSetSignature) -} - -func (a *aggregationJob) SendRequest(ctx context.Context) error { - // TODO dont rely on map iteration order and use weighted sampling instead - for nodeID, validator := range a.nodeIDsToValidator { - if a.signerBitSet.Contains(validator.Index) { - continue - } - - if err := a.client.AppRequest(ctx, set.Of(nodeID), a.requestBytes, a.HandleResponse); err != nil { - return err - } + msg, err := warp.NewMessage(unsignedMessage, bitSetSignature) + if err != nil { + return result{Err: err} } - return nil + return result{ + Message: msg, + AggregatedWeight: a.aggregatedStakeWeight, + TotalWeight: a.totalStakeWeight, + } } func (a *aggregationJob) HandleResponse( @@ -248,15 +234,10 @@ func (a *aggregationJob) HandleResponse( validator := a.nodeIDsToValidator[nodeID] failed := true + defer a.lock.Unlock() defer func() { - defer func() { - a.lock.Unlock() - <-a.sem - }() - if a.aggregatedStakeWeight >= a.minThreshold { - signature, err := a.getResult() - a.done <- result{Message: signature, Err: err} + a.done <- a.getResult() return } From 26bb1a0dd2e01ba0d497349e6746fb1efc56b999 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:02:49 -0500 Subject: [PATCH 19/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 225 ++++++++++---------------- network/p2p/acp118/aggregator_test.go | 36 ++--- 2 files changed, 98 insertions(+), 163 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index fc842d8d3bcb..169d8607a8ff 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -7,9 +7,7 @@ import ( "context" "errors" "fmt" - "sync" - "go.uber.org/zap" "google.golang.org/protobuf/proto" "github.com/ava-labs/avalanchego/ids" @@ -38,30 +36,23 @@ type indexedValidator struct { } type result struct { - Message *warp.Message - AggregatedWeight uint64 - TotalWeight uint64 - Err error + Validator indexedValidator + Signature *bls.Signature + Err error } // NewSignatureAggregator returns an instance of SignatureAggregator -func NewSignatureAggregator( - log logging.Logger, - client *p2p.Client, - maxPending int, -) *SignatureAggregator { +func NewSignatureAggregator(log logging.Logger, client *p2p.Client) *SignatureAggregator { return &SignatureAggregator{ - log: log, - client: client, - maxPending: maxPending, + log: log, + client: client, } } // SignatureAggregator aggregates validator signatures for warp messages type SignatureAggregator struct { - log logging.Logger - client *p2p.Client - maxPending int + log logging.Logger + client *p2p.Client } // AggregateSignatures blocks until quorumNum/quorumDen signatures from @@ -87,8 +78,6 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, 0, 0, fmt.Errorf("failed to marshal signature request: %w", err) } - done := make(chan result) - totalStakeWeight := uint64(0) nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) @@ -105,6 +94,7 @@ func (s *SignatureAggregator) AggregateSignatures( sampleable := make([]ids.NodeID, 0, len(validators)) aggregatedStakeWeight := uint64(0) + totalStakeWeight := uint64(0) for i, v := range validators { totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) if err != nil { @@ -125,7 +115,7 @@ func (s *SignatureAggregator) AggregateSignatures( sampleable = append(sampleable, v.NodeID) } - signatures := make([]*bls.Signature, 0, 1) + signatures := make([]*bls.Signature, 0, len(sampleable)+1) if bitSetSignature.Signature != [bls.SignatureLen]byte{} { blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) if err != nil { @@ -134,166 +124,123 @@ func (s *SignatureAggregator) AggregateSignatures( signatures = append(signatures, blsSignature) } - job := aggregationJob{ - log: s.log, - message: message, - minThreshold: (totalStakeWeight * quorumNum) / quorumDen, - nodeIDsToValidator: nodeIDsToValidator, - totalStakeWeight: totalStakeWeight, - aggregatedStakeWeight: aggregatedStakeWeight, - signatures: signatures, - signerBitSet: signerBitSet, - done: done, + results := make(chan result) + job := responseHandler{ + message: message, + nodeIDsToValidator: nodeIDsToValidator, + results: results, } for _, nodeID := range sampleable { - if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, job.HandleResponse); err != nil { - return nil, 0, 0, err + // Avoid var shadowing in goroutine + nodeIDCopy := nodeID + go func() { + if err := s.client.AppRequest(ctx, set.Of(nodeIDCopy), requestBytes, job.HandleResponse); err != nil { + results <- result{Validator: nodeIDsToValidator[nodeIDCopy], Err: err} + return + } + }() + } + + failedStakeWeight := uint64(0) + minThreshold := (totalStakeWeight * quorumNum) / quorumDen + + for { + select { + case <-ctx.Done(): + // Try to return whatever progress we have if the context is cancelled + msg, err := s.newWarpMessage(message, signerBitSet, signatures) + if err != nil { + return nil, 0, 0, err + } + + return msg, aggregatedStakeWeight, totalStakeWeight, nil + case result := <-results: + if result.Err != nil { + return nil, 0, 0, result.Err + } + + if result.Err != nil { + failedStakeWeight += result.Validator.Weight + continue + } + + signatures = append(signatures, result.Signature) + signerBitSet.Add(result.Validator.Index) + aggregatedStakeWeight += result.Validator.Weight + + if aggregatedStakeWeight >= minThreshold { + msg, err := s.newWarpMessage(message, signerBitSet, signatures) + if err != nil { + return nil, 0, 0, err + } + + return msg, aggregatedStakeWeight, totalStakeWeight, nil + } + + // Fast-fail if it's not possible to generate a signature that meets the + // minimum threshold + if totalStakeWeight-failedStakeWeight < minThreshold { + return nil, 0, 0, ErrFailedAggregation + } } } - - select { - case <-ctx.Done(): - // Try to return whatever progress we have made if the context is - // cancelled early - result := job.GetResult() - if result.Err != nil { - return nil, 0, 0, result.Err - } - - return result.Message, result.AggregatedWeight, result.TotalWeight, nil - case result := <-done: - if result.Err != nil { - return nil, 0, 0, result.Err - } - - return result.Message, result.AggregatedWeight, result.TotalWeight, nil - } -} - -type aggregationJob struct { - log logging.Logger - message *warp.Message - minThreshold uint64 - nodeIDsToValidator map[ids.NodeID]indexedValidator - totalStakeWeight uint64 - - lock sync.Mutex - aggregatedStakeWeight uint64 - failedStakeWeight uint64 - signatures []*bls.Signature - signerBitSet set.Bits - - done chan result -} - -func (a *aggregationJob) GetResult() result { - a.lock.Lock() - defer a.lock.Unlock() - - return a.getResult() } -func (a *aggregationJob) getResult() result { - aggregateSignature, err := bls.AggregateSignatures(a.signatures) +func (s *SignatureAggregator) newWarpMessage( + message *warp.Message, + signerBitSet set.Bits, + signatures []*bls.Signature, +) (*warp.Message, error) { + aggregateSignature, err := bls.AggregateSignatures(signatures) if err != nil { - return result{Err: fmt.Errorf("%w: %w", ErrFailedAggregation, err)} + return nil, err } bitSetSignature := &warp.BitSetSignature{ - Signers: a.signerBitSet.Bytes(), + Signers: signerBitSet.Bytes(), Signature: [bls.SignatureLen]byte{}, } - copy(bitSetSignature.Signature[:], bls.SignatureToBytes(aggregateSignature)) - unsignedMessage, err := warp.NewUnsignedMessage(a.message.NetworkID, a.message.SourceChainID, a.message.Payload) - if err != nil { - return result{Err: fmt.Errorf("failed to initialize message: %w", err)} - } - msg, err := warp.NewMessage(unsignedMessage, bitSetSignature) - if err != nil { - return result{Err: err} - } + return warp.NewMessage(&message.UnsignedMessage, bitSetSignature) +} - return result{ - Message: msg, - AggregatedWeight: a.aggregatedStakeWeight, - TotalWeight: a.totalStakeWeight, - } +type responseHandler struct { + message *warp.Message + nodeIDsToValidator map[ids.NodeID]indexedValidator + results chan result } -func (a *aggregationJob) HandleResponse( +func (a *responseHandler) HandleResponse( _ context.Context, nodeID ids.NodeID, responseBytes []byte, err error, ) { - a.lock.Lock() - // We are guaranteed a response from a node in the validator set validator := a.nodeIDsToValidator[nodeID] - failed := true - - defer a.lock.Unlock() - defer func() { - if a.aggregatedStakeWeight >= a.minThreshold { - a.done <- a.getResult() - return - } - - if failed { - a.failedStakeWeight += validator.Weight - } - - // Fast-fail if it's not possible to generate a signature that meets the - // minimum threshold - if a.totalStakeWeight-a.failedStakeWeight < a.minThreshold { - a.done <- result{Err: ErrFailedAggregation} - return - } - }() if err != nil { - a.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.Error(err), - ) + a.results <- result{Validator: validator, Err: err} return } response := &sdk.SignatureResponse{} if err := proto.Unmarshal(responseBytes, response); err != nil { - a.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.Error(err), - ) + a.results <- result{Validator: validator, Err: err} return } signature, err := bls.SignatureFromBytes(response.Signature) if err != nil { - a.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.String("reason", "invalid signature"), - zap.Error(err), - ) + a.results <- result{Validator: validator, Err: err} return } if !bls.Verify(validator.PublicKey, signature, a.message.UnsignedMessage.Bytes()) { - a.log.Debug( - "dropping response", - zap.Stringer("nodeID", nodeID), - zap.String("reason", "public key failed verification"), - ) + a.results <- result{Validator: validator, Err: err} return } - failed = false - a.signerBitSet.Add(validator.Index) - a.signatures = append(a.signatures, signature) - a.aggregatedStakeWeight += validator.Weight + a.results <- result{Validator: validator, Signature: signature} } diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 6370cf2c3162..2b3267905d8b 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -107,8 +107,9 @@ func TestVerifier_Verify(t *testing.T) { }, nil }, }, - quorumNum: 1, - quorumDen: 1, + quorumNum: 1, + quorumDen: 1, + wantAggregateSignaturesErr: ErrFailedAggregation, }, { name: "overflow", @@ -168,26 +169,6 @@ func TestVerifier_Verify(t *testing.T) { quorumNum: 1, quorumDen: 1, }, - { - name: "invalid validator set", - handler: NewHandler(&testVerifier{}, signer), - ctx: context.Background(), - validators: []Validator{ - { - NodeID: nodeID0, - PublicKey: pk0, - Weight: 1, - }, - { - NodeID: nodeID0, - PublicKey: pk0, - Weight: 1, - }, - }, - wantAggregateSignaturesErr: ErrDuplicateValidator, - quorumNum: 1, - quorumDen: 1, - }, { name: "context canceled", handler: NewHandler(&testVerifier{}, signer), @@ -219,9 +200,9 @@ func TestVerifier_Verify(t *testing.T) { msg, err := warp.NewMessage(unsignedMsg, &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}) require.NoError(err) client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) - aggregator := NewSignatureAggregator(logging.NoLog{}, client, 1) + aggregator := NewSignatureAggregator(logging.NoLog{}, client) - gotMsg, err := aggregator.AggregateSignatures( + gotMsg, _, gotDen, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -235,6 +216,13 @@ func TestVerifier_Verify(t *testing.T) { return } + wantDen := uint64(0) + for _, v := range tt.validators { + wantDen += v.Weight + } + + require.Equal(wantDen, gotDen) + err = gotMsg.Signature.Verify( context.Background(), &gotMsg.UnsignedMessage, From d2139cce7345329374c6e8b4ac13e5c7a33a62a8 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:22:53 -0500 Subject: [PATCH 20/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 30 +++--- network/p2p/acp118/aggregator_test.go | 145 ++++++++------------------ 2 files changed, 60 insertions(+), 115 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 169d8607a8ff..781ff2d2fd01 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" + "go.uber.org/zap" "google.golang.org/protobuf/proto" "github.com/ava-labs/avalanchego/ids" @@ -148,6 +149,10 @@ func (s *SignatureAggregator) AggregateSignatures( for { select { case <-ctx.Done(): + if len(signatures) == 0 { + return message, 0, totalStakeWeight, nil + } + // Try to return whatever progress we have if the context is cancelled msg, err := s.newWarpMessage(message, signerBitSet, signatures) if err != nil { @@ -157,11 +162,19 @@ func (s *SignatureAggregator) AggregateSignatures( return msg, aggregatedStakeWeight, totalStakeWeight, nil case result := <-results: if result.Err != nil { - return nil, 0, 0, result.Err - } - - if result.Err != nil { + s.log.Debug( + "dropping response", + zap.Stringer("nodeID", result.Validator.NodeID), + zap.Uint64("weight", result.Validator.Weight), + zap.Error(err), + ) + + // Fast-fail if it's not possible to generate a signature that meets the + // minimum threshold failedStakeWeight += result.Validator.Weight + if totalStakeWeight-failedStakeWeight < minThreshold { + return nil, 0, 0, ErrFailedAggregation + } continue } @@ -177,12 +190,6 @@ func (s *SignatureAggregator) AggregateSignatures( return msg, aggregatedStakeWeight, totalStakeWeight, nil } - - // Fast-fail if it's not possible to generate a signature that meets the - // minimum threshold - if totalStakeWeight-failedStakeWeight < minThreshold { - return nil, 0, 0, ErrFailedAggregation - } } } } @@ -190,8 +197,7 @@ func (s *SignatureAggregator) AggregateSignatures( func (s *SignatureAggregator) newWarpMessage( message *warp.Message, signerBitSet set.Bits, - signatures []*bls.Signature, -) (*warp.Message, error) { + signatures []*bls.Signature) (*warp.Message, error) { aggregateSignature, err := bls.AggregateSignatures(signatures) if err != nil { return nil, err diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 2b3267905d8b..521468d7a51a 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,17 +7,15 @@ import ( "context" "testing" + "github.com/ava-labs/avalanchego/utils/math" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/network/p2p" "github.com/ava-labs/avalanchego/network/p2p/p2ptest" "github.com/ava-labs/avalanchego/snow/engine/common" - "github.com/ava-labs/avalanchego/snow/validators" - "github.com/ava-labs/avalanchego/snow/validators/validatorstest" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/logging" - "github.com/ava-labs/avalanchego/utils/math" "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) @@ -37,23 +35,16 @@ func TestVerifier_Verify(t *testing.T) { signer := warp.NewSigner(sk0, networkID, chainID) tests := []struct { - name string - - handler p2p.Handler - - ctx context.Context - validators []Validator - - pChainState validators.State - pChainHeight uint64 - quorumNum uint64 - quorumDen uint64 - + name string + handler p2p.Handler + ctx context.Context + validators []Validator + quorumNum uint64 + quorumDen uint64 wantAggregateSignaturesErr error - wantVerifyErr error }{ { - name: "gets signatures from sufficient stake", + name: "aggregates from all validators", handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ @@ -63,48 +54,28 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - pChainState: &validatorstest.State{ - T: t, - GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { - return ids.Empty, nil - }, - GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { - return map[ids.NodeID]*validators.GetValidatorOutput{ - nodeID0: { - NodeID: nodeID0, - PublicKey: pk0, - Weight: 1, - }, - }, nil - }, - }, quorumNum: 1, quorumDen: 1, }, { - name: "gets signatures from insufficient stake", - handler: NewHandler(&testVerifier{}, signer), - ctx: context.Background(), + name: "fails aggregation from some validators", + handler: NewHandler( + &testVerifier{ + Errs: []*common.AppError{common.ErrUndefined}, + }, + signer, + ), + ctx: context.Background(), validators: []Validator{ { NodeID: nodeID0, PublicKey: pk0, Weight: 1, }, - }, - pChainState: &validatorstest.State{ - T: t, - GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { - return ids.Empty, nil - }, - GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { - return map[ids.NodeID]*validators.GetValidatorOutput{ - nodeID0: { - NodeID: nodeID0, - PublicKey: pk0, - Weight: 1, - }, - }, nil + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 1, }, }, quorumNum: 1, @@ -112,7 +83,25 @@ func TestVerifier_Verify(t *testing.T) { wantAggregateSignaturesErr: ErrFailedAggregation, }, { - name: "overflow", + name: "fails aggregation from all validators", + handler: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer, + ), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + }, + wantAggregateSignaturesErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, + }, + { + name: "stake overflow", handler: NewHandler(&testVerifier{}, signer), ctx: context.Background(), validators: []Validator{ @@ -127,48 +116,10 @@ func TestVerifier_Verify(t *testing.T) { Weight: math.MaxUint[uint64](), }, }, - pChainState: &validatorstest.State{ - T: t, - GetSubnetIDF: func(context.Context, ids.ID) (ids.ID, error) { - return ids.Empty, nil - }, - GetValidatorSetF: func(context.Context, uint64, ids.ID) (map[ids.NodeID]*validators.GetValidatorOutput, error) { - return map[ids.NodeID]*validators.GetValidatorOutput{ - nodeID0: { - NodeID: nodeID0, - PublicKey: pk0, - Weight: math.MaxUint[uint64](), - }, - nodeID1: { - NodeID: nodeID1, - PublicKey: pk1, - Weight: math.MaxUint[uint64](), - }, - }, nil - }, - }, quorumNum: 1, quorumDen: 2, wantAggregateSignaturesErr: math.ErrOverflow, }, - { - name: "fails attestation", - handler: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer, - ), - ctx: context.Background(), - validators: []Validator{ - { - NodeID: nodeID0, - PublicKey: pk0, - Weight: 1, - }, - }, - wantAggregateSignaturesErr: ErrFailedAggregation, - quorumNum: 1, - quorumDen: 1, - }, { name: "context canceled", handler: NewHandler(&testVerifier{}, signer), @@ -185,9 +136,8 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - wantAggregateSignaturesErr: ErrFailedAggregation, - quorumNum: 1, - quorumDen: 1, + quorumNum: 0, + quorumDen: 1, }, } @@ -202,7 +152,7 @@ func TestVerifier_Verify(t *testing.T) { client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) aggregator := NewSignatureAggregator(logging.NoLog{}, client) - gotMsg, _, gotDen, err := aggregator.AggregateSignatures( + _, _, gotDen, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -222,17 +172,6 @@ func TestVerifier_Verify(t *testing.T) { } require.Equal(wantDen, gotDen) - - err = gotMsg.Signature.Verify( - context.Background(), - &gotMsg.UnsignedMessage, - networkID, - tt.pChainState, - 0, - tt.quorumNum, - tt.quorumDen, - ) - require.ErrorIs(err, tt.wantVerifyErr) }) } } From 4868a99e9e2215ca7badf47864903402fecd5902 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:47:21 -0500 Subject: [PATCH 21/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 6 +----- network/p2p/acp118/aggregator_test.go | 29 ++++++--------------------- 2 files changed, 7 insertions(+), 28 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 781ff2d2fd01..4e59bbe149ce 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -16,7 +16,6 @@ import ( "github.com/ava-labs/avalanchego/proto/pb/sdk" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/logging" - "github.com/ava-labs/avalanchego/utils/math" "github.com/ava-labs/avalanchego/utils/set" "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) @@ -97,10 +96,7 @@ func (s *SignatureAggregator) AggregateSignatures( aggregatedStakeWeight := uint64(0) totalStakeWeight := uint64(0) for i, v := range validators { - totalStakeWeight, err = math.Add[uint64](totalStakeWeight, v.Weight) - if err != nil { - return nil, 0, 0, err - } + totalStakeWeight += v.Weight // Only try to aggregate signatures from validators that are not already in // the signer bit set diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 521468d7a51a..873e057c733a 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,7 +7,6 @@ import ( "context" "testing" - "github.com/ava-labs/avalanchego/utils/math" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/ids" @@ -41,6 +40,8 @@ func TestVerifier_Verify(t *testing.T) { validators []Validator quorumNum uint64 quorumDen uint64 + wantMsg *warp.Message + wantSigners []int wantAggregateSignaturesErr error }{ { @@ -54,8 +55,9 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - quorumNum: 1, - quorumDen: 1, + wantSigners: []int{0}, + quorumNum: 1, + quorumDen: 1, }, { name: "fails aggregation from some validators", @@ -80,6 +82,7 @@ func TestVerifier_Verify(t *testing.T) { }, quorumNum: 1, quorumDen: 1, + wantSigners: []int{1}, wantAggregateSignaturesErr: ErrFailedAggregation, }, { @@ -100,26 +103,6 @@ func TestVerifier_Verify(t *testing.T) { quorumNum: 1, quorumDen: 1, }, - { - name: "stake overflow", - handler: NewHandler(&testVerifier{}, signer), - ctx: context.Background(), - validators: []Validator{ - { - NodeID: nodeID0, - PublicKey: pk0, - Weight: math.MaxUint[uint64](), - }, - { - NodeID: nodeID1, - PublicKey: pk1, - Weight: math.MaxUint[uint64](), - }, - }, - quorumNum: 1, - quorumDen: 2, - wantAggregateSignaturesErr: math.ErrOverflow, - }, { name: "context canceled", handler: NewHandler(&testVerifier{}, signer), From c5aaebbc5406f11b48a4f7c5ad6d2f5dde495446 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:52:15 -0500 Subject: [PATCH 22/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 873e057c733a..bf28f8ffb372 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + "github.com/ava-labs/avalanchego/utils/set" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/ids" @@ -135,7 +136,7 @@ func TestVerifier_Verify(t *testing.T) { client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) aggregator := NewSignatureAggregator(logging.NoLog{}, client) - _, _, gotDen, err := aggregator.AggregateSignatures( + msg, gotNum, gotDen, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -149,11 +150,22 @@ func TestVerifier_Verify(t *testing.T) { return } + bitSetSignature := msg.Signature.(*warp.BitSetSignature) + require.Len(bitSetSignature.Signers, len(tt.wantSigners)) + + wantNum := uint64(0) + bitSet := set.BitsFromBytes(bitSetSignature.Signers) + for _, i := range tt.wantSigners { + require.True(bitSet.Contains(i)) + wantNum += tt.validators[i].Weight + } + wantDen := uint64(0) for _, v := range tt.validators { wantDen += v.Weight } + require.Equal(wantNum, gotNum) require.Equal(wantDen, gotDen) }) } From fecd189c4075f1b344f2038748c161bd11336d6f Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:55:21 -0500 Subject: [PATCH 23/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 4e59bbe149ce..57f9638f5525 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -24,6 +24,8 @@ import ( // generate a signature var ErrFailedAggregation = errors.New("failed aggregation") +// Validator signs warp messages. NodeID must be unique across validators, but +// PublicKey is not guaranteed to be unique. type Validator struct { NodeID ids.NodeID PublicKey *bls.PublicKey @@ -214,35 +216,35 @@ type responseHandler struct { results chan result } -func (a *responseHandler) HandleResponse( +func (r *responseHandler) HandleResponse( _ context.Context, nodeID ids.NodeID, responseBytes []byte, err error, ) { - validator := a.nodeIDsToValidator[nodeID] + validator := r.nodeIDsToValidator[nodeID] if err != nil { - a.results <- result{Validator: validator, Err: err} + r.results <- result{Validator: validator, Err: err} return } response := &sdk.SignatureResponse{} if err := proto.Unmarshal(responseBytes, response); err != nil { - a.results <- result{Validator: validator, Err: err} + r.results <- result{Validator: validator, Err: err} return } signature, err := bls.SignatureFromBytes(response.Signature) if err != nil { - a.results <- result{Validator: validator, Err: err} + r.results <- result{Validator: validator, Err: err} return } - if !bls.Verify(validator.PublicKey, signature, a.message.UnsignedMessage.Bytes()) { - a.results <- result{Validator: validator, Err: err} + if !bls.Verify(validator.PublicKey, signature, r.message.UnsignedMessage.Bytes()) { + r.results <- result{Validator: validator, Err: err} return } - a.results <- result{Validator: validator, Signature: signature} + r.results <- result{Validator: validator, Signature: signature} } From 3b23a2246f416a6ce321470252ff7d847339c67a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:55:45 -0500 Subject: [PATCH 24/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 57f9638f5525..9a028f54e149 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -181,7 +181,7 @@ func (s *SignatureAggregator) AggregateSignatures( aggregatedStakeWeight += result.Validator.Weight if aggregatedStakeWeight >= minThreshold { - msg, err := s.newWarpMessage(message, signerBitSet, signatures) + msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { return nil, 0, 0, err } @@ -192,7 +192,7 @@ func (s *SignatureAggregator) AggregateSignatures( } } -func (s *SignatureAggregator) newWarpMessage( +func newWarpMessage( message *warp.Message, signerBitSet set.Bits, signatures []*bls.Signature) (*warp.Message, error) { From 420253d5dcd4032efe4200b1be97f41f8cea5b2c Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:56:05 -0500 Subject: [PATCH 25/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 9a028f54e149..fa2d7aea2381 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -195,7 +195,8 @@ func (s *SignatureAggregator) AggregateSignatures( func newWarpMessage( message *warp.Message, signerBitSet set.Bits, - signatures []*bls.Signature) (*warp.Message, error) { + signatures []*bls.Signature, +) (*warp.Message, error) { aggregateSignature, err := bls.AggregateSignatures(signatures) if err != nil { return nil, err From 4c4c3806e6ec7ccf67617421e6885b2f302ac515 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 16:56:29 -0500 Subject: [PATCH 26/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index fa2d7aea2381..a639ba05c9ab 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -152,7 +152,7 @@ func (s *SignatureAggregator) AggregateSignatures( } // Try to return whatever progress we have if the context is cancelled - msg, err := s.newWarpMessage(message, signerBitSet, signatures) + msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { return nil, 0, 0, err } From 738b0ce070b0487354bfa11eca67cc4e734706a2 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 19 Nov 2024 17:06:14 -0500 Subject: [PATCH 27/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 42 +++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index bf28f8ffb372..44b58dadff08 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -30,6 +30,11 @@ func TestVerifier_Verify(t *testing.T) { require.NoError(t, err) pk1 := bls.PublicFromSecretKey(sk1) + nodeID2 := ids.GenerateTestNodeID() + sk2, err := bls.NewSecretKey() + require.NoError(t, err) + pk2 := bls.PublicFromSecretKey(sk2) + networkID := uint32(123) chainID := ids.GenerateTestID() signer := warp.NewSigner(sk0, networkID, chainID) @@ -61,10 +66,36 @@ func TestVerifier_Verify(t *testing.T) { quorumDen: 1, }, { - name: "fails aggregation from some validators", + name: "fails aggregation from some validators - 1/2", + handler: NewHandler( + &testVerifier{ + Errs: []*common.AppError{nil, common.ErrUndefined}, + }, + signer, + ), + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 1, + }, + }, + quorumNum: 1, + quorumDen: 1, + wantSigners: []int{0}, + wantAggregateSignaturesErr: ErrFailedAggregation, + }, + { + name: "fails aggregation from some validators - 2/3", handler: NewHandler( &testVerifier{ - Errs: []*common.AppError{common.ErrUndefined}, + Errs: []*common.AppError{nil, nil, common.ErrUndefined}, }, signer, ), @@ -80,10 +111,15 @@ func TestVerifier_Verify(t *testing.T) { PublicKey: pk1, Weight: 1, }, + { + NodeID: nodeID2, + PublicKey: pk2, + Weight: 1, + }, }, quorumNum: 1, quorumDen: 1, - wantSigners: []int{1}, + wantSigners: []int{0, 1}, wantAggregateSignaturesErr: ErrFailedAggregation, }, { From 455e03e89171b6282511d6eba14e23d4b5c81f8d Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 00:34:35 -0500 Subject: [PATCH 28/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 69 ++++++++++++++++----------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 44b58dadff08..bd3dd36a5947 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -19,7 +19,7 @@ import ( "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) -func TestVerifier_Verify(t *testing.T) { +func TestSignatureAggregator_AggregateSignatures(t *testing.T) { nodeID0 := ids.GenerateTestNodeID() sk0, err := bls.NewSecretKey() require.NoError(t, err) @@ -40,15 +40,15 @@ func TestVerifier_Verify(t *testing.T) { signer := warp.NewSigner(sk0, networkID, chainID) tests := []struct { - name string - handler p2p.Handler - ctx context.Context - validators []Validator - quorumNum uint64 - quorumDen uint64 - wantMsg *warp.Message - wantSigners []int - wantAggregateSignaturesErr error + name string + handler p2p.Handler + ctx context.Context + validators []Validator + quorumNum uint64 + quorumDen uint64 + wantMsg *warp.Message + wantSigners []int + wantErr error }{ { name: "aggregates from all validators", @@ -86,10 +86,10 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0}, - wantAggregateSignaturesErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, + wantSigners: []int{0}, + wantErr: ErrFailedAggregation, }, { name: "fails aggregation from some validators - 2/3", @@ -117,10 +117,10 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0, 1}, - wantAggregateSignaturesErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, + wantSigners: []int{0, 1}, + wantErr: ErrFailedAggregation, }, { name: "fails aggregation from all validators", @@ -136,9 +136,9 @@ func TestVerifier_Verify(t *testing.T) { Weight: 1, }, }, - wantAggregateSignaturesErr: ErrFailedAggregation, - quorumNum: 1, - quorumDen: 1, + wantErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, }, { name: "context canceled", @@ -165,13 +165,26 @@ func TestVerifier_Verify(t *testing.T) { t.Run(tt.name, func(t *testing.T) { require := require.New(t) - unsignedMsg, err := warp.NewUnsignedMessage(networkID, chainID, []byte("payload")) - require.NoError(err) - msg, err := warp.NewMessage(unsignedMsg, &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}) - require.NoError(err) - client := p2ptest.NewClient(t, context.Background(), tt.handler, ids.GenerateTestNodeID(), nodeID0) + client := p2ptest.NewClient( + t, + context.Background(), + tt.handler, + ids.EmptyNodeID, + tt.validators[0].NodeID, + ) aggregator := NewSignatureAggregator(logging.NoLog{}, client) + unsignedMsg, err := warp.NewUnsignedMessage( + networkID, + chainID, + []byte("payload"), + ) + require.NoError(err) + msg, err := warp.NewMessage( + unsignedMsg, + &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, + ) + require.NoError(err) msg, gotNum, gotDen, err := aggregator.AggregateSignatures( tt.ctx, msg, @@ -180,9 +193,9 @@ func TestVerifier_Verify(t *testing.T) { tt.quorumNum, tt.quorumDen, ) - require.ErrorIs(err, tt.wantAggregateSignaturesErr) + require.ErrorIs(err, tt.wantErr) - if tt.wantAggregateSignaturesErr != nil { + if tt.wantErr != nil { return } From 7d7c3c0aac11b82c0c1fac694e80968e69be4b77 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:04:18 -0500 Subject: [PATCH 29/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 7 +- network/p2p/acp118/aggregator_test.go | 137 +++++++++++++++++++------- network/p2p/acp118/handler_test.go | 2 +- network/p2p/p2ptest/client.go | 110 ++++++++++++++------- network/p2p/p2ptest/client_test.go | 8 +- 5 files changed, 189 insertions(+), 75 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index a639ba05c9ab..14a499999055 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -22,7 +22,10 @@ import ( // ErrFailedAggregation is returned if it's not possible for us to // generate a signature -var ErrFailedAggregation = errors.New("failed aggregation") +var ( + ErrFailedAggregation = errors.New("failed aggregation") + errFailedVerification = errors.New("failed verification") +) // Validator signs warp messages. NodeID must be unique across validators, but // PublicKey is not guaranteed to be unique. @@ -243,7 +246,7 @@ func (r *responseHandler) HandleResponse( } if !bls.Verify(validator.PublicKey, signature, r.message.UnsignedMessage.Bytes()) { - r.results <- result{Validator: validator, Err: err} + r.results <- result{Validator: validator, Err: errFailedVerification} return } diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index bd3dd36a5947..526e8db19cff 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -20,28 +20,30 @@ import ( ) func TestSignatureAggregator_AggregateSignatures(t *testing.T) { + networkID := uint32(123) + chainID := ids.GenerateTestID() + nodeID0 := ids.GenerateTestNodeID() sk0, err := bls.NewSecretKey() require.NoError(t, err) pk0 := bls.PublicFromSecretKey(sk0) + signer0 := warp.NewSigner(sk0, networkID, chainID) nodeID1 := ids.GenerateTestNodeID() sk1, err := bls.NewSecretKey() require.NoError(t, err) pk1 := bls.PublicFromSecretKey(sk1) + signer1 := warp.NewSigner(sk1, networkID, chainID) nodeID2 := ids.GenerateTestNodeID() sk2, err := bls.NewSecretKey() require.NoError(t, err) pk2 := bls.PublicFromSecretKey(sk2) - - networkID := uint32(123) - chainID := ids.GenerateTestID() - signer := warp.NewSigner(sk0, networkID, chainID) + signer2 := warp.NewSigner(sk2, networkID, chainID) tests := []struct { name string - handler p2p.Handler + peers map[ids.NodeID]p2p.Handler ctx context.Context validators []Validator quorumNum uint64 @@ -51,9 +53,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { wantErr error }{ { - name: "aggregates from all validators", - handler: NewHandler(&testVerifier{}, signer), - ctx: context.Background(), + name: "aggregates from all validators 1/1", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + }, + ctx: context.Background(), validators: []Validator{ { NodeID: nodeID0, @@ -66,13 +70,75 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 1, }, { - name: "fails aggregation from some validators - 1/2", - handler: NewHandler( - &testVerifier{ - Errs: []*common.AppError{nil, common.ErrUndefined}, + name: "aggregates from all validators - 3/3", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer2), + }, + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 2, + }, + { + NodeID: nodeID2, + PublicKey: pk2, + Weight: 3, }, - signer, - ), + }, + wantSigners: []int{0, 1, 2}, + quorumNum: 1, + quorumDen: 1, + }, + { + name: "aggregates from some validators - 2/3", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer2, + ), + }, + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 2, + }, + { + NodeID: nodeID2, + PublicKey: pk2, + Weight: 3, + }, + }, + wantSigners: []int{0, 1}, + quorumNum: 3, + quorumDen: 6, + }, + { + name: "fails aggregation from some validators - 1/2", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer1, + ), + }, ctx: context.Background(), validators: []Validator{ { @@ -93,12 +159,14 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { name: "fails aggregation from some validators - 2/3", - handler: NewHandler( - &testVerifier{ - Errs: []*common.AppError{nil, nil, common.ErrUndefined}, - }, - signer, - ), + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer2, + ), + }, ctx: context.Background(), validators: []Validator{ { @@ -124,10 +192,12 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { name: "fails aggregation from all validators", - handler: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer, - ), + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer0, + ), + }, ctx: context.Background(), validators: []Validator{ { @@ -141,8 +211,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 1, }, { - name: "context canceled", - handler: NewHandler(&testVerifier{}, signer), + name: "context canceled", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + }, ctx: func() context.Context { ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -165,12 +237,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { t.Run(tt.name, func(t *testing.T) { require := require.New(t) - client := p2ptest.NewClient( + client := p2ptest.NewClientWithPeers( t, context.Background(), - tt.handler, ids.EmptyNodeID, - tt.validators[0].NodeID, + tt.peers, ) aggregator := NewSignatureAggregator(logging.NoLog{}, client) @@ -185,7 +256,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, ) require.NoError(err) - msg, gotNum, gotDen, err := aggregator.AggregateSignatures( + gotMsg, gotNum, gotDen, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -199,11 +270,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { return } - bitSetSignature := msg.Signature.(*warp.BitSetSignature) - require.Len(bitSetSignature.Signers, len(tt.wantSigners)) + bitSetSignature := gotMsg.Signature.(*warp.BitSetSignature) + bitSet := set.BitsFromBytes(bitSetSignature.Signers) + require.Equal(len(tt.wantSigners), bitSet.Len()) wantNum := uint64(0) - bitSet := set.BitsFromBytes(bitSetSignature.Signers) for _, i := range tt.wantSigners { require.True(bitSet.Contains(i)) wantNum += tt.validators[i].Weight diff --git a/network/p2p/acp118/handler_test.go b/network/p2p/acp118/handler_test.go index e58d61a8f6a0..372156eaf866 100644 --- a/network/p2p/acp118/handler_test.go +++ b/network/p2p/acp118/handler_test.go @@ -134,7 +134,7 @@ func TestHandler(t *testing.T) { } for _, expectedErr = range tt.expectedErrs { - require.NoError(c.AppRequest(ctx, set.Of(clientNodeID), requestBytes, onResponse)) + require.NoError(c.AppRequest(ctx, set.Of(serverNodeID), requestBytes, onResponse)) <-handled } }) diff --git a/network/p2p/p2ptest/client.go b/network/p2p/p2ptest/client.go index b75654028666..89d1ac48a2b3 100644 --- a/network/p2p/p2ptest/client.go +++ b/network/p2p/p2ptest/client.go @@ -5,6 +5,7 @@ package p2ptest import ( "context" + "fmt" "testing" "time" @@ -28,63 +29,100 @@ func NewClient( clientNodeID ids.NodeID, serverNodeID ids.NodeID, ) *p2p.Client { - clientSender := &enginetest.Sender{} - serverSender := &enginetest.Sender{} + return NewClientWithPeers( + t, + ctx, + clientNodeID, + map[ids.NodeID]p2p.Handler{serverNodeID: handler}, + ) +} +// NewClientWithPeers generates a client to communicate to a set of peers +func NewClientWithPeers( + t *testing.T, + ctx context.Context, + clientNodeID ids.NodeID, + peers map[ids.NodeID]p2p.Handler, +) *p2p.Client { + clientSender := &enginetest.Sender{} clientNetwork, err := p2p.NewNetwork(logging.NoLog{}, clientSender, prometheus.NewRegistry(), "") require.NoError(t, err) - serverNetwork, err := p2p.NewNetwork(logging.NoLog{}, serverSender, prometheus.NewRegistry(), "") - require.NoError(t, err) + peerSenders := make(map[ids.NodeID]*enginetest.Sender) + peerNetworks := make(map[ids.NodeID]*p2p.Network) + for nodeID := range peers { + peerSenders[nodeID] = &enginetest.Sender{} + serverNetwork, err := p2p.NewNetwork(logging.NoLog{}, peerSenders[nodeID], prometheus.NewRegistry(), "") + require.NoError(t, err) + peerNetworks[nodeID] = serverNetwork + } clientSender.SendAppGossipF = func(ctx context.Context, _ common.SendConfig, gossipBytes []byte) error { // Send the request asynchronously to avoid deadlock when the server // sends the response back to the client - go func() { - require.NoError(t, serverNetwork.AppGossip(ctx, clientNodeID, gossipBytes)) - }() + for nodeID := range peers { + nodeIDCopy := nodeID + go func() { + require.NoError(t, peerNetworks[nodeIDCopy].AppGossip(ctx, nodeIDCopy, gossipBytes)) + }() + } return nil } - clientSender.SendAppRequestF = func(ctx context.Context, _ set.Set[ids.NodeID], requestID uint32, requestBytes []byte) error { - // Send the request asynchronously to avoid deadlock when the server - // sends the response back to the client - go func() { - require.NoError(t, serverNetwork.AppRequest(ctx, clientNodeID, requestID, time.Time{}, requestBytes)) - }() + clientSender.SendAppRequestF = func(ctx context.Context, nodeIDs set.Set[ids.NodeID], requestID uint32, requestBytes []byte) error { + for nodeID := range nodeIDs { + network, ok := peerNetworks[nodeID] + if !ok { + return fmt.Errorf("%s is not connected", nodeID) + } - return nil - } - - serverSender.SendAppResponseF = func(ctx context.Context, _ ids.NodeID, requestID uint32, responseBytes []byte) error { - // Send the request asynchronously to avoid deadlock when the server - // sends the response back to the client - go func() { - require.NoError(t, clientNetwork.AppResponse(ctx, serverNodeID, requestID, responseBytes)) - }() + // Send the request asynchronously to avoid deadlock when the server + // sends the response back to the client + go func() { + require.NoError(t, network.AppRequest(ctx, clientNodeID, requestID, time.Time{}, requestBytes)) + }() + } return nil } - serverSender.SendAppErrorF = func(ctx context.Context, _ ids.NodeID, requestID uint32, errorCode int32, errorMessage string) error { - // Send the request asynchronously to avoid deadlock when the server - // sends the response back to the client - go func() { - require.NoError(t, clientNetwork.AppRequestFailed(ctx, serverNodeID, requestID, &common.AppError{ - Code: errorCode, - Message: errorMessage, - })) - }() + for nodeID := range peers { + peerSenders[nodeID].SendAppResponseF = func(ctx context.Context, _ ids.NodeID, requestID uint32, responseBytes []byte) error { + // Send the request asynchronously to avoid deadlock when the server + // sends the response back to the client + nodeIDCopy := nodeID + go func() { + require.NoError(t, clientNetwork.AppResponse(ctx, nodeIDCopy, requestID, responseBytes)) + }() + + return nil + } + } - return nil + for nodeID := range peers { + peerSenders[nodeID].SendAppErrorF = func(ctx context.Context, _ ids.NodeID, requestID uint32, errorCode int32, errorMessage string) error { + // Send the request asynchronously to avoid deadlock when the server + // sends the response back to the client + nodeIDCopy := nodeID + go func() { + require.NoError(t, clientNetwork.AppRequestFailed(ctx, nodeIDCopy, requestID, &common.AppError{ + Code: errorCode, + Message: errorMessage, + })) + }() + + return nil + } } require.NoError(t, clientNetwork.Connected(ctx, clientNodeID, nil)) - require.NoError(t, clientNetwork.Connected(ctx, serverNodeID, nil)) - require.NoError(t, serverNetwork.Connected(ctx, clientNodeID, nil)) - require.NoError(t, serverNetwork.Connected(ctx, serverNodeID, nil)) + for nodeID := range peers { + require.NoError(t, clientNetwork.Connected(ctx, nodeID, nil)) + require.NoError(t, peerNetworks[nodeID].Connected(ctx, clientNodeID, nil)) + require.NoError(t, peerNetworks[nodeID].Connected(ctx, nodeID, nil)) + require.NoError(t, peerNetworks[nodeID].AddHandler(0, peers[nodeID])) + } - require.NoError(t, serverNetwork.AddHandler(0, handler)) return clientNetwork.NewClient(0) } diff --git a/network/p2p/p2ptest/client_test.go b/network/p2p/p2ptest/client_test.go index 45ae970ecf0f..8a163aee9cf5 100644 --- a/network/p2p/p2ptest/client_test.go +++ b/network/p2p/p2ptest/client_test.go @@ -33,6 +33,8 @@ func TestNewClient_AppGossip(t *testing.T) { } func TestNewClient_AppRequest(t *testing.T) { + nodeID := ids.GenerateTestNodeID() + tests := []struct { name string appResponse []byte @@ -43,7 +45,7 @@ func TestNewClient_AppRequest(t *testing.T) { name: "AppRequest - response", appResponse: []byte("foobar"), appRequestF: func(ctx context.Context, client *p2p.Client, onResponse p2p.AppResponseCallback) error { - return client.AppRequest(ctx, set.Of(ids.GenerateTestNodeID()), []byte("foo"), onResponse) + return client.AppRequest(ctx, set.Of(nodeID), []byte("foo"), onResponse) }, }, { @@ -53,7 +55,7 @@ func TestNewClient_AppRequest(t *testing.T) { Message: "foobar", }, appRequestF: func(ctx context.Context, client *p2p.Client, onResponse p2p.AppResponseCallback) error { - return client.AppRequest(ctx, set.Of(ids.GenerateTestNodeID()), []byte("foo"), onResponse) + return client.AppRequest(ctx, set.Of(nodeID), []byte("foo"), onResponse) }, }, { @@ -94,7 +96,7 @@ func TestNewClient_AppRequest(t *testing.T) { }, } - client := NewClient(t, ctx, testHandler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + client := NewClient(t, ctx, testHandler, ids.GenerateTestNodeID(), nodeID) require.NoError(tt.appRequestF( ctx, client, From f989cf5b92a6e4a6ea793430fc32b9d752f84020 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:07:18 -0500 Subject: [PATCH 30/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 36 +++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 526e8db19cff..b34f8d3d9c62 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -157,6 +157,42 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { wantSigners: []int{0}, wantErr: ErrFailedAggregation, }, + { + name: "fails aggregation from some validators - 1/3", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer1, + ), + nodeID2: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer2, + ), + }, + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 1, + }, + { + NodeID: nodeID2, + PublicKey: pk2, + Weight: 1, + }, + }, + quorumNum: 2, + quorumDen: 3, + wantSigners: []int{0}, + wantErr: ErrFailedAggregation, + }, { name: "fails aggregation from some validators - 2/3", peers: map[ids.NodeID]p2p.Handler{ From 900279dab4fe3108e7d0cc16ee540f374931dc6a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:08:19 -0500 Subject: [PATCH 31/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 49 +++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index b34f8d3d9c62..5ffc49f9a1a8 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -70,11 +70,17 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 1, }, { - name: "aggregates from all validators - 3/3", + name: "aggregates from some validators - 1/3", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), - nodeID1: NewHandler(&testVerifier{}, signer1), - nodeID2: NewHandler(&testVerifier{}, signer2), + nodeID1: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer1, + ), + nodeID2: NewHandler( + &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, + signer2, + ), }, ctx: context.Background(), validators: []Validator{ @@ -86,17 +92,17 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { { NodeID: nodeID1, PublicKey: pk1, - Weight: 2, + Weight: 1, }, { NodeID: nodeID2, PublicKey: pk2, - Weight: 3, + Weight: 1, }, }, - wantSigners: []int{0, 1, 2}, + wantSigners: []int{0}, quorumNum: 1, - quorumDen: 1, + quorumDen: 3, }, { name: "aggregates from some validators - 2/3", @@ -130,6 +136,35 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumNum: 3, quorumDen: 6, }, + { + name: "aggregates from all validators - 3/3", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer2), + }, + ctx: context.Background(), + validators: []Validator{ + { + NodeID: nodeID0, + PublicKey: pk0, + Weight: 1, + }, + { + NodeID: nodeID1, + PublicKey: pk1, + Weight: 2, + }, + { + NodeID: nodeID2, + PublicKey: pk2, + Weight: 3, + }, + }, + wantSigners: []int{0, 1, 2}, + quorumNum: 1, + quorumDen: 1, + }, { name: "fails aggregation from some validators - 1/2", peers: map[ids.NodeID]p2p.Handler{ From de7b65c9165145f832d96dcbc73f79ad8eb6fd3b Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:09:24 -0500 Subject: [PATCH 32/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 5ffc49f9a1a8..d5eb7e6ac84d 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -7,7 +7,6 @@ import ( "context" "testing" - "github.com/ava-labs/avalanchego/utils/set" "github.com/stretchr/testify/require" "github.com/ava-labs/avalanchego/ids" @@ -16,6 +15,7 @@ import ( "github.com/ava-labs/avalanchego/snow/engine/common" "github.com/ava-labs/avalanchego/utils/crypto/bls" "github.com/ava-labs/avalanchego/utils/logging" + "github.com/ava-labs/avalanchego/utils/set" "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) From 7c4fa96bc3026d3665e5663dc4a99df6f35210d0 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:32:10 -0500 Subject: [PATCH 33/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- x/sync/sync_test.go | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/x/sync/sync_test.go b/x/sync/sync_test.go index 8f065d838e5f..b6223cd79463 100644 --- a/x/sync/sync_test.go +++ b/x/sync/sync_test.go @@ -177,8 +177,8 @@ func Test_Sync_FindNextKey_InSync(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -376,8 +376,8 @@ func Test_Sync_FindNextKey_ExtraValues(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -777,7 +777,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = append(response.KeyValues, merkledb.KeyValue{}) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -787,7 +787,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = response.KeyValues[min(1, len(response.KeyValues)):] }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -813,7 +813,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { } }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -824,7 +824,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { _ = slices.Delete(response.KeyValues, i, min(len(response.KeyValues), i+1)) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -835,7 +835,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -845,7 +845,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -857,7 +857,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -866,7 +866,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { return p2ptest.NewClient(t, context.Background(), &flakyHandler{ Handler: NewGetRangeProofHandler(logging.NoLog{}, db), c: &counter{m: 2}, - }, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + }, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -876,7 +876,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyChanges = append(response.KeyChanges, make([]merkledb.KeyChange, defaultRequestKeyLimit)...) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -886,7 +886,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyChanges = response.KeyChanges[min(1, len(response.KeyChanges)):] }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -897,7 +897,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { _ = slices.Delete(response.KeyChanges, i, min(len(response.KeyChanges), i+1)) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -908,7 +908,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) }, }, { @@ -917,7 +917,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { return p2ptest.NewClient(t, context.Background(), &flakyHandler{ Handler: NewGetChangeProofHandler(logging.NoLog{}, db), c: &counter{m: 2}, - }, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + }, ids.EmptyNodeID, ids.EmptyNodeID) }, }, } @@ -946,13 +946,13 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { ) rangeProofHandler := NewGetRangeProofHandler(logging.NoLog{}, dbToSync) - rangeProofClient = p2ptest.NewClient(t, ctx, rangeProofHandler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + rangeProofClient = p2ptest.NewClient(t, ctx, rangeProofHandler, ids.EmptyNodeID, ids.EmptyNodeID) if tt.rangeProofClient != nil { rangeProofClient = tt.rangeProofClient(dbToSync) } changeProofHandler := NewGetChangeProofHandler(logging.NoLog{}, dbToSync) - changeProofClient = p2ptest.NewClient(t, ctx, changeProofHandler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + changeProofClient = p2ptest.NewClient(t, ctx, changeProofHandler, ids.EmptyNodeID, ids.EmptyNodeID) if tt.changeProofClient != nil { changeProofClient = tt.changeProofClient(dbToSync) } @@ -1031,8 +1031,8 @@ func Test_Sync_Result_Correct_Root_With_Sync_Restart(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, From 9802ab224eb8ecdb6924dc71bd7e3e7f341bbbde Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:33:23 -0500 Subject: [PATCH 34/62] fix tests Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- x/sync/sync_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x/sync/sync_test.go b/x/sync/sync_test.go index b6223cd79463..9cb314600b4f 100644 --- a/x/sync/sync_test.go +++ b/x/sync/sync_test.go @@ -507,8 +507,8 @@ func Test_Sync_FindNextKey_DifferentChild(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -1058,8 +1058,8 @@ func Test_Sync_Result_Correct_Root_With_Sync_Restart(t *testing.T) { newSyncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, From 8b6563a447a1a0cc26590068544eff0185216add Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:41:43 -0500 Subject: [PATCH 35/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/network/p2p/p2ptest/client_test.go b/network/p2p/p2ptest/client_test.go index 8a163aee9cf5..179067bfa7df 100644 --- a/network/p2p/p2ptest/client_test.go +++ b/network/p2p/p2ptest/client_test.go @@ -33,8 +33,6 @@ func TestNewClient_AppGossip(t *testing.T) { } func TestNewClient_AppRequest(t *testing.T) { - nodeID := ids.GenerateTestNodeID() - tests := []struct { name string appResponse []byte @@ -45,7 +43,7 @@ func TestNewClient_AppRequest(t *testing.T) { name: "AppRequest - response", appResponse: []byte("foobar"), appRequestF: func(ctx context.Context, client *p2p.Client, onResponse p2p.AppResponseCallback) error { - return client.AppRequest(ctx, set.Of(nodeID), []byte("foo"), onResponse) + return client.AppRequest(ctx, set.Of(ids.EmptyNodeID), []byte("foo"), onResponse) }, }, { @@ -55,7 +53,7 @@ func TestNewClient_AppRequest(t *testing.T) { Message: "foobar", }, appRequestF: func(ctx context.Context, client *p2p.Client, onResponse p2p.AppResponseCallback) error { - return client.AppRequest(ctx, set.Of(nodeID), []byte("foo"), onResponse) + return client.AppRequest(ctx, set.Of(ids.EmptyNodeID), []byte("foo"), onResponse) }, }, { @@ -96,7 +94,7 @@ func TestNewClient_AppRequest(t *testing.T) { }, } - client := NewClient(t, ctx, testHandler, ids.GenerateTestNodeID(), nodeID) + client := NewClient(t, ctx, testHandler, ids.EmptyNodeID, ids.EmptyNodeID) require.NoError(tt.appRequestF( ctx, client, From 4480b48c46193a932b8161678180534f503c3eb6 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:22:56 -0500 Subject: [PATCH 36/62] Update network/p2p/acp118/aggregator.go Co-authored-by: Stephen Buttolph Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 14a499999055..c57f89c6dd96 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -90,12 +90,7 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, 0, 0, errors.New("invalid warp signature type") } - var signerBitSet set.Bits - if bitSetSignature.Signers != nil { - signerBitSet = set.BitsFromBytes(bitSetSignature.Signers) - } else { - signerBitSet = set.NewBits() - } + signerBitSet := set.BitsFromBytes(bitSetSignature.Signers) sampleable := make([]ids.NodeID, 0, len(validators)) aggregatedStakeWeight := uint64(0) From 67f533ed18ec37f6144faa638f50705677ea2160 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:23:44 -0500 Subject: [PATCH 37/62] Update network/p2p/acp118/aggregator.go Co-authored-by: Stephen Buttolph Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index c57f89c6dd96..b8727ed0dc63 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -129,8 +129,6 @@ func (s *SignatureAggregator) AggregateSignatures( } for _, nodeID := range sampleable { - // Avoid var shadowing in goroutine - nodeIDCopy := nodeID go func() { if err := s.client.AppRequest(ctx, set.Of(nodeIDCopy), requestBytes, job.HandleResponse); err != nil { results <- result{Validator: nodeIDsToValidator[nodeIDCopy], Err: err} From 2a9621bf7e29ad5dc6a546ad3b9bf32971180fba Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:24:03 -0500 Subject: [PATCH 38/62] Update network/p2p/acp118/aggregator.go Co-authored-by: Stephen Buttolph Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 1 - 1 file changed, 1 deletion(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index b8727ed0dc63..8dc4497d9dc6 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -132,7 +132,6 @@ func (s *SignatureAggregator) AggregateSignatures( go func() { if err := s.client.AppRequest(ctx, set.Of(nodeIDCopy), requestBytes, job.HandleResponse); err != nil { results <- result{Validator: nodeIDsToValidator[nodeIDCopy], Err: err} - return } }() } From 3773575506950f3ee19e1408183f172ed9c7c07c Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:24:59 -0500 Subject: [PATCH 39/62] move nil signatures check Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 8dc4497d9dc6..048ce381851d 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -142,10 +142,6 @@ func (s *SignatureAggregator) AggregateSignatures( for { select { case <-ctx.Done(): - if len(signatures) == 0 { - return message, 0, totalStakeWeight, nil - } - // Try to return whatever progress we have if the context is cancelled msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { @@ -192,6 +188,10 @@ func newWarpMessage( signerBitSet set.Bits, signatures []*bls.Signature, ) (*warp.Message, error) { + if len(signatures) == 0 { + return message, nil + } + aggregateSignature, err := bls.AggregateSignatures(signatures) if err != nil { return nil, err From 356c36d932ada24f9921812c4b72425239cb6388 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 26 Nov 2024 12:53:14 -0500 Subject: [PATCH 40/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 048ce381851d..1020d7bdcc4c 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -130,8 +130,8 @@ func (s *SignatureAggregator) AggregateSignatures( for _, nodeID := range sampleable { go func() { - if err := s.client.AppRequest(ctx, set.Of(nodeIDCopy), requestBytes, job.HandleResponse); err != nil { - results <- result{Validator: nodeIDsToValidator[nodeIDCopy], Err: err} + if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, job.HandleResponse); err != nil { + results <- result{Validator: nodeIDsToValidator[nodeID], Err: err} } }() } From c27987665aec3e67072da004e1ad1abb65ba4c5e Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 26 Nov 2024 13:15:00 -0500 Subject: [PATCH 41/62] nti Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 2 +- network/p2p/acp118/aggregator_test.go | 74 +++++++++++++-------------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 1020d7bdcc4c..514a57cdfa10 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -69,7 +69,7 @@ func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.Message, justification []byte, - validators []Validator, + validators []warp.Validator, quorumNum uint64, quorumDen uint64, ) (*warp.Message, uint64, uint64, error) { diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index d5eb7e6ac84d..234d02036a33 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -45,7 +45,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { name string peers map[ids.NodeID]p2p.Handler ctx context.Context - validators []Validator + validators []warp.Validator quorumNum uint64 quorumDen uint64 wantMsg *warp.Message @@ -58,11 +58,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { nodeID0: NewHandler(&testVerifier{}, signer0), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, }, wantSigners: []int{0}, @@ -83,21 +83,21 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, }, { - NodeID: nodeID2, PublicKey: pk2, Weight: 1, + NodeIDs: []ids.NodeID{nodeID2}, }, }, wantSigners: []int{0}, @@ -115,21 +115,21 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 2, + NodeIDs: []ids.NodeID{nodeID1}, }, { - NodeID: nodeID2, PublicKey: pk2, Weight: 3, + NodeIDs: []ids.NodeID{nodeID2}, }, }, wantSigners: []int{0, 1}, @@ -144,21 +144,21 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { nodeID2: NewHandler(&testVerifier{}, signer2), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 2, + NodeIDs: []ids.NodeID{nodeID1}, }, { - NodeID: nodeID2, PublicKey: pk2, Weight: 3, + NodeIDs: []ids.NodeID{nodeID2}, }, }, wantSigners: []int{0, 1, 2}, @@ -175,16 +175,16 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, }, }, quorumNum: 1, @@ -206,21 +206,21 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, }, { - NodeID: nodeID2, PublicKey: pk2, Weight: 1, + NodeIDs: []ids.NodeID{nodeID2}, }, }, quorumNum: 2, @@ -239,21 +239,21 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, { - NodeID: nodeID1, PublicKey: pk1, Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, }, { - NodeID: nodeID2, PublicKey: pk2, Weight: 1, + NodeIDs: []ids.NodeID{nodeID2}, }, }, quorumNum: 1, @@ -270,11 +270,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, }, wantErr: ErrFailedAggregation, @@ -292,11 +292,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { return ctx }(), - validators: []Validator{ + validators: []warp.Validator{ { - NodeID: nodeID0, PublicKey: pk0, Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, }, }, quorumNum: 0, @@ -327,7 +327,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, ) require.NoError(err) - gotMsg, gotNum, gotDen, err := aggregator.AggregateSignatures( + gotMsg, gotAggregatedStake, gotTotalStake, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -345,19 +345,19 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { bitSet := set.BitsFromBytes(bitSetSignature.Signers) require.Equal(len(tt.wantSigners), bitSet.Len()) - wantNum := uint64(0) + wantAggregatedStake := uint64(0) for _, i := range tt.wantSigners { require.True(bitSet.Contains(i)) - wantNum += tt.validators[i].Weight + wantAggregatedStake += tt.validators[i].Weight } - wantDen := uint64(0) + wantTotalStake := uint64(0) for _, v := range tt.validators { - wantDen += v.Weight + wantTotalStake += v.Weight } - require.Equal(wantNum, gotNum) - require.Equal(wantDen, gotDen) + require.Equal(wantAggregatedStake, gotAggregatedStake) + require.Equal(wantTotalStake, gotTotalStake) }) } } From 7bfe878eb775edb32bde4c91dc2b59864e2dd533 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 21 Nov 2024 12:25:31 -0500 Subject: [PATCH 42/62] wip Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 96 ++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 42 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 514a57cdfa10..f0e7359c305a 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -27,20 +27,13 @@ var ( errFailedVerification = errors.New("failed verification") ) -// Validator signs warp messages. NodeID must be unique across validators, but -// PublicKey is not guaranteed to be unique. -type Validator struct { - NodeID ids.NodeID - PublicKey *bls.PublicKey - Weight uint64 -} - type indexedValidator struct { - Validator + warp.Validator Index int } type result struct { + NodeID ids.NodeID Validator indexedValidator Signature *bls.Signature Err error @@ -83,7 +76,8 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, 0, 0, fmt.Errorf("failed to marshal signature request: %w", err) } - nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) + publicKeysToValidators := make(map[*bls.PublicKey]indexedValidator) + nodeIDsToPublicKeys := make(map[ids.NodeID]*bls.PublicKey) // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) if !ok { @@ -92,9 +86,10 @@ func (s *SignatureAggregator) AggregateSignatures( signerBitSet := set.BitsFromBytes(bitSetSignature.Signers) - sampleable := make([]ids.NodeID, 0, len(validators)) + nonSigners := make([]*bls.PublicKey, 0, len(validators)) aggregatedStakeWeight := uint64(0) totalStakeWeight := uint64(0) + numRequests := 0 for i, v := range validators { totalStakeWeight += v.Weight @@ -105,14 +100,20 @@ func (s *SignatureAggregator) AggregateSignatures( continue } - nodeIDsToValidator[v.NodeID] = indexedValidator{ + publicKeysToValidators[v.PublicKey] = indexedValidator{ Index: i, Validator: v, } - sampleable = append(sampleable, v.NodeID) + + for _, nodeID := range v.NodeIDs { + numRequests += 1 + nodeIDsToPublicKeys[nodeID] = v.PublicKey + } + + nonSigners = append(nonSigners, v.PublicKey) } - signatures := make([]*bls.Signature, 0, len(sampleable)+1) + signatures := make([]*bls.Signature, 0, len(nonSigners)+1) if bitSetSignature.Signature != [bls.SignatureLen]byte{} { blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) if err != nil { @@ -121,25 +122,28 @@ func (s *SignatureAggregator) AggregateSignatures( signatures = append(signatures, blsSignature) } - results := make(chan result) + //TODO better than buffered channel? + results := make(chan result, numRequests) job := responseHandler{ - message: message, - nodeIDsToValidator: nodeIDsToValidator, - results: results, + message: message, + publicKeysToValidators: publicKeysToValidators, + nodeIDsToPublicKeys: nodeIDsToPublicKeys, + results: results, } - for _, nodeID := range sampleable { - go func() { + for _, pk := range nonSigners { + validator := publicKeysToValidators[pk] + for _, nodeID := range validator.NodeIDs { if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, job.HandleResponse); err != nil { - results <- result{Validator: nodeIDsToValidator[nodeID], Err: err} + results <- result{NodeID: nodeID, Validator: validator, Err: err} + // TODO fatal } - }() + } } - failedStakeWeight := uint64(0) minThreshold := (totalStakeWeight * quorumNum) / quorumDen - for { + for i := 0; i < numRequests; i++ { select { case <-ctx.Done(): // Try to return whatever progress we have if the context is cancelled @@ -153,17 +157,18 @@ func (s *SignatureAggregator) AggregateSignatures( if result.Err != nil { s.log.Debug( "dropping response", - zap.Stringer("nodeID", result.Validator.NodeID), - zap.Uint64("weight", result.Validator.Weight), + zap.Stringer("nodeID", result.NodeID), zap.Error(err), ) + continue + } - // Fast-fail if it's not possible to generate a signature that meets the - // minimum threshold - failedStakeWeight += result.Validator.Weight - if totalStakeWeight-failedStakeWeight < minThreshold { - return nil, 0, 0, ErrFailedAggregation - } + if signerBitSet.Contains(result.Validator.Index) { + s.log.Debug( + "dropping duplicate signature", + zap.Stringer("nodeID", result.NodeID), + zap.Error(err), + ) continue } @@ -181,6 +186,13 @@ func (s *SignatureAggregator) AggregateSignatures( } } } + + msg, err := newWarpMessage(message, signerBitSet, signatures) + if err != nil { + return nil, 0, 0, err + } + + return msg, aggregatedStakeWeight, totalStakeWeight, nil } func newWarpMessage( @@ -207,9 +219,10 @@ func newWarpMessage( } type responseHandler struct { - message *warp.Message - nodeIDsToValidator map[ids.NodeID]indexedValidator - results chan result + message *warp.Message + publicKeysToValidators map[*bls.PublicKey]indexedValidator + nodeIDsToPublicKeys map[ids.NodeID]*bls.PublicKey + results chan result } func (r *responseHandler) HandleResponse( @@ -218,29 +231,28 @@ func (r *responseHandler) HandleResponse( responseBytes []byte, err error, ) { - validator := r.nodeIDsToValidator[nodeID] - + validator := r.publicKeysToValidators[r.nodeIDsToPublicKeys[nodeID]] if err != nil { - r.results <- result{Validator: validator, Err: err} + r.results <- result{NodeID: nodeID, Validator: validator, Err: err} return } response := &sdk.SignatureResponse{} if err := proto.Unmarshal(responseBytes, response); err != nil { - r.results <- result{Validator: validator, Err: err} + r.results <- result{NodeID: nodeID, Validator: validator, Err: err} return } signature, err := bls.SignatureFromBytes(response.Signature) if err != nil { - r.results <- result{Validator: validator, Err: err} + r.results <- result{NodeID: nodeID, Validator: validator, Err: err} return } if !bls.Verify(validator.PublicKey, signature, r.message.UnsignedMessage.Bytes()) { - r.results <- result{Validator: validator, Err: errFailedVerification} + r.results <- result{NodeID: nodeID, Validator: validator, Err: errFailedVerification} return } - r.results <- result{Validator: validator, Signature: signature} + r.results <- result{NodeID: nodeID, Validator: validator, Signature: signature} } From cd716bc0adc84ef89b667f5afdbcbf7277dcc243 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 27 Nov 2024 02:46:27 -0500 Subject: [PATCH 43/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client.go | 37 +++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/network/p2p/p2ptest/client.go b/network/p2p/p2ptest/client.go index 89d1ac48a2b3..c5ff44c86f64 100644 --- a/network/p2p/p2ptest/client.go +++ b/network/p2p/p2ptest/client.go @@ -20,20 +20,28 @@ import ( "github.com/ava-labs/avalanchego/utils/set" ) +func NewSelfClient(t *testing.T, ctx context.Context, nodeID ids.NodeID, handler p2p.Handler) *p2p.Client { + return NewClient(t, ctx, nodeID, handler, nodeID, handler) +} + // NewClient generates a client-server pair and returns the client used to // communicate with a server with the specified handler func NewClient( t *testing.T, ctx context.Context, - handler p2p.Handler, clientNodeID ids.NodeID, + clientHandler p2p.Handler, serverNodeID ids.NodeID, + serverHandler p2p.Handler, ) *p2p.Client { return NewClientWithPeers( t, ctx, clientNodeID, - map[ids.NodeID]p2p.Handler{serverNodeID: handler}, + clientHandler, + map[ids.NodeID]p2p.Handler{ + serverNodeID: serverHandler, + }, ) } @@ -42,35 +50,33 @@ func NewClientWithPeers( t *testing.T, ctx context.Context, clientNodeID ids.NodeID, + clientHandler p2p.Handler, peers map[ids.NodeID]p2p.Handler, ) *p2p.Client { - clientSender := &enginetest.Sender{} - clientNetwork, err := p2p.NewNetwork(logging.NoLog{}, clientSender, prometheus.NewRegistry(), "") - require.NoError(t, err) + peers[clientNodeID] = clientHandler peerSenders := make(map[ids.NodeID]*enginetest.Sender) peerNetworks := make(map[ids.NodeID]*p2p.Network) for nodeID := range peers { peerSenders[nodeID] = &enginetest.Sender{} - serverNetwork, err := p2p.NewNetwork(logging.NoLog{}, peerSenders[nodeID], prometheus.NewRegistry(), "") + peerNetwork, err := p2p.NewNetwork(logging.NoLog{}, peerSenders[nodeID], prometheus.NewRegistry(), "") require.NoError(t, err) - peerNetworks[nodeID] = serverNetwork + peerNetworks[nodeID] = peerNetwork } - clientSender.SendAppGossipF = func(ctx context.Context, _ common.SendConfig, gossipBytes []byte) error { + peerSenders[clientNodeID].SendAppGossipF = func(ctx context.Context, _ common.SendConfig, gossipBytes []byte) error { // Send the request asynchronously to avoid deadlock when the server // sends the response back to the client for nodeID := range peers { - nodeIDCopy := nodeID go func() { - require.NoError(t, peerNetworks[nodeIDCopy].AppGossip(ctx, nodeIDCopy, gossipBytes)) + require.NoError(t, peerNetworks[nodeID].AppGossip(ctx, nodeID, gossipBytes)) }() } return nil } - clientSender.SendAppRequestF = func(ctx context.Context, nodeIDs set.Set[ids.NodeID], requestID uint32, requestBytes []byte) error { + peerSenders[clientNodeID].SendAppRequestF = func(ctx context.Context, nodeIDs set.Set[ids.NodeID], requestID uint32, requestBytes []byte) error { for nodeID := range nodeIDs { network, ok := peerNetworks[nodeID] if !ok { @@ -91,9 +97,8 @@ func NewClientWithPeers( peerSenders[nodeID].SendAppResponseF = func(ctx context.Context, _ ids.NodeID, requestID uint32, responseBytes []byte) error { // Send the request asynchronously to avoid deadlock when the server // sends the response back to the client - nodeIDCopy := nodeID go func() { - require.NoError(t, clientNetwork.AppResponse(ctx, nodeIDCopy, requestID, responseBytes)) + require.NoError(t, peerNetworks[clientNodeID].AppResponse(ctx, nodeID, requestID, responseBytes)) }() return nil @@ -106,7 +111,7 @@ func NewClientWithPeers( // sends the response back to the client nodeIDCopy := nodeID go func() { - require.NoError(t, clientNetwork.AppRequestFailed(ctx, nodeIDCopy, requestID, &common.AppError{ + require.NoError(t, peerNetworks[clientNodeID].AppRequestFailed(ctx, nodeIDCopy, requestID, &common.AppError{ Code: errorCode, Message: errorMessage, })) @@ -116,13 +121,11 @@ func NewClientWithPeers( } } - require.NoError(t, clientNetwork.Connected(ctx, clientNodeID, nil)) for nodeID := range peers { - require.NoError(t, clientNetwork.Connected(ctx, nodeID, nil)) require.NoError(t, peerNetworks[nodeID].Connected(ctx, clientNodeID, nil)) require.NoError(t, peerNetworks[nodeID].Connected(ctx, nodeID, nil)) require.NoError(t, peerNetworks[nodeID].AddHandler(0, peers[nodeID])) } - return clientNetwork.NewClient(0) + return peerNetworks[clientNodeID].NewClient(0) } From 7752928cccc04dea197e32f9871f434c99398717 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Wed, 27 Nov 2024 02:53:08 -0500 Subject: [PATCH 44/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 78 ++++++++++++----------- network/p2p/acp118/aggregator_test.go | 90 +++++++++++++++------------ network/p2p/acp118/handler_test.go | 4 +- 3 files changed, 95 insertions(+), 77 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index f0e7359c305a..942ed0beb4f0 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "math/big" "go.uber.org/zap" "google.golang.org/protobuf/proto" @@ -20,23 +21,19 @@ import ( "github.com/ava-labs/avalanchego/vms/platformvm/warp" ) -// ErrFailedAggregation is returned if it's not possible for us to -// generate a signature -var ( - ErrFailedAggregation = errors.New("failed aggregation") - errFailedVerification = errors.New("failed verification") -) +var errFailedVerification = errors.New("failed verification") type indexedValidator struct { - warp.Validator + *warp.Validator Index int } type result struct { - NodeID ids.NodeID - Validator indexedValidator - Signature *bls.Signature - Err error + NodeID ids.NodeID + Validator indexedValidator + Signature *bls.Signature + Err error + ShouldClose bool } // NewSignatureAggregator returns an instance of SignatureAggregator @@ -62,10 +59,16 @@ func (s *SignatureAggregator) AggregateSignatures( ctx context.Context, message *warp.Message, justification []byte, - validators []warp.Validator, + validators []*warp.Validator, quorumNum uint64, quorumDen uint64, -) (*warp.Message, uint64, uint64, error) { +) ( + _ *warp.Message, + aggregatedStake *big.Int, + totalStake *big.Int, + finished bool, + _ error, +) { request := &sdk.SignatureRequest{ Message: message.UnsignedMessage.Bytes(), Justification: justification, @@ -73,7 +76,7 @@ func (s *SignatureAggregator) AggregateSignatures( requestBytes, err := proto.Marshal(request) if err != nil { - return nil, 0, 0, fmt.Errorf("failed to marshal signature request: %w", err) + return nil, nil, nil, false, fmt.Errorf("failed to marshal signature request: %w", err) } publicKeysToValidators := make(map[*bls.PublicKey]indexedValidator) @@ -81,22 +84,22 @@ func (s *SignatureAggregator) AggregateSignatures( // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) if !ok { - return nil, 0, 0, errors.New("invalid warp signature type") + return nil, nil, nil, false, errors.New("invalid warp signature type") } signerBitSet := set.BitsFromBytes(bitSetSignature.Signers) nonSigners := make([]*bls.PublicKey, 0, len(validators)) - aggregatedStakeWeight := uint64(0) - totalStakeWeight := uint64(0) + aggregatedStakeWeight := new(big.Int) + totalStakeWeight := new(big.Int) numRequests := 0 for i, v := range validators { - totalStakeWeight += v.Weight + totalStakeWeight.Add(totalStakeWeight, new(big.Int).SetUint64(v.Weight)) // Only try to aggregate signatures from validators that are not already in // the signer bit set if signerBitSet.Contains(i) { - aggregatedStakeWeight += v.Weight + aggregatedStakeWeight.Add(aggregatedStakeWeight, new(big.Int).SetUint64(v.Weight)) continue } @@ -117,14 +120,14 @@ func (s *SignatureAggregator) AggregateSignatures( if bitSetSignature.Signature != [bls.SignatureLen]byte{} { blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) if err != nil { - return nil, 0, 0, fmt.Errorf("failed to parse bls signature: %w", err) + return nil, nil, nil, false, fmt.Errorf("failed to parse bls signature: %w", err) } signatures = append(signatures, blsSignature) } - //TODO better than buffered channel? + // TODO something better than a buffered channel? results := make(chan result, numRequests) - job := responseHandler{ + handler := responseHandler{ message: message, publicKeysToValidators: publicKeysToValidators, nodeIDsToPublicKeys: nodeIDsToPublicKeys, @@ -134,14 +137,14 @@ func (s *SignatureAggregator) AggregateSignatures( for _, pk := range nonSigners { validator := publicKeysToValidators[pk] for _, nodeID := range validator.NodeIDs { - if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, job.HandleResponse); err != nil { - results <- result{NodeID: nodeID, Validator: validator, Err: err} - // TODO fatal + if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, handler.HandleResponse); err != nil { + results <- result{NodeID: nodeID, Validator: validator, Err: err, ShouldClose: true} } } } - minThreshold := (totalStakeWeight * quorumNum) / quorumDen + minThreshold := new(big.Int).Mul(totalStakeWeight, new(big.Int).SetUint64(quorumNum)) + minThreshold.Div(minThreshold, new(big.Int).SetUint64(quorumDen)) for i := 0; i < numRequests; i++ { select { @@ -149,10 +152,10 @@ func (s *SignatureAggregator) AggregateSignatures( // Try to return whatever progress we have if the context is cancelled msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { - return nil, 0, 0, err + return nil, nil, nil, false, err } - return msg, aggregatedStakeWeight, totalStakeWeight, nil + return msg, aggregatedStakeWeight, totalStakeWeight, false, nil case result := <-results: if result.Err != nil { s.log.Debug( @@ -160,7 +163,12 @@ func (s *SignatureAggregator) AggregateSignatures( zap.Stringer("nodeID", result.NodeID), zap.Error(err), ) - continue + + if !result.ShouldClose { + continue + } + + return nil, nil, nil, false, result.Err } if signerBitSet.Contains(result.Validator.Index) { @@ -174,25 +182,25 @@ func (s *SignatureAggregator) AggregateSignatures( signatures = append(signatures, result.Signature) signerBitSet.Add(result.Validator.Index) - aggregatedStakeWeight += result.Validator.Weight + aggregatedStakeWeight.Add(aggregatedStakeWeight, new(big.Int).SetUint64(result.Validator.Weight)) - if aggregatedStakeWeight >= minThreshold { + if aggregatedStakeWeight.Cmp(minThreshold) != -1 { msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { - return nil, 0, 0, err + return nil, nil, nil, false, err } - return msg, aggregatedStakeWeight, totalStakeWeight, nil + return msg, aggregatedStakeWeight, totalStakeWeight, true, nil } } } msg, err := newWarpMessage(message, signerBitSet, signatures) if err != nil { - return nil, 0, 0, err + return nil, nil, nil, false, err } - return msg, aggregatedStakeWeight, totalStakeWeight, nil + return msg, aggregatedStakeWeight, totalStakeWeight, true, nil } func newWarpMessage( diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 234d02036a33..639dbf83708e 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -5,6 +5,7 @@ package acp118 import ( "context" + "math/big" "testing" "github.com/stretchr/testify/require" @@ -42,15 +43,16 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { signer2 := warp.NewSigner(sk2, networkID, chainID) tests := []struct { - name string - peers map[ids.NodeID]p2p.Handler - ctx context.Context - validators []warp.Validator - quorumNum uint64 - quorumDen uint64 - wantMsg *warp.Message - wantSigners []int - wantErr error + name string + peers map[ids.NodeID]p2p.Handler + ctx context.Context + validators []warp.Validator + quorumNum uint64 + quorumDen uint64 + wantMsg *warp.Message + wantSigners []int + wantFinished bool + wantErr error }{ { name: "aggregates from all validators 1/1", @@ -65,9 +67,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantSigners: []int{0}, - quorumNum: 1, - quorumDen: 1, + wantSigners: []int{0}, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, { name: "aggregates from some validators - 1/3", @@ -100,9 +103,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0}, - quorumNum: 1, - quorumDen: 3, + wantSigners: []int{0}, + wantFinished: true, + quorumNum: 1, + quorumDen: 3, }, { name: "aggregates from some validators - 2/3", @@ -132,9 +136,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0, 1}, - quorumNum: 3, - quorumDen: 6, + wantSigners: []int{0, 1}, + wantFinished: true, + quorumNum: 3, + quorumDen: 6, }, { name: "aggregates from all validators - 3/3", @@ -161,9 +166,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0, 1, 2}, - quorumNum: 1, - quorumDen: 1, + wantSigners: []int{0, 1, 2}, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, { name: "fails aggregation from some validators - 1/2", @@ -187,10 +193,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID1}, }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0}, - wantErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, + wantSigners: []int{0}, + wantFinished: true, }, { name: "fails aggregation from some validators - 1/3", @@ -223,10 +229,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - quorumNum: 2, - quorumDen: 3, - wantSigners: []int{0}, - wantErr: ErrFailedAggregation, + quorumNum: 2, + quorumDen: 3, + wantSigners: []int{0}, + wantFinished: true, }, { name: "fails aggregation from some validators - 2/3", @@ -256,10 +262,10 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0, 1}, - wantErr: ErrFailedAggregation, + quorumNum: 1, + quorumDen: 1, + wantSigners: []int{0, 1}, + wantFinished: true, }, { name: "fails aggregation from all validators", @@ -277,9 +283,9 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantErr: ErrFailedAggregation, - quorumNum: 1, - quorumDen: 1, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, { name: "context canceled", @@ -312,6 +318,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { t, context.Background(), ids.EmptyNodeID, + p2p.NoOpHandler{}, tt.peers, ) aggregator := NewSignatureAggregator(logging.NoLog{}, client) @@ -327,7 +334,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, ) require.NoError(err) - gotMsg, gotAggregatedStake, gotTotalStake, err := aggregator.AggregateSignatures( + gotMsg, gotAggregatedStake, gotTotalStake, finished, err := aggregator.AggregateSignatures( tt.ctx, msg, []byte("justification"), @@ -336,6 +343,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { tt.quorumDen, ) require.ErrorIs(err, tt.wantErr) + require.Equal(tt.wantFinished, finished) if tt.wantErr != nil { return @@ -345,15 +353,15 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { bitSet := set.BitsFromBytes(bitSetSignature.Signers) require.Equal(len(tt.wantSigners), bitSet.Len()) - wantAggregatedStake := uint64(0) + wantAggregatedStake := new(big.Int) for _, i := range tt.wantSigners { require.True(bitSet.Contains(i)) - wantAggregatedStake += tt.validators[i].Weight + wantAggregatedStake.Add(wantAggregatedStake, new(big.Int).SetUint64(tt.validators[i].Weight)) } - wantTotalStake := uint64(0) + wantTotalStake := new(big.Int) for _, v := range tt.validators { - wantTotalStake += v.Weight + wantTotalStake.Add(wantTotalStake, new(big.Int).SetUint64(v.Weight)) } require.Equal(wantAggregatedStake, gotAggregatedStake) diff --git a/network/p2p/acp118/handler_test.go b/network/p2p/acp118/handler_test.go index 372156eaf866..fad61cf332e7 100644 --- a/network/p2p/acp118/handler_test.go +++ b/network/p2p/acp118/handler_test.go @@ -7,6 +7,7 @@ import ( "context" "testing" + "github.com/ava-labs/avalanchego/network/p2p" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" @@ -84,9 +85,10 @@ func TestHandler(t *testing.T) { c := p2ptest.NewClient( t, ctx, - h, clientNodeID, + p2p.NoOpHandler{}, serverNodeID, + h, ) unsignedMessage, err := warp.NewUnsignedMessage( From 6fcc0aac62329b13c31c2b4d78a7eb22f3c4dad5 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:30:28 -0500 Subject: [PATCH 45/62] nti Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 639dbf83708e..40e82844680a 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -46,7 +46,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { name string peers map[ids.NodeID]p2p.Handler ctx context.Context - validators []warp.Validator + validators []*warp.Validator quorumNum uint64 quorumDen uint64 wantMsg *warp.Message @@ -60,7 +60,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { nodeID0: NewHandler(&testVerifier{}, signer0), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -86,7 +86,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -119,7 +119,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -149,7 +149,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { nodeID2: NewHandler(&testVerifier{}, signer2), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -181,7 +181,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -212,7 +212,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -245,7 +245,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -276,7 +276,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { ), }, ctx: context.Background(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, @@ -298,7 +298,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { return ctx }(), - validators: []warp.Validator{ + validators: []*warp.Validator{ { PublicKey: pk0, Weight: 1, From 17f2d754902f94cdcd67af08047365ffe9acf23a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:35:33 -0500 Subject: [PATCH 46/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client_test.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/network/p2p/p2ptest/client_test.go b/network/p2p/p2ptest/client_test.go index 179067bfa7df..0acb5e4c1a5f 100644 --- a/network/p2p/p2ptest/client_test.go +++ b/network/p2p/p2ptest/client_test.go @@ -27,7 +27,14 @@ func TestNewClient_AppGossip(t *testing.T) { }, } - client := NewClient(t, ctx, testHandler, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + client := NewClient( + t, + ctx, + ids.GenerateTestNodeID(), + p2p.NoOpHandler{}, + ids.GenerateTestNodeID(), + testHandler, + ) require.NoError(client.AppGossip(ctx, common.SendConfig{}, []byte("foobar"))) <-appGossipChan } @@ -94,7 +101,14 @@ func TestNewClient_AppRequest(t *testing.T) { }, } - client := NewClient(t, ctx, testHandler, ids.EmptyNodeID, ids.EmptyNodeID) + client := NewClient( + t, + ctx, + ids.GenerateTestNodeID(), + p2p.NoOpHandler{}, + ids.GenerateTestNodeID(), + testHandler, + ) require.NoError(tt.appRequestF( ctx, client, From 57506a41e958725975da451841da2caee3099d28 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 10:43:29 -0500 Subject: [PATCH 47/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client_test.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/network/p2p/p2ptest/client_test.go b/network/p2p/p2ptest/client_test.go index 0acb5e4c1a5f..17269d16dadd 100644 --- a/network/p2p/p2ptest/client_test.go +++ b/network/p2p/p2ptest/client_test.go @@ -16,7 +16,7 @@ import ( "github.com/ava-labs/avalanchego/utils/set" ) -func TestNewClient_AppGossip(t *testing.T) { +func TestClient_AppGossip(t *testing.T) { require := require.New(t) ctx := context.Background() @@ -27,19 +27,17 @@ func TestNewClient_AppGossip(t *testing.T) { }, } - client := NewClient( + client := NewSelfClient( t, ctx, ids.GenerateTestNodeID(), - p2p.NoOpHandler{}, - ids.GenerateTestNodeID(), testHandler, ) require.NoError(client.AppGossip(ctx, common.SendConfig{}, []byte("foobar"))) <-appGossipChan } -func TestNewClient_AppRequest(t *testing.T) { +func TestClient_AppRequest(t *testing.T) { tests := []struct { name string appResponse []byte @@ -101,21 +99,19 @@ func TestNewClient_AppRequest(t *testing.T) { }, } - client := NewClient( + client := NewSelfClient( t, ctx, - ids.GenerateTestNodeID(), - p2p.NoOpHandler{}, - ids.GenerateTestNodeID(), + ids.EmptyNodeID, testHandler, ) require.NoError(tt.appRequestF( ctx, client, func(_ context.Context, _ ids.NodeID, responseBytes []byte, err error) { + defer close(appRequestChan) require.ErrorIs(err, tt.appErr) require.Equal(tt.appResponse, responseBytes) - close(appRequestChan) }, )) <-appRequestChan From 048dca9c9dbe1fb15f76b173412b11f821ba2853 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:03:26 -0500 Subject: [PATCH 48/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 21 +++---- x/sync/sync_test.go | 94 ++++++++++++++++---------------- 2 files changed, 54 insertions(+), 61 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 942ed0beb4f0..bb653cf8c9f8 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -29,11 +29,10 @@ type indexedValidator struct { } type result struct { - NodeID ids.NodeID - Validator indexedValidator - Signature *bls.Signature - Err error - ShouldClose bool + NodeID ids.NodeID + Validator indexedValidator + Signature *bls.Signature + Err error } // NewSignatureAggregator returns an instance of SignatureAggregator @@ -125,8 +124,7 @@ func (s *SignatureAggregator) AggregateSignatures( signatures = append(signatures, blsSignature) } - // TODO something better than a buffered channel? - results := make(chan result, numRequests) + results := make(chan result) handler := responseHandler{ message: message, publicKeysToValidators: publicKeysToValidators, @@ -138,7 +136,7 @@ func (s *SignatureAggregator) AggregateSignatures( validator := publicKeysToValidators[pk] for _, nodeID := range validator.NodeIDs { if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, handler.HandleResponse); err != nil { - results <- result{NodeID: nodeID, Validator: validator, Err: err, ShouldClose: true} + return nil, nil, nil, false, fmt.Errorf("failed to send aggregation request: %w", err) } } } @@ -163,12 +161,7 @@ func (s *SignatureAggregator) AggregateSignatures( zap.Stringer("nodeID", result.NodeID), zap.Error(err), ) - - if !result.ShouldClose { - continue - } - - return nil, nil, nil, false, result.Err + continue } if signerBitSet.Contains(result.Validator.Index) { diff --git a/x/sync/sync_test.go b/x/sync/sync_test.go index 9cb314600b4f..a2800e4f812e 100644 --- a/x/sync/sync_test.go +++ b/x/sync/sync_test.go @@ -39,8 +39,8 @@ func Test_Creation(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), SimultaneousWorkLimit: 5, Log: logging.NoLog{}, BranchFactor: merkledb.BranchFactor16, @@ -72,8 +72,8 @@ func Test_Completion(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, emptyDB), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, emptyDB), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, emptyDB)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, emptyDB)), TargetRoot: emptyRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -177,8 +177,8 @@ func Test_Sync_FindNextKey_InSync(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, dbToSync)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, dbToSync)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -253,8 +253,8 @@ func Test_Sync_FindNextKey_Deleted(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -303,8 +303,8 @@ func Test_Sync_FindNextKey_BranchInLocal(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), TargetRoot: targetRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -340,8 +340,8 @@ func Test_Sync_FindNextKey_BranchInReceived(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), TargetRoot: targetRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -376,8 +376,8 @@ func Test_Sync_FindNextKey_ExtraValues(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, dbToSync)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, dbToSync)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -438,8 +438,8 @@ func TestFindNextKeyEmptyEndProof(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), TargetRoot: ids.Empty, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -507,8 +507,8 @@ func Test_Sync_FindNextKey_DifferentChild(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, dbToSync)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, dbToSync)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -730,8 +730,8 @@ func TestFindNextKeyRandom(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: localDB, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, remoteDB), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, remoteDB), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, remoteDB)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, remoteDB)), TargetRoot: ids.GenerateTestID(), SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -777,7 +777,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = append(response.KeyValues, merkledb.KeyValue{}) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -787,7 +787,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = response.KeyValues[min(1, len(response.KeyValues)):] }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -813,7 +813,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { } }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -824,7 +824,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { _ = slices.Delete(response.KeyValues, i, min(len(response.KeyValues), i+1)) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -835,7 +835,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -845,7 +845,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -857,16 +857,16 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyValues = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { name: "range proof server flake", rangeProofClient: func(db merkledb.MerkleDB) *p2p.Client { - return p2ptest.NewClient(t, context.Background(), &flakyHandler{ + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, &flakyHandler{ Handler: NewGetRangeProofHandler(logging.NoLog{}, db), c: &counter{m: 2}, - }, ids.EmptyNodeID, ids.EmptyNodeID) + }) }, }, { @@ -876,7 +876,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyChanges = append(response.KeyChanges, make([]merkledb.KeyChange, defaultRequestKeyLimit)...) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -886,7 +886,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.KeyChanges = response.KeyChanges[min(1, len(response.KeyChanges)):] }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -897,7 +897,7 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { _ = slices.Delete(response.KeyChanges, i, min(len(response.KeyChanges), i+1)) }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { @@ -908,16 +908,16 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { response.EndProof = nil }) - return p2ptest.NewClient(t, context.Background(), handler, ids.EmptyNodeID, ids.EmptyNodeID) + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, handler) }, }, { name: "change proof flaky server", changeProofClient: func(db merkledb.MerkleDB) *p2p.Client { - return p2ptest.NewClient(t, context.Background(), &flakyHandler{ + return p2ptest.NewSelfClient(t, context.Background(), ids.EmptyNodeID, &flakyHandler{ Handler: NewGetChangeProofHandler(logging.NoLog{}, db), c: &counter{m: 2}, - }, ids.EmptyNodeID, ids.EmptyNodeID) + }) }, }, } @@ -946,13 +946,13 @@ func Test_Sync_Result_Correct_Root(t *testing.T) { ) rangeProofHandler := NewGetRangeProofHandler(logging.NoLog{}, dbToSync) - rangeProofClient = p2ptest.NewClient(t, ctx, rangeProofHandler, ids.EmptyNodeID, ids.EmptyNodeID) + rangeProofClient = p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, rangeProofHandler) if tt.rangeProofClient != nil { rangeProofClient = tt.rangeProofClient(dbToSync) } changeProofHandler := NewGetChangeProofHandler(logging.NoLog{}, dbToSync) - changeProofClient = p2ptest.NewClient(t, ctx, changeProofHandler, ids.EmptyNodeID, ids.EmptyNodeID) + changeProofClient = p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, changeProofHandler) if tt.changeProofClient != nil { changeProofClient = tt.changeProofClient(dbToSync) } @@ -1031,8 +1031,8 @@ func Test_Sync_Result_Correct_Root_With_Sync_Restart(t *testing.T) { ctx := context.Background() syncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, dbToSync)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, dbToSync)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -1058,8 +1058,8 @@ func Test_Sync_Result_Correct_Root_With_Sync_Restart(t *testing.T) { newSyncer, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, dbToSync), ids.EmptyNodeID, ids.EmptyNodeID), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, dbToSync)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, dbToSync)), TargetRoot: syncRoot, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, @@ -1130,15 +1130,15 @@ func Test_Sync_Result_Correct_Root_Update_Root_During(t *testing.T) { updatedRootChan <- struct{}{} ctx := context.Background() - rangeProofClient := p2ptest.NewClient(t, ctx, &waitingHandler{ + rangeProofClient := p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, &waitingHandler{ handler: NewGetRangeProofHandler(logging.NoLog{}, dbToSync), updatedRootChan: updatedRootChan, - }, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + }) - changeProofClient := p2ptest.NewClient(t, ctx, &waitingHandler{ + changeProofClient := p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, &waitingHandler{ handler: NewGetChangeProofHandler(logging.NoLog{}, dbToSync), updatedRootChan: updatedRootChan, - }, ids.GenerateTestNodeID(), ids.GenerateTestNodeID()) + }) syncer, err := NewManager(ManagerConfig{ DB: db, @@ -1189,8 +1189,8 @@ func Test_Sync_UpdateSyncTarget(t *testing.T) { ctx := context.Background() m, err := NewManager(ManagerConfig{ DB: db, - RangeProofClient: p2ptest.NewClient(t, ctx, NewGetRangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), - ChangeProofClient: p2ptest.NewClient(t, ctx, NewGetChangeProofHandler(logging.NoLog{}, db), ids.GenerateTestNodeID(), ids.GenerateTestNodeID()), + RangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetRangeProofHandler(logging.NoLog{}, db)), + ChangeProofClient: p2ptest.NewSelfClient(t, ctx, ids.EmptyNodeID, NewGetChangeProofHandler(logging.NoLog{}, db)), TargetRoot: ids.Empty, SimultaneousWorkLimit: 5, Log: logging.NoLog{}, From 79f560b0dca788b317c8ed5ff9e9ab0a261cec1a Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:08:16 -0500 Subject: [PATCH 49/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/network/p2p/acp118/handler_test.go b/network/p2p/acp118/handler_test.go index fad61cf332e7..e67518a5a572 100644 --- a/network/p2p/acp118/handler_test.go +++ b/network/p2p/acp118/handler_test.go @@ -7,12 +7,12 @@ import ( "context" "testing" - "github.com/ava-labs/avalanchego/network/p2p" "github.com/stretchr/testify/require" "google.golang.org/protobuf/proto" "github.com/ava-labs/avalanchego/cache" "github.com/ava-labs/avalanchego/ids" + "github.com/ava-labs/avalanchego/network/p2p" "github.com/ava-labs/avalanchego/network/p2p/p2ptest" "github.com/ava-labs/avalanchego/proto/pb/sdk" "github.com/ava-labs/avalanchego/snow/engine/common" From e55212f85d9e5a35ac8c8869cbc249187a67bb66 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 11:38:22 -0500 Subject: [PATCH 50/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 1 + 1 file changed, 1 insertion(+) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index bb653cf8c9f8..d8d3d398de14 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -115,6 +115,7 @@ func (s *SignatureAggregator) AggregateSignatures( nonSigners = append(nonSigners, v.PublicKey) } + // Account for requested signatures + the signature that was provided signatures := make([]*bls.Signature, 0, len(nonSigners)+1) if bitSetSignature.Signature != [bls.SignatureLen]byte{} { blsSignature, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) From 835de57f52acdff2667618195c5037ae53fd4299 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Tue, 3 Dec 2024 15:02:33 -0500 Subject: [PATCH 51/62] wip Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 40e82844680a..54b4c9499674 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -54,8 +54,9 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { wantFinished bool wantErr error }{ + // TODO test different weights { - name: "aggregates from all validators 1/1", + name: "meets threshold - 1/1 validators", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), }, @@ -73,7 +74,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 1, }, { - name: "aggregates from some validators - 1/3", + name: "meets threshold - 1/3 validators", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler( @@ -109,7 +110,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 3, }, { - name: "aggregates from some validators - 2/3", + name: "meets threshold - 2/3 validators", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), @@ -127,19 +128,19 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk1, - Weight: 2, + Weight: 1, NodeIDs: []ids.NodeID{nodeID1}, }, { PublicKey: pk2, - Weight: 3, + Weight: 1, NodeIDs: []ids.NodeID{nodeID2}, }, }, wantSigners: []int{0, 1}, wantFinished: true, - quorumNum: 3, - quorumDen: 6, + quorumNum: 2, + quorumDen: 3, }, { name: "aggregates from all validators - 3/3", @@ -157,12 +158,12 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk1, - Weight: 2, + Weight: 1, NodeIDs: []ids.NodeID{nodeID1}, }, { PublicKey: pk2, - Weight: 3, + Weight: 1, NodeIDs: []ids.NodeID{nodeID2}, }, }, @@ -321,6 +322,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { p2p.NoOpHandler{}, tt.peers, ) + aggregator := NewSignatureAggregator(logging.NoLog{}, client) unsignedMsg, err := warp.NewUnsignedMessage( @@ -334,6 +336,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, ) require.NoError(err) + gotMsg, gotAggregatedStake, gotTotalStake, finished, err := aggregator.AggregateSignatures( tt.ctx, msg, From 39f8ca4fbeb8e6f14ac4fc1f04249c9604dcbf3e Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 14:36:30 -0500 Subject: [PATCH 52/62] add better tests Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 266 ++++++++++++++++---------- 1 file changed, 165 insertions(+), 101 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 54b4c9499674..705a75086f02 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -43,20 +43,41 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { signer2 := warp.NewSigner(sk2, networkID, chainID) tests := []struct { - name string - peers map[ids.NodeID]p2p.Handler - ctx context.Context - validators []*warp.Validator - quorumNum uint64 - quorumDen uint64 - wantMsg *warp.Message - wantSigners []int - wantFinished bool - wantErr error + name string + peers map[ids.NodeID]p2p.Handler + ctx context.Context + validators []*warp.Validator + quorumNum uint64 + quorumDen uint64 + wantTotalStake int + wantMsg *warp.Message + wantSigners int + wantPossibleSigners []int + wantFinished bool + wantErr error }{ - // TODO test different weights + // TODO test message w/ signature + // TODO test shared pks { - name: "meets threshold - 1/1 validators", + name: "single validator - less than threshold", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer0), + }, + ctx: context.Background(), + validators: []*warp.Validator{ + { + PublicKey: pk0, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, + }, + }, + wantTotalStake: 1, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, + }, + { + name: "single validator - equal to threshold", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), }, @@ -68,23 +89,39 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantSigners: []int{0}, - wantFinished: true, - quorumNum: 1, - quorumDen: 1, + wantTotalStake: 1, + wantSigners: 1, + wantPossibleSigners: []int{0}, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, { - name: "meets threshold - 1/3 validators", + name: "single validator - greater than threshold", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), - nodeID1: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer1, - ), - nodeID2: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer2, - ), + }, + ctx: context.Background(), + validators: []*warp.Validator{ + { + PublicKey: pk0, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, + }, + }, + wantTotalStake: 1, + wantSigners: 1, + wantPossibleSigners: []int{0}, + wantFinished: true, + quorumNum: 1, + quorumDen: 2, + }, + { + name: "multiple validators - less than threshold - equal weights", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer1), + nodeID2: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -104,20 +141,19 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0}, - wantFinished: true, - quorumNum: 1, - quorumDen: 3, + wantTotalStake: 3, + wantSigners: 1, + wantPossibleSigners: []int{0}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { - name: "meets threshold - 2/3 validators", + name: "multiple validators - equal to threshold - equal weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), - nodeID2: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer2, - ), + nodeID2: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -137,13 +173,15 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0, 1}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 3, + wantSigners: 2, + wantPossibleSigners: []int{0, 1}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { - name: "aggregates from all validators - 3/3", + name: "multiple validators - greater than threshold - equal weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), @@ -167,19 +205,19 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantSigners: []int{0, 1, 2}, - wantFinished: true, - quorumNum: 1, - quorumDen: 1, + wantTotalStake: 3, + wantSigners: 2, + wantPossibleSigners: []int{0, 1, 2}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { - name: "fails aggregation from some validators - 1/2", + name: "multiple validators - less than threshold - different weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), - nodeID1: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer1, - ), + nodeID1: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer1), + nodeID2: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -190,27 +228,28 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk1, - Weight: 1, + Weight: 2, NodeIDs: []ids.NodeID{nodeID1}, }, + { + PublicKey: pk2, + Weight: 3, + NodeIDs: []ids.NodeID{nodeID2}, + }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0}, - wantFinished: true, + wantTotalStake: 6, + wantSigners: 1, + wantPossibleSigners: []int{0}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { - name: "fails aggregation from some validators - 1/3", + name: "multiple validators - equal to threshold - equal weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), - nodeID1: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer1, - ), - nodeID2: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer2, - ), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -221,29 +260,28 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk1, - Weight: 1, + Weight: 2, NodeIDs: []ids.NodeID{nodeID1}, }, { PublicKey: pk2, - Weight: 1, + Weight: 3, NodeIDs: []ids.NodeID{nodeID2}, }, }, - quorumNum: 2, - quorumDen: 3, - wantSigners: []int{0}, - wantFinished: true, + wantTotalStake: 6, + wantSigners: 2, + wantPossibleSigners: []int{0, 1}, + wantFinished: true, + quorumNum: 1, + quorumDen: 2, }, { - name: "fails aggregation from some validators - 2/3", + name: "multiple validators - greater than threshold - equal weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), - nodeID2: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer2, - ), + nodeID2: NewHandler(&testVerifier{}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -254,29 +292,33 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk1, - Weight: 1, + Weight: 2, NodeIDs: []ids.NodeID{nodeID1}, }, { PublicKey: pk2, - Weight: 1, + Weight: 3, NodeIDs: []ids.NodeID{nodeID2}, }, }, - quorumNum: 1, - quorumDen: 1, - wantSigners: []int{0, 1}, - wantFinished: true, + wantTotalStake: 6, + wantSigners: 2, + wantPossibleSigners: []int{0, 1, 2}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { - name: "fails aggregation from all validators", + name: "single validator - context canceled", peers: map[ids.NodeID]p2p.Handler{ - nodeID0: NewHandler( - &testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, - signer0, - ), + nodeID0: NewHandler(&testVerifier{}, signer0), }, - ctx: context.Background(), + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + return ctx + }(), validators: []*warp.Validator{ { PublicKey: pk0, @@ -284,14 +326,16 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantFinished: true, - quorumNum: 1, - quorumDen: 1, + wantTotalStake: 1, + quorumNum: 1, + quorumDen: 1, }, { - name: "context canceled", + name: "multiple validators - context canceled", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer2), }, ctx: func() context.Context { ctx, cancel := context.WithCancel(context.Background()) @@ -305,9 +349,20 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { Weight: 1, NodeIDs: []ids.NodeID{nodeID0}, }, + { + PublicKey: pk1, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, + }, + { + PublicKey: pk2, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID2}, + }, }, - quorumNum: 0, - quorumDen: 1, + wantTotalStake: 3, + quorumNum: 1, + quorumDen: 1, }, } @@ -354,21 +409,30 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { bitSetSignature := gotMsg.Signature.(*warp.BitSetSignature) bitSet := set.BitsFromBytes(bitSetSignature.Signers) - require.Equal(len(tt.wantSigners), bitSet.Len()) + require.Equal(tt.wantSigners, bitSet.Len()) + + pks := make([]*bls.PublicKey, 0) + wantAggregatedStake := uint64(0) + for i := 0; i < bitSet.BitLen(); i++ { + if !bitSet.Contains(i) { + continue + } - wantAggregatedStake := new(big.Int) - for _, i := range tt.wantSigners { - require.True(bitSet.Contains(i)) - wantAggregatedStake.Add(wantAggregatedStake, new(big.Int).SetUint64(tt.validators[i].Weight)) + pks = append(pks, tt.validators[i].PublicKey) + wantAggregatedStake += tt.validators[i].Weight } - wantTotalStake := new(big.Int) - for _, v := range tt.validators { - wantTotalStake.Add(wantTotalStake, new(big.Int).SetUint64(v.Weight)) + if tt.wantSigners > 0 { + aggPk, err := bls.AggregatePublicKeys(pks) + require.NoError(err) + blsSig, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) + require.NoError(err) + require.True(bls.Verify(aggPk, blsSig, unsignedMsg.Bytes())) } - require.Equal(wantAggregatedStake, gotAggregatedStake) - require.Equal(wantTotalStake, gotTotalStake) + require.Equal(new(big.Int).SetUint64(wantAggregatedStake), gotAggregatedStake) + require.Equal(new(big.Int).SetUint64(uint64(tt.wantTotalStake)), gotTotalStake) + }) } } From f5e05b92282525e4aec15efa1ea63810900dc54d Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:08:38 -0500 Subject: [PATCH 53/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 123 ++++++++++++++++++++++---- 1 file changed, 105 insertions(+), 18 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 705a75086f02..77ac83566dbb 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -42,10 +42,18 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { pk2 := bls.PublicFromSecretKey(sk2) signer2 := warp.NewSigner(sk2, networkID, chainID) + unsignedMsg, err := warp.NewUnsignedMessage( + networkID, + chainID, + []byte("payload"), + ) + require.NoError(t, err) + tests := []struct { name string peers map[ids.NodeID]p2p.Handler ctx context.Context + signature warp.BitSetSignature validators []*warp.Validator quorumNum uint64 quorumDen uint64 @@ -56,8 +64,6 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { wantFinished bool wantErr error }{ - // TODO test message w/ signature - // TODO test shared pks { name: "single validator - less than threshold", peers: map[ids.NodeID]p2p.Handler{ @@ -308,6 +314,55 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumNum: 2, quorumDen: 3, }, + { + name: "multiple validators - shared public keys", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer1), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer1), + }, + ctx: context.Background(), + validators: []*warp.Validator{ + { + PublicKey: pk1, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID0, nodeID1, nodeID2}, + }, + }, + wantTotalStake: 1, + wantSigners: 1, + wantPossibleSigners: []int{0, 1, 2}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, + }, + { + name: "multiple validators - unique and shared public keys", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer1), + }, + ctx: context.Background(), + validators: []*warp.Validator{ + { + PublicKey: pk0, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, + }, + { + PublicKey: pk1, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID1, nodeID2}, + }, + }, + wantTotalStake: 2, + wantSigners: 1, + wantPossibleSigners: []int{0, 1}, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, + }, { name: "single validator - context canceled", peers: map[ids.NodeID]p2p.Handler{ @@ -364,7 +419,50 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumNum: 1, quorumDen: 1, }, - } + { + name: "multiple validators - resume aggregation on signature", + peers: map[ids.NodeID]p2p.Handler{ + nodeID0: NewHandler(&testVerifier{}, signer0), + nodeID1: NewHandler(&testVerifier{}, signer1), + nodeID2: NewHandler(&testVerifier{}, signer2), + }, + ctx: context.Background(), + signature: func() warp.BitSetSignature { + sig := warp.BitSetSignature{ + Signers: set.NewBits(0).Bytes(), + Signature: [96]byte{}, + } + + sigBytes, err := signer0.Sign(unsignedMsg) + require.NoError(t, err) + copy(sig.Signature[:], sigBytes) + + return sig + }(), + validators: []*warp.Validator{ + { + PublicKey: pk0, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID0}, + }, + { + PublicKey: pk1, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID1}, + }, + { + PublicKey: pk2, + Weight: 1, + NodeIDs: []ids.NodeID{nodeID2}, + }, + }, + wantTotalStake: 3, + wantSigners: 3, + wantPossibleSigners: []int{0, 1, 2}, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -377,19 +475,9 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { p2p.NoOpHandler{}, tt.peers, ) - aggregator := NewSignatureAggregator(logging.NoLog{}, client) - unsignedMsg, err := warp.NewUnsignedMessage( - networkID, - chainID, - []byte("payload"), - ) - require.NoError(err) - msg, err := warp.NewMessage( - unsignedMsg, - &warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}}, - ) + msg, err := warp.NewMessage(unsignedMsg, &tt.signature) require.NoError(err) gotMsg, gotAggregatedStake, gotTotalStake, finished, err := aggregator.AggregateSignatures( @@ -407,8 +495,8 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { return } - bitSetSignature := gotMsg.Signature.(*warp.BitSetSignature) - bitSet := set.BitsFromBytes(bitSetSignature.Signers) + gotSignature := gotMsg.Signature.(*warp.BitSetSignature) + bitSet := set.BitsFromBytes(gotSignature.Signers) require.Equal(tt.wantSigners, bitSet.Len()) pks := make([]*bls.PublicKey, 0) @@ -425,14 +513,13 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { if tt.wantSigners > 0 { aggPk, err := bls.AggregatePublicKeys(pks) require.NoError(err) - blsSig, err := bls.SignatureFromBytes(bitSetSignature.Signature[:]) + blsSig, err := bls.SignatureFromBytes(gotSignature.Signature[:]) require.NoError(err) require.True(bls.Verify(aggPk, blsSig, unsignedMsg.Bytes())) } require.Equal(new(big.Int).SetUint64(wantAggregatedStake), gotAggregatedStake) require.Equal(new(big.Int).SetUint64(uint64(tt.wantTotalStake)), gotTotalStake) - }) } } From b598c398e25f9406d13e3fc573b0a12c88e97714 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:13:31 -0500 Subject: [PATCH 54/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index 77ac83566dbb..aa27dd38dca2 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -462,7 +462,8 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { wantFinished: true, quorumNum: 1, quorumDen: 1, - }} + }, + } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From ba022cabc3bd2faa92941ec1fb97621f9723b8eb Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:55:07 -0500 Subject: [PATCH 55/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index aa27dd38dca2..f21ac06621b1 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -251,7 +251,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 3, }, { - name: "multiple validators - equal to threshold - equal weights", + name: "multiple validators - equal to threshold - different weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), @@ -283,7 +283,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { quorumDen: 2, }, { - name: "multiple validators - greater than threshold - equal weights", + name: "multiple validators - greater than threshold - different weights", peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), From e308117755a9ec7ee272fe8651592f37dcf784c2 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:04:01 -0500 Subject: [PATCH 56/62] fix flake Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index f21ac06621b1..e4376267df0b 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -287,7 +287,7 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { peers: map[ids.NodeID]p2p.Handler{ nodeID0: NewHandler(&testVerifier{}, signer0), nodeID1: NewHandler(&testVerifier{}, signer1), - nodeID2: NewHandler(&testVerifier{}, signer2), + nodeID2: NewHandler(&testVerifier{Errs: []*common.AppError{common.ErrUndefined}}, signer2), }, ctx: context.Background(), validators: []*warp.Validator{ @@ -309,9 +309,9 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, wantTotalStake: 6, wantSigners: 2, - wantPossibleSigners: []int{0, 1, 2}, + wantPossibleSigners: []int{0, 1}, wantFinished: true, - quorumNum: 2, + quorumNum: 1, quorumDen: 3, }, { From b8702202bf72d660e76c2bd8d3e85c6e48d2a817 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:06:39 -0500 Subject: [PATCH 57/62] remove copy for shadowing Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/network/p2p/p2ptest/client.go b/network/p2p/p2ptest/client.go index c5ff44c86f64..0efb96dc67b8 100644 --- a/network/p2p/p2ptest/client.go +++ b/network/p2p/p2ptest/client.go @@ -107,11 +107,8 @@ func NewClientWithPeers( for nodeID := range peers { peerSenders[nodeID].SendAppErrorF = func(ctx context.Context, _ ids.NodeID, requestID uint32, errorCode int32, errorMessage string) error { - // Send the request asynchronously to avoid deadlock when the server - // sends the response back to the client - nodeIDCopy := nodeID go func() { - require.NoError(t, peerNetworks[clientNodeID].AppRequestFailed(ctx, nodeIDCopy, requestID, &common.AppError{ + require.NoError(t, peerNetworks[clientNodeID].AppRequestFailed(ctx, nodeID, requestID, &common.AppError{ Code: errorCode, Message: errorMessage, })) From e6959b1459487cd0e7601b7cabe99fe53ee6f18d Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:07:40 -0500 Subject: [PATCH 58/62] use send config Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/p2ptest/client.go | 4 ++-- network/p2p/p2ptest/client_test.go | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/network/p2p/p2ptest/client.go b/network/p2p/p2ptest/client.go index 0efb96dc67b8..4d722b968e2f 100644 --- a/network/p2p/p2ptest/client.go +++ b/network/p2p/p2ptest/client.go @@ -64,10 +64,10 @@ func NewClientWithPeers( peerNetworks[nodeID] = peerNetwork } - peerSenders[clientNodeID].SendAppGossipF = func(ctx context.Context, _ common.SendConfig, gossipBytes []byte) error { + peerSenders[clientNodeID].SendAppGossipF = func(ctx context.Context, sendConfig common.SendConfig, gossipBytes []byte) error { // Send the request asynchronously to avoid deadlock when the server // sends the response back to the client - for nodeID := range peers { + for nodeID := range sendConfig.NodeIDs { go func() { require.NoError(t, peerNetworks[nodeID].AppGossip(ctx, nodeID, gossipBytes)) }() diff --git a/network/p2p/p2ptest/client_test.go b/network/p2p/p2ptest/client_test.go index 17269d16dadd..c0032195aff3 100644 --- a/network/p2p/p2ptest/client_test.go +++ b/network/p2p/p2ptest/client_test.go @@ -27,13 +27,14 @@ func TestClient_AppGossip(t *testing.T) { }, } + nodeID := ids.GenerateTestNodeID() client := NewSelfClient( t, ctx, - ids.GenerateTestNodeID(), + nodeID, testHandler, ) - require.NoError(client.AppGossip(ctx, common.SendConfig{}, []byte("foobar"))) + require.NoError(client.AppGossip(ctx, common.SendConfig{NodeIDs: set.Of(nodeID)}, []byte("foobar"))) <-appGossipChan } From bab3a65144e88322740d361de89e79c757606216 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 16:17:10 -0500 Subject: [PATCH 59/62] remove unused testing fields Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator_test.go | 145 ++++++++++++-------------- 1 file changed, 66 insertions(+), 79 deletions(-) diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index e4376267df0b..f678e00487ee 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -50,19 +50,17 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { require.NoError(t, err) tests := []struct { - name string - peers map[ids.NodeID]p2p.Handler - ctx context.Context - signature warp.BitSetSignature - validators []*warp.Validator - quorumNum uint64 - quorumDen uint64 - wantTotalStake int - wantMsg *warp.Message - wantSigners int - wantPossibleSigners []int - wantFinished bool - wantErr error + name string + peers map[ids.NodeID]p2p.Handler + ctx context.Context + signature warp.BitSetSignature + validators []*warp.Validator + quorumNum uint64 + quorumDen uint64 + wantTotalStake int + wantSigners int + wantFinished bool + wantErr error }{ { name: "single validator - less than threshold", @@ -95,12 +93,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantTotalStake: 1, - wantSigners: 1, - wantPossibleSigners: []int{0}, - wantFinished: true, - quorumNum: 1, - quorumDen: 1, + wantTotalStake: 1, + wantSigners: 1, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, { name: "single validator - greater than threshold", @@ -115,12 +112,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0}, }, }, - wantTotalStake: 1, - wantSigners: 1, - wantPossibleSigners: []int{0}, - wantFinished: true, - quorumNum: 1, - quorumDen: 2, + wantTotalStake: 1, + wantSigners: 1, + wantFinished: true, + quorumNum: 1, + quorumDen: 2, }, { name: "multiple validators - less than threshold - equal weights", @@ -147,12 +143,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 3, - wantSigners: 1, - wantPossibleSigners: []int{0}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 3, + wantSigners: 1, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "multiple validators - equal to threshold - equal weights", @@ -179,12 +174,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 3, - wantSigners: 2, - wantPossibleSigners: []int{0, 1}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 3, + wantSigners: 2, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "multiple validators - greater than threshold - equal weights", @@ -211,12 +205,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 3, - wantSigners: 2, - wantPossibleSigners: []int{0, 1, 2}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 3, + wantSigners: 2, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "multiple validators - less than threshold - different weights", @@ -243,12 +236,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 6, - wantSigners: 1, - wantPossibleSigners: []int{0}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 6, + wantSigners: 1, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "multiple validators - equal to threshold - different weights", @@ -275,12 +267,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 6, - wantSigners: 2, - wantPossibleSigners: []int{0, 1}, - wantFinished: true, - quorumNum: 1, - quorumDen: 2, + wantTotalStake: 6, + wantSigners: 2, + wantFinished: true, + quorumNum: 1, + quorumDen: 2, }, { name: "multiple validators - greater than threshold - different weights", @@ -307,12 +298,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 6, - wantSigners: 2, - wantPossibleSigners: []int{0, 1}, - wantFinished: true, - quorumNum: 1, - quorumDen: 3, + wantTotalStake: 6, + wantSigners: 2, + wantFinished: true, + quorumNum: 1, + quorumDen: 3, }, { name: "multiple validators - shared public keys", @@ -329,12 +319,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID0, nodeID1, nodeID2}, }, }, - wantTotalStake: 1, - wantSigners: 1, - wantPossibleSigners: []int{0, 1, 2}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 1, + wantSigners: 1, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "multiple validators - unique and shared public keys", @@ -356,12 +345,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID1, nodeID2}, }, }, - wantTotalStake: 2, - wantSigners: 1, - wantPossibleSigners: []int{0, 1}, - wantFinished: true, - quorumNum: 2, - quorumDen: 3, + wantTotalStake: 2, + wantSigners: 1, + wantFinished: true, + quorumNum: 2, + quorumDen: 3, }, { name: "single validator - context canceled", @@ -456,12 +444,11 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 3, - wantSigners: 3, - wantPossibleSigners: []int{0, 1, 2}, - wantFinished: true, - quorumNum: 1, - quorumDen: 1, + wantTotalStake: 3, + wantSigners: 3, + wantFinished: true, + quorumNum: 1, + quorumDen: 1, }, } From f6d20e89fac68989328fb0c1441a373663292e9b Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Thu, 5 Dec 2024 23:59:47 -0500 Subject: [PATCH 60/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index d8d3d398de14..0a871166933d 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -135,10 +135,8 @@ func (s *SignatureAggregator) AggregateSignatures( for _, pk := range nonSigners { validator := publicKeysToValidators[pk] - for _, nodeID := range validator.NodeIDs { - if err := s.client.AppRequest(ctx, set.Of(nodeID), requestBytes, handler.HandleResponse); err != nil { - return nil, nil, nil, false, fmt.Errorf("failed to send aggregation request: %w", err) - } + if err := s.client.AppRequest(ctx, set.Of(validator.NodeIDs...), requestBytes, handler.HandleResponse); err != nil { + return nil, nil, nil, false, fmt.Errorf("failed to send aggregation request: %w", err) } } From 3e074e62b0c4f785341678de82202e68dadeda72 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Fri, 6 Dec 2024 14:33:44 -0500 Subject: [PATCH 61/62] wip Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 42 ++++++++++++++------------------ 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 0a871166933d..3b710ac18ece 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -78,8 +78,7 @@ func (s *SignatureAggregator) AggregateSignatures( return nil, nil, nil, false, fmt.Errorf("failed to marshal signature request: %w", err) } - publicKeysToValidators := make(map[*bls.PublicKey]indexedValidator) - nodeIDsToPublicKeys := make(map[ids.NodeID]*bls.PublicKey) + nodeIDsToValidator := make(map[ids.NodeID]indexedValidator) // TODO expose concrete type to avoid type casting bitSetSignature, ok := message.Signature.(*warp.BitSetSignature) if !ok { @@ -88,31 +87,31 @@ func (s *SignatureAggregator) AggregateSignatures( signerBitSet := set.BitsFromBytes(bitSetSignature.Signers) - nonSigners := make([]*bls.PublicKey, 0, len(validators)) + nonSigners := make([]ids.NodeID, 0, len(validators)) aggregatedStakeWeight := new(big.Int) totalStakeWeight := new(big.Int) numRequests := 0 - for i, v := range validators { - totalStakeWeight.Add(totalStakeWeight, new(big.Int).SetUint64(v.Weight)) + for i, validator := range validators { + totalStakeWeight.Add(totalStakeWeight, new(big.Int).SetUint64(validator.Weight)) // Only try to aggregate signatures from validators that are not already in // the signer bit set if signerBitSet.Contains(i) { - aggregatedStakeWeight.Add(aggregatedStakeWeight, new(big.Int).SetUint64(v.Weight)) + aggregatedStakeWeight.Add(aggregatedStakeWeight, new(big.Int).SetUint64(validator.Weight)) continue } - publicKeysToValidators[v.PublicKey] = indexedValidator{ + v := indexedValidator{ Index: i, - Validator: v, + Validator: validator, } for _, nodeID := range v.NodeIDs { numRequests += 1 - nodeIDsToPublicKeys[nodeID] = v.PublicKey + nodeIDsToValidator[nodeID] = v } - nonSigners = append(nonSigners, v.PublicKey) + nonSigners = append(nonSigners, v.NodeIDs...) } // Account for requested signatures + the signature that was provided @@ -127,17 +126,13 @@ func (s *SignatureAggregator) AggregateSignatures( results := make(chan result) handler := responseHandler{ - message: message, - publicKeysToValidators: publicKeysToValidators, - nodeIDsToPublicKeys: nodeIDsToPublicKeys, - results: results, + message: message, + nodeIDsToValidators: nodeIDsToValidator, + results: results, } - for _, pk := range nonSigners { - validator := publicKeysToValidators[pk] - if err := s.client.AppRequest(ctx, set.Of(validator.NodeIDs...), requestBytes, handler.HandleResponse); err != nil { - return nil, nil, nil, false, fmt.Errorf("failed to send aggregation request: %w", err) - } + if err := s.client.AppRequest(ctx, set.Of(nonSigners...), requestBytes, handler.HandleResponse); err != nil { + return nil, nil, nil, false, fmt.Errorf("failed to send aggregation request: %w", err) } minThreshold := new(big.Int).Mul(totalStakeWeight, new(big.Int).SetUint64(quorumNum)) @@ -219,10 +214,9 @@ func newWarpMessage( } type responseHandler struct { - message *warp.Message - publicKeysToValidators map[*bls.PublicKey]indexedValidator - nodeIDsToPublicKeys map[ids.NodeID]*bls.PublicKey - results chan result + message *warp.Message + nodeIDsToValidators map[ids.NodeID]indexedValidator + results chan result } func (r *responseHandler) HandleResponse( @@ -231,7 +225,7 @@ func (r *responseHandler) HandleResponse( responseBytes []byte, err error, ) { - validator := r.publicKeysToValidators[r.nodeIDsToPublicKeys[nodeID]] + validator := r.nodeIDsToValidators[nodeID] if err != nil { r.results <- result{NodeID: nodeID, Validator: validator, Err: err} return From 8c88acf81af6bf65dd24db61c791a5b1469020c2 Mon Sep 17 00:00:00 2001 From: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> Date: Fri, 6 Dec 2024 14:41:46 -0500 Subject: [PATCH 62/62] nit Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com> --- network/p2p/acp118/aggregator.go | 9 ++++++--- network/p2p/acp118/aggregator_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/network/p2p/acp118/aggregator.go b/network/p2p/acp118/aggregator.go index 3b710ac18ece..78bbfe4c5b1a 100644 --- a/network/p2p/acp118/aggregator.go +++ b/network/p2p/acp118/aggregator.go @@ -90,7 +90,6 @@ func (s *SignatureAggregator) AggregateSignatures( nonSigners := make([]ids.NodeID, 0, len(validators)) aggregatedStakeWeight := new(big.Int) totalStakeWeight := new(big.Int) - numRequests := 0 for i, validator := range validators { totalStakeWeight.Add(totalStakeWeight, new(big.Int).SetUint64(validator.Weight)) @@ -107,7 +106,6 @@ func (s *SignatureAggregator) AggregateSignatures( } for _, nodeID := range v.NodeIDs { - numRequests += 1 nodeIDsToValidator[nodeID] = v } @@ -138,7 +136,11 @@ func (s *SignatureAggregator) AggregateSignatures( minThreshold := new(big.Int).Mul(totalStakeWeight, new(big.Int).SetUint64(quorumNum)) minThreshold.Div(minThreshold, new(big.Int).SetUint64(quorumDen)) - for i := 0; i < numRequests; i++ { + // Block until: + // 1. The context is cancelled + // 2. We get responses from all validators + // 3. The specified security threshold is reached + for i := 0; i < len(nonSigners); i++ { select { case <-ctx.Done(): // Try to return whatever progress we have if the context is cancelled @@ -158,6 +160,7 @@ func (s *SignatureAggregator) AggregateSignatures( continue } + // Validators may share public keys so drop any duplicate signatures if signerBitSet.Contains(result.Validator.Index) { s.log.Debug( "dropping duplicate signature", diff --git a/network/p2p/acp118/aggregator_test.go b/network/p2p/acp118/aggregator_test.go index f678e00487ee..573f8f79f5b3 100644 --- a/network/p2p/acp118/aggregator_test.go +++ b/network/p2p/acp118/aggregator_test.go @@ -294,15 +294,15 @@ func TestSignatureAggregator_AggregateSignatures(t *testing.T) { }, { PublicKey: pk2, - Weight: 3, + Weight: 7, NodeIDs: []ids.NodeID{nodeID2}, }, }, - wantTotalStake: 6, + wantTotalStake: 10, wantSigners: 2, wantFinished: true, - quorumNum: 1, - quorumDen: 3, + quorumNum: 4, + quorumDen: 10, }, { name: "multiple validators - shared public keys",