Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add p2p.Network component #2283

Merged
merged 39 commits into from
Nov 30, 2023
Merged

Add p2p.Network component #2283

merged 39 commits into from
Nov 30, 2023

Conversation

joshua-kim
Copy link
Contributor

@joshua-kim joshua-kim commented Nov 9, 2023 •
edited
Loading

Why this should be merged

RegisterAppProtocol in the Router currently (and in the future will likely have more) allows the caller to configure the Client to make requests with. Because the Router only knows about user-defined handlers, it doesn't have a great way of offering default values if a Client isn't configured with certain values.

This PR introduces a Network component that wraps a Router and tracks network state. Clients that don't use default options values will have their defaults derived from the Network struct.

How this works

Changes:

  • Router is now un-exported, people should implement p2p networking logic by wrapping Network instead.
  • Peers is now un-exported, it's offered as a default in Network.
  • RegisterAppProtocol now takes in a variadic list of options that can configure a Client instead of allowing callers to pass in nil.

Coreth PR: ava-labs/coreth#384

How this was tested

UTs

@joshua-kim joshua-kim self-assigned this Nov 9, 2023
@joshua-kim joshua-kim added the networking This involves networking label Nov 9, 2023
@joshua-kim joshua-kim marked this pull request as ready for review November 9, 2023 17:18
joshua-kim
joshua-kim commented Nov 9, 2023
View reviewed changes
network/p2p/network.go Outdated Show resolved Hide resolved
@joshua-kim
p2p network
c073533 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
add test
ec6b9f9 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
b84be08 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
74765cb 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
go mod
c54c7d5 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
refactor validators
f61113e 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
f377a8b 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
2995647 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
accd487 
Signed-off-by: Joshua Kim <[email protected]>
joshua-kim
joshua-kim commented Nov 15, 2023
View reviewed changes
network/p2p/peers_test.go Outdated Show resolved Hide resolved
joshua-kim
joshua-kim commented Nov 15, 2023
View reviewed changes
network/p2p/peers.go Outdated Show resolved Hide resolved
joshua-kim
joshua-kim commented Nov 15, 2023
View reviewed changes
network/p2p/validators.go
@@ -22,12 +22,18 @@ var (
)

