Add SDK Sampling interface #1877

joshua-kim · 2023-08-17T20:44:33Z

Why this should be merged

Allow the SDK clients to configure their node sampling strategy for AppRequestAny.

How this works

Moves the peer state in Router (which was questionable anyways) to an implementation of an interface. We now expose two different implementations, one to sample any peer and one to sample strictly validators. The VM is expected to keep a reference to a set of samplers, and re-use them for any clients it might create.

How this was tested

Added some unit tests.

network/p2p/client.go

network/p2p/node_sampler.go

hexfusion · 2023-08-21T12:29:26Z

network/p2p/node_sampler.go

+type NodeSampler interface {
+	// Sample returns at most [n] nodes. This may return fewer nodes if fewer
+	// than [n] are available.
+	Sample(n int) ([]ids.NodeID, error)


Shouldn't Sample take a context? You are already needing to stub out TODO getRecentValidatorIDs.

Can you give an example of a sample that would return an error?

Shouldn't Sample take a context? You are already needing to stub out TODO getRecentValidatorIDs.

Yeah this makes sense, I can add this to the signature

Can you give an example of a sample that would return an error?

Hmm I guess an example would be some grpc failure? Some implementation might hit the p-chain for example

Actually, the only reason we have this Sample interface is to use it for the Client node selection implementation. We could actually swallow this error and just return an empty slice of node ids, and have the Client report an error if it sees no nodes to send to, so the error is still visible and this interface signature is much cleaner since we can get rid of the error here.

We could actually swallow this error and just return an empty slice of node ids, and have the Client report an error if it sees no nodes to send to,

If we error and return on calls to v.validators.GetCurrentHeight(ctx) or v.validators.GetValidatorSet(ctx, height, v.subnetID) we won't update v.validatorIDs so future sampling won't return an empty slice of node IDs, it'll just return stale node IDs. FWIW the sampler implementations in the sampler package have errors in their return signature.

I added a unit test for this specific scenario to make sure we return an empty slice instead of stale ids so the error is surfaced correctly in the Client.

network/p2p/node_sampler_test.go

network/p2p/router.go

network/p2p/router_test.go

network/p2p/router.go

hexfusion · 2023-08-21T13:18:36Z

network/p2p/node_sampler_test.go

+			}
+
+			for disconnected := range tt.disconnected {
+				require.NoError(sampler.Disconnected(context.Background(), disconnected))


why not use context.WithCancel for the test?

Hum can you elaborate? I didn't understand why this would be helpful here.

hexfusion · 2023-08-21T13:26:05Z

network/p2p/node_sampler.go

+	_ NodeSampler          = (*Peers)(nil)
+)
+
+type NodeSampler interface {


Is this NodeSampler or PeerSampler

Seems more like a node sampler to me. It samples nodes (which may or may not be connected)

Yeah my thinking was that the terminology we use are:

node: a member in the network

peer: a node we are connected to

validator: a staking node

so Peers is a component that represents nodes are are connected to, and Validators is a component that represents staking nodes.

network/p2p/node_sampler.go

network/p2p/node_sampler_test.go

network/p2p/node_sampler.go

Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

joshua-kim · 2023-08-22T06:12:26Z

network/p2p/node_sampler.go

+type NodeSampler interface {
+	// Sample returns at most [limit] nodes. This may return fewer nodes if
+	// fewer than [limit] are available.
+	Sample(ctx context.Context, limit int) []ids.NodeID


note: I also considered making another type to return here like:

type Node struct { ID ids.NodeID }

so we could add onto it without causing breaking changes. I felt like it was reasonable that in the future we might add some peer-level metadata to this, but it seemed premature to add for now.

network/p2p/validators.go

danlaine · 2023-08-22T13:39:11Z

network/p2p/validators.go

+	if err != nil {
+		return
+	}
+	validatorSet, err := v.validators.GetValidatorSet(ctx, height, v.subnetID)
+	if err != nil {
+		return
+	}


Hmm is there something we could do other than swallow these errors? Adding error return values seems kind of annoying but also I dislike swallowing errors. Thoughts?

Yeah it does seem strange. Here's a link to the rationale behind this: #1877 (comment)

danlaine · 2023-08-22T13:45:26Z

network/p2p/client.go

@@ -41,6 +41,7 @@ type Client struct {
 	handlerPrefix []byte


Not part of this PR but should we have a NewClient function? If this in intended to be used only inside this package, Client can be unexported. If it's intended to be used outside this package, then a user can't use Client because they can't set its inner fields.

This is created via RegisterAppProtocol. It's not possible to use a Client without a corresponding Router because the Router holds the map of pending requests -> the callbacks that need to be fired on response. We could theoretically refactor this into a NewClient(Router, handlerID, ....) if we felt that looked cleaner.

It just feels weird to export this if it's actually unusable outside this package

Client is usable outside of this package, RegisterAppProtocol initializes it here.

Maybe we could rename this to NewClient to be more verbose or remove the receiver on Router. This definitely might seem confusing coming from the existing paradigm of how we've developed VMs without the SDK component.

Ah I see. Yeah I think it might be a bit confusing to someone looking at this package how to create a new client. I think maybe even just a comment on Client might suffice.

Yeah I could see an argument to make the constructor code NewClient(sender common.AppSender, router *Router, handlerID byte) *Client. This would have the added benefit of not having to keep around the sender as a field in Router which we currently do.

network/p2p/client.go

network/p2p/validators_test.go

Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

joshua-kim requested review from danlaine, darioush, dboehm-avalabs, abi87, gyuho, hexfusion, StephenButtolph, coffeeavax and ceyonur as code owners August 17, 2023 20:44

joshua-kim changed the base branch from master to dev August 17, 2023 20:44

joshua-kim commented Aug 17, 2023

View reviewed changes

network/p2p/client.go Outdated Show resolved Hide resolved

add sampling interface

499a7f2

joshua-kim force-pushed the sdk-sampling branch from 7cbd5bd to 499a7f2 Compare August 17, 2023 20:52

StephenButtolph reviewed Aug 18, 2023

View reviewed changes

network/p2p/node_sampler.go Outdated Show resolved Hide resolved

joshua-kim added 4 commits August 18, 2023 16:54

cache validator sets

fb4573f

Merge branch 'dev' into sdk-sampling

b4ab267

use of

25d8e69

nit

203dbe6

joshua-kim self-assigned this Aug 18, 2023

joshua-kim added the sdk This involves SDK tooling or frameworks label Aug 18, 2023

joshua-kim added 3 commits August 18, 2023 17:05

nit

fc6b4fb

nit

77849bb

cleanup

cb1b78f

hexfusion reviewed Aug 21, 2023

View reviewed changes

StephenButtolph reviewed Aug 21, 2023

View reviewed changes

joshua-kim and others added 3 commits August 21, 2023 17:20

Update network/p2p/node_sampler.go

0af1473

Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

Update network/p2p/node_sampler_test.go

d140e94

Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

Update network/p2p/node_sampler_test.go

d437998

Co-authored-by: Stephen Buttolph <stephen@avalabs.org> Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>

joshua-kim mentioned this pull request Aug 21, 2023

SDK AppRequestAny should take in a parameter for number of nodes to make a request to #1868

Open

Merge branch 'dev' into sdk-sampling

ad542ec

joshua-kim added 7 commits August 21, 2023 22:55

nits

167ba94

nits

b8324d1

nit

2aa0d20

nit

1809aff

lol

da20c25

nit

956efba

nit

868893b

joshua-kim commented Aug 22, 2023

View reviewed changes

nit

ab45557

joshua-kim requested review from hexfusion and StephenButtolph and removed request for coffeeavax August 22, 2023 13:08

danlaine reviewed Aug 22, 2023

View reviewed changes

joshua-kim added 2 commits August 23, 2023 11:13

refactor unit test

0187c34

nits

0ae6659

joshua-kim requested a review from danlaine August 23, 2023 15:15

Merge branch 'dev' into sdk-sampling

38e482d

danlaine approved these changes Aug 23, 2023

View reviewed changes

Dan Laine and others added 2 commits August 23, 2023 12:10

Merge branch 'dev' into sdk-sampling

a9c62b6

nit

386f96a

StephenButtolph reviewed Aug 23, 2023

View reviewed changes

network/p2p/validators_test.go Outdated Show resolved Hide resolved

Update network/p2p/validators_test.go

af938d5

Signed-off-by: Stephen Buttolph <stephen@avalabs.org>

StephenButtolph approved these changes Aug 23, 2023

View reviewed changes

StephenButtolph added this to the v1.10.9 milestone Aug 23, 2023

darioush approved these changes Aug 23, 2023

View reviewed changes

StephenButtolph merged commit a3c4649 into ava-labs:dev Aug 23, 2023
15 checks passed

StephenButtolph mentioned this pull request Sep 21, 2023

Support sampling validators in SDK client #1869

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SDK Sampling interface #1877

Add SDK Sampling interface #1877

joshua-kim commented Aug 17, 2023 •

edited

Loading

hexfusion Aug 21, 2023

hexfusion Aug 21, 2023

joshua-kim Aug 21, 2023

joshua-kim Aug 22, 2023 •

edited

Loading

danlaine Aug 22, 2023

joshua-kim Aug 23, 2023

hexfusion Aug 21, 2023

joshua-kim Aug 22, 2023

hexfusion Aug 21, 2023

StephenButtolph Aug 21, 2023

joshua-kim Aug 21, 2023 •

edited

Loading

joshua-kim Aug 22, 2023 •

edited

Loading

danlaine Aug 22, 2023

joshua-kim Aug 22, 2023

danlaine Aug 22, 2023

joshua-kim Aug 22, 2023

danlaine Aug 22, 2023

joshua-kim Aug 22, 2023

danlaine Aug 22, 2023

joshua-kim Aug 22, 2023

Add SDK Sampling interface #1877

Add SDK Sampling interface #1877

Conversation

joshua-kim commented Aug 17, 2023 • edited Loading

Why this should be merged

How this works

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshua-kim Aug 22, 2023 • 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

joshua-kim Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

joshua-kim Aug 22, 2023 • 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

joshua-kim commented Aug 17, 2023 •

edited

Loading

joshua-kim Aug 22, 2023 •

edited

Loading

joshua-kim Aug 21, 2023 •

edited

Loading

joshua-kim Aug 22, 2023 •

edited

Loading