p2p: peer metainfo support #6312
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6312 +/- ##
==========================================
+ Coverage 51.85% 51.87% +0.01%
==========================================
Files 652 653 +1
Lines 87433 87554 +121
==========================================
+ Hits 45340 45417 +77
- Misses 39226 39267 +41
- Partials 2867 2870 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
algorandskiy
force-pushed
the
pavel/p2p-handshake
branch
from
7647d0e to
0d674e2
Compare
algorandskiy
force-pushed
the
pavel/p2p-handshake
branch
from
df213ee to
249c77d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds peer meta info exchange support for p2p networks and generalizes header setting to work across both wsnet and p2pnet. Key changes include:
- Renaming of identifier fields (e.g. GenesisID/RandomID to genesisID/randomID) and updating their usages.
- Refactoring of header-setting into a generic function using a peerMetadataProvider interface.
- Addition of support for the new /algorand-ws/2.2.0 protocol in p2p stream handlers and adjusted test timeouts to aid stability in race builds.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| node/node_test.go | Updated log messages to include peer IDs in phonebook responses. |
| network/wsNetwork.go | Transitioned from public to lowercase identifier fields and refactored header setting. |
| network/p2pNetwork.go | Introduced support for dual protocol versions and temporary logic with a TODO comment. |
| network/*_test.go | Extended test timeouts for p2p tests to mitigate race conditions. |
| network/streams.go | Refactored stream dispatch to use protocol-specific handler maps. |
| network/msgp_gen*.go | Regenerated msgp code for peer meta headers/values. |
Comments suppressed due to low confidence (3)
network/p2pNetwork_test.go:109
- Increasing the timeout from 2 to 5 seconds can help with race conditions on CI builds, but please confirm that the longer wait is intentional and does not hide potential performance issues.
}, 5*time.Second, 50*time.Millisecond)
network/p2pNetwork.go:292
- The temporary protocol version selection logic gated by the disableV22Protocol flag is clearly marked with a TODO; ensure that this block is removed once consensus v41 is deployed.
TODO: remove after consensus v41 takes effect.
node/node_test.go:1025
- [nitpick] Please verify that using ni[0].p2pID for node 0 is the intended behavior and is consistent with the later cases that log ni[1].p2pID and ni[2].p2pID.
t.Logf("Node%d %s phonebook: %s", i, ni[0].p2pID, ni[1].p2pMultiAddr())
| const algorandGUIDProtocolTemplate = algorandGUIDProtocolPrefix + "%s/%s" | ||
| // AlgorandWsProtocolV22 defines a libp2p protocol name for algorand's websockets messages | ||
| // as a version supporting peer metadata and wsnet v2.2 protocol features | ||
| const AlgorandWsProtocolV22 = "/algorand-ws/2.2.0" |
There was a problem hiding this comment.
Q: should it somehow in sync with wsnetwork.ProtocolVersion?
There was a problem hiding this comment.
Ideally one would reference the other..
There was a problem hiding this comment.
Going through wsNetwork - it definitely feels like these two constants should be defined together/one should direct reference the other to help keep in sync.
There was a problem hiding this comment.
a "problem" with this approach is wsnet v1.0 is not supported but we still need /algorand-ws/1.0.0 in p2p. I suggest to unify 2.2 in later release after the consensus upgrade when /algorand-ws/1.0.0 can be removed
| localPeer := n.host.ID() | ||
| func (n *streamManager) peerWatcher(ctx context.Context, sub event.Subscription) { | ||
| defer sub.Close() | ||
| for e := range sub.Out() { |
There was a problem hiding this comment.
the inner body of the for loop is the same as Connected handler with return replaced by continue
| @@ -106,7 +106,7 @@ func TestP2PSubmitTX(t *testing.T) { | |||
| ) | |||
| require.Eventually(t, func() bool { | |||
| return netA.hasPeers() && netB.hasPeers() && netC.hasPeers() | |||
| }, 2*time.Second, 50*time.Millisecond) | |||
| }, 5*time.Second, 50*time.Millisecond) | |||
There was a problem hiding this comment.
new EvtPeerIdentificationCompleted handler requires more time on CI
that's the point of an extra p2p proto to have it fully backward compatible (telemetryID stuff is removed b/c it non-working I believe in most cases). |
algorandskiy
force-pushed
the
pavel/p2p-handshake
branch
from
fa45b75 to
610d555
Compare
|
I am only getting started understanding how this stuff works, but it looks to me like in v1, there is a single byte exchange, but neither side cares what the bytes actually are. It looks like they send ascii "1". Why couldn't we say "if you send a certain byte, the headers come next"? |
algorandskiy
force-pushed
the
pavel/p2p-handshake
branch
from
610d555 to
7108389
Compare
because of the following: new client and old server combo: old server can only read one byte and write one byte, and new client initiates the exchange by writing something not knowing what other side expects - one byte or multiple bytes. |
| return args.Int(0), args.Error(1) | ||
| } | ||
|
|
||
| func TestReadPeerMetaHeaders(t *testing.T) { |
There was a problem hiding this comment.
This doesn't have any testing of bad data on the stream. Is the assumption that bad data isn't very dangerous, it'll just cause a error and the stream to close if it's too short, or if the length bytes are wrong, or whatever?
There was a problem hiding this comment.
And I guess with only a uint16 for length, it's not like a bad peer can make you read a crazy amount of data.
There was a problem hiding this comment.
Fair, added test cases covering all the error branches
|
Right, of course. Thanks!
…On Tue, May 13, 2025 at 12:29 PM Pavel Zbitskiy ***@***.***> wrote:
*algorandskiy* left a comment (algorand/go-algorand#6312)
<#6312 (comment)>
Why couldn't we say "if you send a certain byte, the headers come next"?
because of the following: new client and old server combo: old server can
only read one byte and write one byte, and new client initiates the
exchange by writing something not knowing what other side expects - one
byte or multiple bytes.
Ok, new client can write a byte and read with timeout, if nothing came out
then guess it is a new server and push meta info. But it brings 1)
connection delay 2) more complex logic rather then two well separated p2p
protocols.
—
Reply to this email directly, view it on GitHub
<#6312 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADL7T7XYKDOJCHUPZQVYZ326IMW5AVCNFSM6AAAAAB4LFDI4WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQNZXGIYDQOBYGU>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Summary
P2P network was missing peer meta info exchange including peer features like vote compression.
General idea: current p2p
/algorand-ws/1.0.0handler has a stub for meta info exchange (handshake) similar to HTTP: connection initiator writes a single byte, server reads a byte. Unfortunately there is no elegant solution for keeping a single/algorand-ws/1.0.0but also support both old single byte handshake and multibyte meta info exchange when both old and new node have to communicate each other. The solution is to add a new p2p/algorand-ws/2.2.0proto with multibyte handshake consisting of 2 bytes of data length and msgp-encoded http.Header-like map as a data portion. p2p nodes can list remote protocols and determine either/algorand-ws/2.2.0or/algorand-ws/1.0.0to use.In this PR:
setHeaderto be able to use in both wsnet and p2pnet/algorand-ws/2.2.0p2p proto in addition to/algorand-ws/1.0.0/algorand-ws/2.2.0handlerConnectednotification toEvtPeerIdentificationCompletedin order to be able to communicate with both old/algorand-ws/1.0.0and new/algorand-ws/2.2.0nodesTest Plan
Added unit tests:
Manual test:
Run a new p2p node vs betanet and testnet to ensure it can fast catchup, catchup and follow the chain.