type ValidatorSet interface {
Has(ctx context.Context, nodeID ids.NodeID) bool
Has(ctx context.Context, nodeID ids.NodeID) bool // TODO return error
Copy link
Contributor Author

@joshua-kim joshua-kim Nov 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In hindsight logging the error instead of bubbling it up was a bad idea.

joshua-kim
joshua-kim commented Nov 15, 2023
View reviewed changes
network/p2p/validators.go
@@ -71,20 +78,33 @@ func (v *Validators) refresh(ctx context.Context) {
v.lastUpdated = time.Now()
}

// Sample returns a random sample of connected validators
func (v *Validators) Sample(ctx context.Context, limit int) []ids.NodeID {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also consider resampling if the validator we get is disconnected

@joshua-kim
nit
be9e25a 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim joshua-kim linked an issue Nov 15, 2023 that may be closed by this pull request
Refactor SDK Clients to accept variadic options parameters #2126
Closed
StephenButtolph
StephenButtolph reviewed Nov 26, 2023
View reviewed changes
network/p2p/network.go Outdated Show resolved Hide resolved
network/p2p/network_test.go Outdated Show resolved Hide resolved
network/p2p/network.go Outdated Show resolved Hide resolved
network/p2p/validators.go Outdated Show resolved Hide resolved
network/p2p/network_test.go Outdated Show resolved Hide resolved
network/p2p/validators_test.go Show resolved Hide resolved
network/p2p/validators.go Outdated Show resolved Hide resolved
network/p2p/network.go Outdated Show resolved Hide resolved
@joshua-kim joshua-kim linked an issue Nov 28, 2023 that may be closed by this pull request
Document p2p.Validators #1998
Open
@joshua-kim
fix
3cc6218 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
af2740d 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
e2781cf 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
f512ead 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nti
741f612 
Signed-off-by: Joshua Kim <[email protected]>
StephenButtolph
StephenButtolph reviewed Nov 29, 2023
View reviewed changes
network/p2p/router.go Outdated Show resolved Hide resolved
network/p2p/validators.go Outdated Show resolved Hide resolved
joshua-kim and others added 3 commits November 29, 2023 15:36
@joshua-kim @StephenButtolph
Update network/p2p/validators.go
bfa0638 
Co-authored-by: Stephen Buttolph <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
nit
4701b31 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
Merge branch 'dev' into network
3a8cca5
StephenButtolph
StephenButtolph approved these changes Nov 29, 2023
View reviewed changes
go.mod Outdated Show resolved Hide resolved
joshua-kim and others added 3 commits November 29, 2023 16:36
@joshua-kim
nit
ee0eb08 
Signed-off-by: Joshua Kim <[email protected]>
@joshua-kim
tidy
5fde830 
Signed-off-by: Joshua Kim <[email protected]>
@StephenButtolph
Merge branch 'dev' into network
6870966
@joshua-kim
Copy link
Contributor Author

Pending approval on coreth: ava-labs/coreth#384

@StephenButtolph StephenButtolph added this to the v1.10.17 milestone Nov 30, 2023
marun
marun approved these changes Nov 30, 2023
View reviewed changes
Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

(No action required) I think there may be room for improvement on the testing front - validating corner-cases at this level of abstraction would seem to require too many moving pieces to be easily reasoned about.

@StephenButtolph StephenButtolph added this pull request to the merge queue Nov 30, 2023
Merged via the queue into dev with commit de3b16c Nov 30, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the network branch November 30, 2023 18:12
@joshua-kim joshua-kim mentioned this pull request Dec 4, 2023
Closed
joshua-kim added a commit that referenced this pull request Dec 6, 2023
@joshua-kim
Squashed commit of the following:
8866972 
commit b36416d
Author: Alberto Benegiamo <[email protected]>
Date:   Wed Dec 6 12:12:11 2023 -0700

    Drop Pending Stakers 1 - introduced ScheduledStaker txs (#2323)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 7df1f3a
Author: Stephen Buttolph <[email protected]>
Date:   Wed Dec 6 13:46:56 2023 -0500

    Restrict GOPROXY (#2434)

commit 21b7ab8
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 19:43:00 2023 -0500

    Fix platformvm.SetPreference (#2429)

commit ada692a
Author: Stephen Buttolph <[email protected]>
Date:   Tue Dec 5 17:40:03 2023 -0500

    Update minimum golang version to v1.20.12 (#2427)

commit 004a23e
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 12:56:25 2023 -0500

    `vms/platformvm`: Add `decisionTxs` parameter to `NewBanffProposalBlock` (#2411)

    Co-authored-by: Dan Laine <[email protected]>

commit 439dc1e
Author: Alberto Benegiamo <[email protected]>
Date:   Tue Dec 5 10:46:13 2023 -0700

    ProposerVM Extend windows 0 - Cleanup (#2404)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 477157d
Author: Dhruba Basu <[email protected]>
Date:   Tue Dec 5 11:10:56 2023 -0500

    `vms/platformvm`: Cleanup some block tests (#2422)

    Co-authored-by: Dan Laine <[email protected]>

commit b6700c9
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 22:24:34 2023 -0500

    Update slices dependency to use Compare (#2424)

    Co-authored-by: James Walker <[email protected]>

commit 5d9e482
Author: Dan Laine <[email protected]>
Date:   Mon Dec 4 19:27:29 2023 -0500

    allow user of `build_fuzz.sh` to specify a directory to fuzz in (#2414)

commit 2e32281
Author: Stephen Buttolph <[email protected]>
Date:   Mon Dec 4 18:30:41 2023 -0500

    Fix duplicated bootstrapper engine termination (#2334)

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

commit b741c19
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:49:15 2023 -0500

    `vms/platformvm`: Move `VerifyUniqueInputs` from `verifier` to `backend` (#2410)

commit 6aa20fc
Author: Dhruba Basu <[email protected]>
Date:   Mon Dec 4 17:32:07 2023 -0500

    `vms/platformvm`: Initialize txs in `Transactions` field for `BanffProposalBlock` (#2419)

commit c11accd
Author: Alberto Benegiamo <[email protected]>
Date:   Mon Dec 4 09:16:01 2023 -0700

    Drop Pending Stakers 0 - De-duplicate staking tx verification (#2335)

commit 05ce366
Author: marun <[email protected]>
Date:   Sat Dec 2 13:33:18 2023 -0800

    testing: Update to latest version of ginkgo (#2390)

commit 04af33e
Author: Dhruba Basu <[email protected]>
Date:   Sat Dec 2 16:33:07 2023 -0500

    `vms/platformvm`: Cleanup block builder tests (#2406)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 7623ffd
Author: Stephen Buttolph <[email protected]>
Date:   Fri Dec 1 19:39:15 2023 -0500

    Update versions for v1.10.17 (#2394)

commit be1a2ad
Author: aaronbuchwald <[email protected]>
Date:   Fri Dec 1 13:31:49 2023 -0500

    Add more descriptive formatted error (#2403)

    Co-authored-by: Stephen Buttolph <[email protected]>

commit 9b85141
Author: Dhruba Basu <[email protected]>
Date:   Thu Nov 30 12:42:36 2023 -0800

    `vms/platformvm`: Move `GetRewardUTXOs`, `GetSubnets`, and `GetChains` to `State` interface (#2402)

commit de3b16c
Author: Joshua Kim <[email protected]>
Date:   Thu Nov 30 12:49:53 2023 -0500

    Add `p2p.Network` component (#2283)

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

commit 0ab2046
Author: marun <[email protected]>
Date:   Thu Nov 30 09:18:59 2023 -0800

    Rename `testnet` fixture to `tmpnet` (#2307)

commit 907b34c
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 21:32:43 2023 -0500

    Update bootstrap IPs (#2396)

commit 96d451d
Author: Stephen Buttolph <[email protected]>
Date:   Wed Nov 29 18:15:47 2023 -0500

    Periodically PullGossip only from connected validators (#2399)

commit 0da5bcc
Author: Dan Laine <[email protected]>
Date:   Wed Nov 29 16:32:16 2023 -0500

    Remove method `CappedList` from `set.Set` (#2395)

Signed-off-by: Joshua Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@marun marun marun approved these changes

@danlaine danlaine Awaiting requested review from danlaine

Assignees

@joshua-kim joshua-kim

Labels
networking This involves networking
Projects
Platform Engineering Group
Archived in project
Milestone
v1.10.17
Development

Successfully merging this pull request may close these issues.

Document p2p.Validators Refactor SDK Clients to accept variadic options parameters
3 participants
@joshua-kim @marun @StephenButtolph