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

p2p: hybrid node net identity for connection deduplication #6035

Merged
merged 26 commits into from
Jul 19, 2024

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Jun 19, 2024

Summary

  1. Refactored net identity scheme constructors to allow hybrid net to create p2pNet and wsNet with predefined identity trackers and schema.
  2. Allowed multiple dedup names in identity tracker.
  3. Setting EnableP2PHybridMode also requires PublicAddress to be set on nodes listening NetAddress or P2PNetAddress
  4. Added hybrid p2p scenarios to netdeploy
  5. Removed gossipSubPeer type because of type cast in hybrid mode causing panic (see in comments)
  6. Fixed possible letter case mismatch of PublicAddress from config with netAddr fetched from DNS.

Test Plan

Added new unit tests and run cluster tests.

Pulled TODOs from #5939

  • cluster performance tests for hybrid mode
    Validated deduplication in logs, found and fixed PublicAddress vs DNS netAddr letter case issue.
    scenario1s half: 4R+4N+4NPN results:
    hybrid=p2p (all nodes p2p, some hybrid): 6244.04 TPS
    hybrid=ws (all nodes ws, some hybrid): 6225.84 TPS
    hybrid=no (all nodes p2p): 6244.78 TPS
    
  • connection/traffic limits are aggregated for both ws and p2p nets in hybrid mode

@algorandskiy algorandskiy self-assigned this Jun 19, 2024
@algorandskiy algorandskiy marked this pull request as ready for review June 20, 2024 15:47
@algorandskiy algorandskiy requested review from cce, jasonpaulos and gmalouf and removed request for cce June 20, 2024 15:47
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 72.28916% with 46 lines in your changes missing coverage. Please review.

Project coverage is 56.30%. Comparing base (1fa0ef7) to head (c3cd19c).

Files Patch % Lines
daemon/algod/server.go 0.00% 15 Missing ⚠️
network/p2p/logger.go 0.00% 12 Missing ⚠️
netdeploy/remote/deployedNetwork.go 0.00% 7 Missing ⚠️
network/p2pNetwork.go 80.00% 5 Missing and 2 partials ⚠️
config/config.go 77.77% 1 Missing and 1 partial ⚠️
network/p2p/p2p.go 88.23% 1 Missing and 1 partial ⚠️
network/wsNetwork.go 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6035      +/-   ##
==========================================
+ Coverage   56.12%   56.30%   +0.17%     
==========================================
  Files         488      488              
  Lines       69474    69581     +107     
==========================================
+ Hits        38994    39178     +184     
+ Misses      27831    27749      -82     
- Partials     2649     2654       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@algorandskiy algorandskiy changed the title WIP: p2p hybrid net identity p2p: hybrid node net identity for connection deduplication Jun 20, 2024
@algorandskiy algorandskiy added the p2p Work related to the p2p project label Jun 20, 2024
network/hybridNetwork.go Outdated Show resolved Hide resolved
@algorandskiy
Copy link
Contributor Author

While implementing this I noticed p2p peerID is not really used because

  1. wsNet uses confg.PublicAddress as identityChallenge.PublicAddress and compare against local dedupNames in the snipped below.
  2. p2p uses directly identityTracker relying on libp2p deduplication.
	// if the address is not meant for this host, return without attaching headers,
	// but also do not emit an error. This is because if an operator were to incorrectly
	// specify their dedupName, it could result in inappropriate disconnections from valid peers
	if _, ok := i.dedupNames[string(idChal.Msg.PublicAddress)]; !ok {
		return identityChallengeValue{}, crypto.PublicKey{}, nil
	}

I thought about eliminating it completely from challenge scheme but it requires more investigation.

@algorandskiy algorandskiy requested a review from cce June 21, 2024 14:24
@algorandskiy algorandskiy force-pushed the pavel/p2p-hybrid-tests branch 2 times, most recently from ab1d0bd to 0eca7c1 Compare June 21, 2024 23:34
@algorandskiy
Copy link
Contributor Author

Rebased/fixed merge conflicts

@algorandskiy
Copy link
Contributor Author

rebased on top of feature/p2p

config/config_test.go Outdated Show resolved Hide resolved
network/netidentity_test.go Outdated Show resolved Hide resolved
network/netidentity_test.go Outdated Show resolved Hide resolved
network/hybridNetwork_test.go Show resolved Hide resolved
@algorandskiy
Copy link
Contributor Author

rebased to fix build errors

@algorandskiy algorandskiy changed the base branch from feature/p2p to master June 28, 2024 18:19
@algorandskiy algorandskiy changed the base branch from master to feature/p2p June 28, 2024 18:20
@algorandskiy
Copy link
Contributor Author

Added a stub ResourceManager to limit connections.
TODOs:

  • Add a new P2PIncomingConnectionsLimit option
  • Update FD limits code to use IncomingConnectionsLimit + P2PIncomingConnectionsLimit
  • Add a test

gmalouf
gmalouf previously approved these changes Jul 10, 2024
@algorandskiy
Copy link
Contributor Author

Added P2PIncomingConnectionsLimit with limits adjustments and re-calculations as needed + tests.

config/localTemplate.go Outdated Show resolved Hide resolved
config/localTemplate.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

Added some minor comments

daemon/algod/server.go Outdated Show resolved Hide resolved
netdeploy/remote/deployedNetwork.go Outdated Show resolved Hide resolved
@algorandskiy
Copy link
Contributor Author

Fixed CR notes

Copy link
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

One suggestion, otherwise I think this is okay. Worth doublechecking all the config cases are covered.

config/localTemplate.go Outdated Show resolved Hide resolved
@algorandskiy algorandskiy merged commit 9c93670 into algorand:master Jul 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement p2p Work related to the p2p project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants