feat(handshake): enum protobuf and capabilities #5105

mfw78 · 2025-05-27T09:54:58Z

Checklist

I have read the coding guide.
~~My change requires a documentation update, and I have done it.~~
I have added tests to cover my changes.
I have filled out the description and linked the related issues.

Description

This PR revises the handshake protocol such that:

There is the inclusive of a Capabilities message within the Ack that determines if the node is a full node and/or supports tracing headers.
The Handshake protobuf now is a oneof message which enforces strict typing of the messages on the wire.
Unless specifically enabled by the configuration flag, no Headers are passed in the opening of new streams within libp2p, essentially reducing RTTs / network traffic that is needlessly sending empty headers on every single stream creation except Handshake. The functionality is preserved for developers / those wanting to debug the protocol.

Open API Spec Version Changes (if applicable)

N/A

Motivation and Context (Optional)

Strict typing and reduction in needless network chatter.

Related Issue (Optional)

add swip-26: strictly typed chunk system SWIPs#67

Screenshots (if appropriate):

N/A

mfw78 · 2025-05-27T10:55:25Z

.golangci.yml

Migrates the golang CI yaml to version 2, given that the project now uses go 1.24.0

mfw78 · 2025-05-27T10:55:59Z

.github/workflows/go.yml

+              if: github.ref != 'refs/heads/master'
+              uses: wagoid/commitlint-github-action@v5
+            - name: GolangCI-Lint
+              uses: golangci/golangci-lint-action@v7


Bump required here as v2.1.6 of GolangCI-lint is only supported in @7

mfw78 · 2025-05-27T10:56:45Z

.golangci.yml

+      - linters:
+          - staticcheck
+        text: "QF1008:"
+      - linters:
+          - staticcheck
+        text: "QF1001:"
+      - linters:
+          - staticcheck
+        text: "QF1002:"
+      - linters:
+          - staticcheck
+        text: "QF1003:"
+      - linters:
+          - staticcheck
+        text: "QF1006:"


Disregarded as these are general in nature

zelig · 2025-05-30T05:12:21Z

@mfw78 please fix linting errors for a green CI, ta

mfw78 · 2025-05-30T06:18:36Z

@mfw78 please fix linting errors for a green CI, ta

I've bumped to the latest golang linters for CI/CD within this PR, the failing CI/CD currently seems limited to beekeeper settlement tests.

martinconic · 2025-07-06T18:42:53Z

pkg/p2p/libp2p/internal/handshake/handshake.go

 	ProtocolName = "handshake"
 	// ProtocolVersion is the current handshake protocol version.
-	ProtocolVersion = "13.0.0"
+	ProtocolVersion = "14.0.0"


You have to increase this once more, ProtocolVersion was already increased in the meanwhile

nugaon

nice initiative!

nugaon · 2025-07-03T15:46:47Z

.github/workflows/go.yml

