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.

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

network: add P2PNetwork implementation #5640

Merged
merged 37 commits into from
Aug 25, 2023
Merged

network: add P2PNetwork implementation #5640

merged 37 commits into from
Aug 25, 2023

Conversation

cce
Copy link
Contributor

@cce cce commented Aug 7, 2023

Summary

Following up on #5634, this adds a new GossipNode implementation (P2PNetwork) which integrates with the libp2p support packages in network/p2p.

Test Plan

Added new tests TestP2PSubmitTX and TestP2PSubmitWS, also tested nodes connecting with each other manually using goal node start -p. Added a basic 10-round test in test/e2e-go/features/p2p package along with 2-, 3-, and 5-node templates to start E2E testing.

@cce cce changed the title network: add initial P2PNetwork implementation network: add P2PNetwork implementation Aug 7, 2023
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #5640 (2935d57) into master (828c1df) will decrease coverage by 0.12%.
Report is 3 commits behind head on master.
The diff coverage is 43.28%.

@@            Coverage Diff             @@
##           master    #5640      +/-   ##
==========================================
- Coverage   55.47%   55.35%   -0.12%     
==========================================
  Files         466      473       +7     
  Lines       65642    66075     +433     
==========================================
+ Hits        36413    36579     +166     
- Misses      26785    27039     +254     
- Partials     2444     2457      +13     
Files Changed Coverage Δ
cmd/algod/main.go 0.00% <0.00%> (ø)
cmd/goal/node.go 12.46% <0.00%> (ø)
config/localTemplate.go 70.76% <ø> (ø)
netdeploy/networkTemplate.go 40.58% <0.00%> (-2.27%) ⬇️
netdeploy/remote/nodeConfig.go 0.00% <ø> (ø)
network/p2p/p2p.go 0.00% <0.00%> (ø)
network/p2p/streams.go 0.00% <0.00%> (ø)
node/node.go 3.68% <6.25%> (-0.47%) ⬇️
network/p2p/pubsub.go 20.75% <20.75%> (ø)
network/p2p/peerID.go 71.87% <54.54%> (-12.13%) ⬇️
... and 5 more

... and 9 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce cce added the Enhancement label Aug 7, 2023
@algorandskiy algorandskiy added New Feature p2p Work related to the p2p project and removed Enhancement labels Aug 7, 2023
network/p2p/p2p.go Outdated Show resolved Hide resolved
network/p2p/p2p.go Outdated Show resolved Hide resolved
network/p2p/p2p.go Outdated Show resolved Hide resolved
network/p2p/peerstore/peerstore.go Outdated Show resolved Hide resolved
network/p2p/pubsub.go Show resolved Hide resolved
cmd/algod/main.go Show resolved Hide resolved
Copy link
Contributor

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

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

Nice start, I think this will get us most of the way to an initial test!

Some general comments at this point.

  • I think we should remove all of the Todo comments. Let's capture them in issues if we need to, and make sure things are relatively clean as we commit them.
  • Needs more test coverage.
  • Might be helpful to commit alongside this some readme or diagram which explains how things are structured in terms of the different gossipsub topics/what they are used for, and the things we're running through streams.

tools/block-generator/go.mod Show resolved Hide resolved
config/local_defaults.go Show resolved Hide resolved
network/wsNetwork.go Outdated Show resolved Hide resolved
network/wsNetwork.go Show resolved Hide resolved
network/p2p/p2p.go Outdated Show resolved Hide resolved
network/p2p/pubsub.go Show resolved Hide resolved
network/p2p/pubsub.go Show resolved Hide resolved
@pao-beep
Copy link

I support a diagram, I hope it's public

@iansuvak
Copy link
Contributor

Remerged master and disabled the FiveNode E2E test. It's currently failing because of a timing issue that nodes are unable to recover from because HTTP block service is not yet implemented. This will be solved in a future PR.

algorandskiy
algorandskiy previously approved these changes Aug 23, 2023
test/e2e-go/features/p2p/p2p_basic_test.go Outdated Show resolved Hide resolved
Co-authored-by: Pavel Zbitskiy <65323360+algorandskiy@users.noreply.github.com>
@cce
Copy link
Contributor Author

cce commented Aug 24, 2023

@Eric-Warehime @pao-beep added a README with a diagram

shiqizng
shiqizng previously approved these changes Aug 24, 2023
iansuvak
iansuvak previously approved these changes Aug 24, 2023
Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

Approving as a second reviewer since @algorandskiy approved recently and the only change since was the readme and a single line suggestion by him.

@cce cce dismissed stale reviews from iansuvak and shiqizng via 8087181 August 24, 2023 18:28
iansuvak
iansuvak previously approved these changes Aug 25, 2023
shiqizng
shiqizng previously approved these changes Aug 25, 2023
algorandskiy
algorandskiy previously approved these changes Aug 25, 2023
test/e2e-go/features/p2p/p2p_basic_test.go Outdated Show resolved Hide resolved
@cce cce dismissed stale reviews from algorandskiy, shiqizng, and iansuvak via 0dd20aa August 25, 2023 15:36
Copy link
Contributor

@iansuvak iansuvak left a comment

Choose a reason for hiding this comment

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

LGTM. I like the move of general code gossipNode.go

@cce cce merged commit b19594e into algorand:master Aug 25, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature p2p Work related to the p2p project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants