Implement ACP-118 Aggregator #3394

joshua-kim · 2024-09-17T16:08:37Z

Why this should be merged

Implements p2p client + server logic for signature request handling as described in acp-118 (ref).

How this works

Client:

Introduces an aggregator type which will make signature requests in batches block until a caller-provided threshold of stake signs the provided warp message
Introduces a server handler that servers aggregation requests

How this was tested

Added unit tests

Signed-off-by: Joshua Kim <[email protected]>

network/p2p/acp118/aggregator.go

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim · 2024-11-20T20:05:43Z

network/p2p/p2ptest/client.go


+// NewClientWithPeers generates a client to communicate to a set of peers
+func NewClientWithPeers(


I can make this a separate PR if requested - but I've gotten feedback in the past about PRs with test utilities where it might be hard to understand why it's needed without corresponding usage.

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim · 2024-11-20T20:35:33Z

network/p2p/p2ptest/client.go

+		for nodeID := range nodeIDs {
+			network, ok := peerNetworks[nodeID]
+			if !ok {
+				return fmt.Errorf("%s is not connected", nodeID)


This is the reason for the big testing diff... the test utility now enforces that you're sending requests to a node registered in the peer map. We could also just drop the requests instead of erroring as an alternative.

Shouldn't this drop the requests since normally an error from the sender would be treated as a fatal error?

Signed-off-by: Joshua Kim <[email protected]>

aaronbuchwald · 2024-11-20T20:54:42Z

network/p2p/acp118/aggregator.go

+		sampleable = append(sampleable, v.NodeID)
+	}
+
+	signatures := make([]*bls.Signature, 0, len(sampleable)+1)


Ah I see we do +1 here to account for the original signature

I can leave a comment on this

aaronbuchwald · 2024-11-20T21:52:38Z

network/p2p/acp118/aggregator.go

+			if err := s.client.AppRequest(ctx, set.Of(nodeIDCopy), requestBytes, job.HandleResponse); err != nil {
+				results <- result{Validator: nodeIDsToValidator[nodeIDCopy], Err: err}
+				return


Should we remove goroutines since the p2p client is non-blocking (added ref from another PR today: ava-labs/hypersdk#1801 (comment) ) ?

aaronbuchwald · 2024-11-20T21:57:29Z

network/p2p/acp118/aggregator.go

+	}
+
+	failedStakeWeight := uint64(0)
+	minThreshold := (totalStakeWeight * quorumNum) / quorumDen


Should this match https://github.com/ava-labs/avalanchego/blob/master/vms/platformvm/warp/signature.go#L150 exactly?

aaronbuchwald · 2024-11-20T22:01:40Z

network/p2p/acp118/aggregator_test.go

+				&warp.BitSetSignature{Signature: [bls.SignatureLen]byte{}},
+			)
+			require.NoError(err)
+			gotMsg, gotNum, gotDen, err := aggregator.AggregateSignatures(


nit: should these be aggregatedSignatureWeight and totalWeight rather than num and den which suggests numerator/denominator?

Yeah these names make a lot more sense

aaronbuchwald · 2024-11-20T22:02:04Z

network/p2p/acp118/aggregator_test.go

+			wantNum := uint64(0)
+			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)


Same naming comment on numerator/denominator here

aaronbuchwald · 2024-11-20T22:02:40Z

network/p2p/acp118/aggregator_test.go

+		{
+			name: "aggregates from all validators 1/1",
+			peers: map[ids.NodeID]p2p.Handler{
+				nodeID0: NewHandler(&testVerifier{}, signer0),
+			},
+			ctx: context.Background(),
+			validators: []Validator{
+				{
+					NodeID:    nodeID0,
+					PublicKey: pk0,
+					Weight:    1,
+				},
+			},
+			wantSigners: []int{0},
+			quorumNum:   1,
+			quorumDen:   1,
+		},


Super easy to read these test cases ❤️

aaronbuchwald · 2024-11-20T22:04:57Z

network/p2p/acp118/aggregator_test.go

+			quorumDen:   1,
+		},
+		{
+			name: "aggregates from some validators - 1/3",


Would it make sense to make these names a little more descriptive to the edge case that they're testing?

ex.

Suggested change

name: "aggregates from some validators - 1/3",

name: "aggregates from min threshold - 1/3",

Reading through the rest, this is probably fine as is, since there's already a convention to these names for success/failure, could just be more explicit in the success case

aaronbuchwald · 2024-11-20T22:08:22Z

network/p2p/acp118/aggregator_test.go

+			quorumDen:   3,
+		},
+		{
+			name: "aggregates from some validators - 2/3",


What's the intended difference for this test case? Just > 1 success rather than > 1 failure but still meeting minimum threshold?

It seems each success test case is meeting the exact required threshold. This is very well tested as is, but could also add cases for reaching greater than minimum threshold.

aaronbuchwald · 2024-11-20T22:17:20Z

network/p2p/p2ptest/client.go

+		for nodeID := range nodeIDs {
+			network, ok := peerNetworks[nodeID]
+			if !ok {
+				return fmt.Errorf("%s is not connected", nodeID)


Shouldn't this drop the requests since normally an error from the sender would be treated as a fatal error?

network/p2p/acp118/aggregator.go

StephenButtolph · 2024-11-20T15:12:22Z

network/p2p/acp118/aggregator.go

+	aggregatedStakeWeight := uint64(0)
+	totalStakeWeight := uint64(0)
+	for i, v := range validators {
+		totalStakeWeight += v.Weight


We should probably protect against overflow here. (It is possible for this to overflow with a real subnet)

network/p2p/acp118/aggregator.go

StephenButtolph · 2024-11-20T22:57:00Z

network/p2p/acp118/aggregator.go

+	}
+
+	failedStakeWeight := uint64(0)
+	minThreshold := (totalStakeWeight * quorumNum) / quorumDen


totalStakeWeight * quorumNum can overflow

network/p2p/acp118/aggregator.go

StephenButtolph · 2024-11-20T23:04:54Z

network/p2p/acp118/aggregator.go

+				// 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


Does this work with hypersdk's expected usage here? I thought that num/dem were going to be the maximum weights it would wait for, but that the minimum would be lower than that (meaning that if we are passing in the max here, we could be terminating when we realize we can't get the maximum... But we actually could have gotten the number that hypersdk wanted).

Maybe it makes more sense for us to change the behavior so that this api blocks until all responses come back, or we reach the provided num/den threshold instead of failing instead.

StephenButtolph · 2024-11-20T23:06:50Z

network/p2p/acp118/aggregator.go

+	if !bls.Verify(validator.PublicKey, signature, r.message.UnsignedMessage.Bytes()) {
+		r.results <- result{Validator: validator, Err: errFailedVerification}
+		return
+	}


StephenButtolph · 2024-11-20T23:15:49Z

network/p2p/acp118/aggregator.go

+// AggregateSignatures blocks until quorumNum/quorumDen signatures from
+// 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,
+	justification []byte,
+	validators []Validator,
+	quorumNum uint64,
+	quorumDen uint64,
+) (*warp.Message, uint64, uint64, error) {


I don't think this correctly handles the case that BLS public keys are shared across validators.

In Warp, only 1 signature is ever allowed from a BLS key in a warp message. If different nodeIDs have the same BLS key, their weights are aggregated for the BLS key's index

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim changed the base branch from master to p2p-sync September 17, 2024 16:08

joshua-kim force-pushed the p2p-sync branch from 2e59a5d to e7648e5 Compare September 17, 2024 16:09

joshua-kim force-pushed the acp-118 branch from fa51ef2 to 17f3d66 Compare September 17, 2024 16:22

joshua-kim self-assigned this Sep 17, 2024

joshua-kim force-pushed the acp-118 branch 6 times, most recently from 368ad1a to 2a47036 Compare September 18, 2024 15:30

joshua-kim force-pushed the p2p-sync branch from 19d8f83 to 3af3bc9 Compare September 24, 2024 14:52

Base automatically changed from p2p-sync to master September 25, 2024 16:58

joshua-kim mentioned this pull request Oct 1, 2024

ACP-118: Implement p2p handler #3434

Merged

joshua-kim force-pushed the acp-118 branch 2 times, most recently from 6e6e88f to cf4ecba Compare October 1, 2024 18:19

joshua-kim changed the title ~~Implement ACP-118 Package~~ Implement ACP-118 Aggregator Oct 22, 2024

joshua-kim force-pushed the acp-118 branch 2 times, most recently from b4d7b35 to 60761f8 Compare October 22, 2024 20:14

joshua-kim added 5 commits November 12, 2024 16:05

implement acp-118 signature aggregation

7c6d0a7

Signed-off-by: Joshua Kim <[email protected]>

undo

11ada0f

Signed-off-by: Joshua Kim <[email protected]>

nit

e650f5d

Signed-off-by: Joshua Kim <[email protected]>

nit

81c9487

Signed-off-by: Joshua Kim <[email protected]>

improve usage of context

0760fcd

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim force-pushed the acp-118 branch from 60761f8 to 0760fcd Compare November 12, 2024 21:39

joshua-kim added 2 commits November 12, 2024 16:45

improve doc

b6cfd0e

Signed-off-by: Joshua Kim <[email protected]>

nit

dacd131

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim marked this pull request as ready for review November 12, 2024 21:48

aaronbuchwald reviewed Nov 13, 2024

View reviewed changes

network/p2p/acp118/aggregator.go Outdated Show resolved Hide resolved

network/p2p/acp118/aggregator.go Outdated Show resolved Hide resolved

network/p2p/acp118/aggregator.go Outdated Show resolved Hide resolved

network/p2p/acp118/aggregator.go Outdated Show resolved Hide resolved

joshua-kim added 2 commits November 13, 2024 17:16

rename i -> index

810fb55

Signed-off-by: Joshua Kim <[email protected]>

nit

6f6c0b5

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim added 6 commits November 19, 2024 03:07

nit

aa5da47

Signed-off-by: Joshua Kim <[email protected]>

nit

a44b653

Signed-off-by: Joshua Kim <[email protected]>

nit

26bb1a0

Signed-off-by: Joshua Kim <[email protected]>

nit

d2139cc

Signed-off-by: Joshua Kim <[email protected]>

nit

4868a99

Signed-off-by: Joshua Kim <[email protected]>

nit

c5aaebb

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim requested review from cam-schultz and aaronbuchwald November 19, 2024 21:52

joshua-kim added 7 commits November 19, 2024 16:55

nit

fecd189

Signed-off-by: Joshua Kim <[email protected]>

nit

3b23a22

Signed-off-by: Joshua Kim <[email protected]>

nit

420253d

Signed-off-by: Joshua Kim <[email protected]>

nit

4c4c380

Signed-off-by: Joshua Kim <[email protected]>

nit

738b0ce

Signed-off-by: Joshua Kim <[email protected]>

nit

455e03e

Signed-off-by: Joshua Kim <[email protected]>

nit

7d7c3c0

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim commented Nov 20, 2024

View reviewed changes

joshua-kim added 5 commits November 20, 2024 15:07

nit

f989cf5

Signed-off-by: Joshua Kim <[email protected]>

nit

900279d

Signed-off-by: Joshua Kim <[email protected]>

nit

de7b65c

Signed-off-by: Joshua Kim <[email protected]>

nit

7c4fa96

Signed-off-by: Joshua Kim <[email protected]>

fix tests

9802ab2

Signed-off-by: Joshua Kim <[email protected]>

joshua-kim requested a review from StephenButtolph as a code owner November 20, 2024 20:33

joshua-kim commented Nov 20, 2024

View reviewed changes

nit

8b6563a

Signed-off-by: Joshua Kim <[email protected]>

aaronbuchwald reviewed Nov 20, 2024

View reviewed changes

StephenButtolph reviewed Nov 20, 2024

View reviewed changes

joshua-kim and others added 4 commits November 21, 2024 12:22

Update network/p2p/acp118/aggregator.go

4480b48

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>

Update network/p2p/acp118/aggregator.go

67f533e

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>

Update network/p2p/acp118/aggregator.go

2a9621b

Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Joshua Kim <[email protected]>

move nil signatures check

3773575

Signed-off-by: Joshua Kim <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ACP-118 Aggregator #3394

Implement ACP-118 Aggregator #3394

joshua-kim commented Sep 17, 2024 •

edited

Loading

joshua-kim Nov 20, 2024 •

edited

Loading

joshua-kim Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

joshua-kim Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

joshua-kim Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

aaronbuchwald Nov 20, 2024

StephenButtolph Nov 20, 2024

StephenButtolph Nov 20, 2024

StephenButtolph Nov 20, 2024

joshua-kim Nov 21, 2024 •

edited

Loading

StephenButtolph Nov 20, 2024

StephenButtolph Nov 20, 2024


		// NewClientWithPeers generates a client to communicate to a set of peers
		func NewClientWithPeers(

	name: "aggregates from some validators - 1/3",
	name: "aggregates from min threshold - 1/3",

Implement ACP-118 Aggregator #3394

Are you sure you want to change the base?

Implement ACP-118 Aggregator #3394

Conversation

joshua-kim commented Sep 17, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

joshua-kim Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim commented Sep 17, 2024 •

edited

Loading

joshua-kim Nov 20, 2024 •

edited

Loading

joshua-kim Nov 21, 2024 •

edited

Loading