-      - packaging/**
-    branches:
-      - '**'
+    push:


unnecessary indentation doubling on each line.

nugaon · 2025-07-03T15:49:30Z

pkg/file/splitter/internal/job.go

 //	F   F   F
 //
-// F   F   F   F S
+// # F   F   F   F S


unnecessary change as well as below

nugaon · 2025-07-03T16:17:28Z

.golangci.yml

+          - goheader
+        text: go-ethereum Authors
+      - linters:
+          - goconst


originally, next to the rules there we comments like "temporally disable...", but this is how temporal solutions work usually : D

nugaon · 2025-07-03T16:58:44Z

pkg/p2p/libp2p/stream.go

 	return &stream{Stream: s, metrics: metrics}
 }
+
+func newStreamWithHeaders(s network.Stream, metrics metrics, ctx context.Context, headler p2p.HeadlerFunc, peerAddress swarm.Address, headers p2p.Headers) (*stream, error) {


headers parameter is unused and ctx is usually the first parameter

nugaon · 2025-07-04T13:32:55Z

pkg/p2p/libp2p/protocols_test.go

+
+	// Write data to trigger the protocol handler
+	if stream != nil {
+		_, _ = stream.Write([]byte("test"))


I would do error check here because it may lead to silent failure.

nugaon · 2025-07-04T13:50:56Z

pkg/p2p/libp2p/libp2p.go

-				return
+			// Check if BOTH local and remote peer support trace headers
+			// At this point, handshake has completed and capabilities are known
+			peerCaps := s.peers.capabilities(overlay)


it could be optimized by mapping to the capabilities only once and get traceability flag along with fullnode flag (above this change)

nugaon · 2025-07-06T21:39:55Z

pkg/p2p/libp2p/internal/handshake/handshake_test.go

 }

+func TestInfo_CapabilitiesReuse(t *testing.T) {
+	// Test that Info struct reuses the protobuf Capabilities object


does it test the pointer functionality in Go? :D

nugaon · 2025-07-06T22:03:43Z

pkg/p2p/libp2p/internal/handshake/pb/handshake.pb.go

-}
-
-func (m *Syn) Marshal() (dAtA []byte, err error) {
+	// 438 bytes of a gzipped FileDescriptorProto


I get different output by running
protoc --gogofaster_out=. --gogofaster_opt=paths=source_relative,Mgzip.proto ./pkg/p2p/libp2p/internal/handshake/pb/handshake.proto

nugaon · 2025-07-06T23:04:46Z

pkg/p2p/libp2p/internal/handshake/handshake.go


 	if s.picker != nil {
-		if !s.picker.Pick(p2p.Peer{Address: overlay, FullNode: ack.FullNode}) {
+		if !s.picker.Pick(p2p.Peer{Address: overlay, FullNode: ack.Capabilities.FullNode}) {


Protocol Buffers does not automatically create empty objects for missing fields and it can cause nil pointer dereference here since there is no nil check on Capabilities.

nugaon · 2025-07-06T23:05:19Z

pkg/p2p/libp2p/internal/handshake/handshake.go

-		BzzAddress: remoteBzzAddress,
-		FullNode:   ack.FullNode,
+		BzzAddress:   remoteBzzAddress,
+		Capabilities: ack.Capabilities,


same as above

janos

Capabilities

Grouping Capabilities into a dedicated protobuf message is nice and adds clarity to the specific features that a node can provide.

Stream Headers

The Headers feature that is added to the Streams in bee are supposed to be a general purpose headers as string key/value pairs similar to the HTTP headers, as libp2p did not have any way to share metadata upon establishing Streams. Tracing is using Stream Headers to avoid adding tracing field to most of bee protobuf messages. So every mention of the tracing alongside headers in names should be removed.

At the time it was measured that headers did not add any significant overhead to streams. I hope that making Headers optional is influenced by measurements and metrics that support this change.

Unrelated changes

This PR holds some changes that are unrelated to the handshake protocol, and I think that they should be part of other PRs.

janos · 2025-07-07T12:57:25Z

Makefile

 GOBIN ?= $$($(GO) env GOPATH)/bin
 GOLANGCI_LINT ?= $(GOBIN)/golangci-lint
-GOLANGCI_LINT_VERSION ?= v1.64.5
+GOLANGCI_LINT_VERSION ?= v2.1.6


Raising golangci-lint version to 2 involves a larger set of changes in this PR which as a different purpose. I would recommend to create a separate PR for improvimg golangci-lint usage.

janos · 2025-07-07T13:02:21Z

cmd/bee/cmd/split.go


 			logger.Debug("write root", "hash", rootRef)
-			_, err = writer.WriteString(fmt.Sprintf("%s\n", rootRef))
+			_, err = fmt.Fprintf(writer, "%s\n", rootRef)


While this change can be considered as an improvement, as well as a change a few lines below, it does not relate to the handshake protocol and are not crucial.

janos · 2025-07-07T13:50:34Z

pkg/p2p/libp2p/libp2p.go

+	hostFactory        func(...libp2p.Option) (host.Host, error)
+	HeadersRWTimeout   time.Duration
+	Registry           *prometheus.Registry
+	EnableTraceHeaders bool


EnableTraceHeaders should be named only EnableHeaders as the whole headers functionality is now optional, not only tracing. Tracing is just one of the possible headers. Headers in bee streams have general purpose just as HTTP protocol headers are, just keys and values. It is ok to make them optional, like here, but tracing is just one of the header key/value pair. I do not think that there are any other usages of headers currently.

janos · 2025-07-07T13:53:10Z

pkg/transaction/transaction.go


 	// PendingNonceAt returns the nonce we should use, but we will
 	// compare this to our pending tx list, therefore the -1.
-	var maxNonce uint64 = onchainNonce - 1


This file has an unrelated changes to the handshake protocol.

janos · 2025-07-07T13:54:32Z

pkg/p2p/libp2p/libp2p.go

+			peerCaps := s.peers.capabilities(overlay)
+			localSupportsTrace := s.handshakeService.SupportsTraceHeaders()
+			remoteSupportsTrace := peerCaps != nil && peerCaps.TraceHeaders
+			bothSupportTrace := localSupportsTrace && remoteSupportsTrace


Thise value should be called localSupportsHeaders, remoteSupportsTrace, bothSupportHeaders...

janos · 2025-07-07T13:56:52Z

pkg/p2p/libp2p/internal/handshake/pb/handshake.proto

-    Ack Ack = 2;
+message Capabilities {
+    bool full_node = 1;
+    bool trace_headers = 2;


Just as on other places, where I provided some context on the naming for similar flags, this filed should be called supports_headers or something similar, just without tracing, as tracing is just one feature that uses headers feature.

janos · 2025-07-07T18:57:05Z

pkg/p2p/libp2p/internal/handshake/pb/handshake.proto

 option go_package = "pb";

-message Syn {
+message Handshake {


I am not sure why Handshake message is needed as it only wraps existing messages. Maybe this indirection could be avoided and only to use explicit messages as before as their order is sequential and deterministic in the protocol?

martinconic · 2025-07-08T09:37:52Z

pkg/p2p/libp2p/peer.go

 	return found, full, peerID
 }

+func (r *peerRegistry) capabilities(overlay swarm.Address) *pb.Capabilities {


I think this would be a cleaner solution:

func (r *peerRegistry) capabilities(overlay swarm.Address) *pb.Capabilities { r.mu.RLock() defer r.mu.RUnlock() peerID, exists := r.underlays[overlay.ByteString()] if !exists { return nil } return r.peerCapabilities[peerID] }

nugaon · 2025-07-14T21:43:33Z

investigated the beekeeper test error and it turned out swap protocol also uses libp2p header exchange.
I uploaded the fix here

mfw78 added 4 commits May 27, 2025 09:54

feat(handshake): enum protobuf and capabilities

8c475fd

refactor(handshake): unify functions

eecb596

chore: lint

3a5d0da

fix(cicd): lint version bump

1ad411d

mfw78 commented May 27, 2025

View reviewed changes

mfw78 added 20 commits May 27, 2025 11:28

fix(headers): eliminate header race condition

0f2227c

fix: lint

518fac4

fix(headers): no empties

1050518

chore(headers): use natural package name import

78d2018

refactor(headers): minimise diff

1e3235e

fix(headers): minimise api redundancy

419d387

fix(headers): reduce diff

f88ef72

fix(headers): reduce diff

ee677ea

fix(headers): reduce diff

27d7589

fix(headers): reduce diff

e9b9802

fix(headers): reduce diff

f8536f7

fix(headers): reduce diff

f6a9c2b

fix(headers): reduce diff

0c4e215

fix(headers): reduce diff

229271e

fix(headers): reduce diff

03bcd24

fix(headers): reduce diff

b0e6cb0

fix(headers): enable tracing in tracing headers test

bd831b2

fix(headers): tests expected writing

28e04bf

fix: lint

f4cc639

chore(headers): rm dead code

c45bf03

mfw78 marked this pull request as ready for review May 27, 2025 21:50

gacevicljubisa requested review from acha-bill and martinconic May 28, 2025 08:36

gacevicljubisa requested a review from janos May 28, 2025 08:36

zelig added ready for review The PR is ready to be reviewed in progress ongoing development , hold out with review go Pull requests that update go code labels May 29, 2025

zelig assigned mfw78 May 29, 2025

gacevicljubisa self-requested a review June 2, 2025 07:51

gacevicljubisa requested a review from nugaon July 3, 2025 15:07

martinconic reviewed Jul 6, 2025

View reviewed changes

nugaon requested changes Jul 6, 2025

View reviewed changes

janos reviewed Jul 7, 2025

View reviewed changes

martinconic requested changes Jul 8, 2025

View reviewed changes

feat(handshake): enum protobuf and capabilities #5105

Are you sure you want to change the base?

feat(handshake): enum protobuf and capabilities #5105

Uh oh!

Conversation

mfw78 commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zelig commented May 30, 2025

Uh oh!

mfw78 commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nugaon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nugaon Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janos left a comment

Choose a reason for hiding this comment

Capabilities

Stream Headers

Unrelated changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nugaon commented Jul 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

mfw78 commented May 27, 2025 •

edited

Loading

mfw78 commented May 30, 2025 •

edited

Loading

nugaon Jul 6, 2025 •

edited

Loading