Skip to content

chore: remove backwards compatibility with 2.6.0#5415

Merged
sbackend123 merged 26 commits into
masterfrom
chore/remove-backwards-compatibility-with-2.6.0
Jun 8, 2026
Merged

chore: remove backwards compatibility with 2.6.0#5415
sbackend123 merged 26 commits into
masterfrom
chore/remove-backwards-compatibility-with-2.6.0

Conversation

@sbackend123

@sbackend123 sbackend123 commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

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

Breaking changes

Removes Bee 2.6.0 backward-compatibility code and the legacy single-multiaddr underlay wire encoding. Minimum supported Bee version on the network is 2.7.0; nodes on 2.6.0 or earlier will fail to complete a handshake with this build.

Underlay wire format
The 0x99 list prefix (underlayListPrefix) is kept. The only supported encoding is now always the prefixed list format, including for a single address or an empty list:

[0x99][varint_len_1][addr_1_bytes][varint_len_2][addr_2_bytes]...

Changes in detail:

  • Removed the legacy path where a single underlay was serialized as raw multiaddr bytes (without the 0x99 prefix).
  • Even a peer with one underlay is encoded as a one-entry prefixed list.
  • An empty underlay list serializes to [0x99] and deserializes to an empty slice.
  • DeserializeUnderlays rejects payloads that do not start with 0x99 (including legacy raw single-multiaddr bytes from Bee 2.6.0).
  • ParseAddress still rejects addresses with zero underlays after deserialization.

Removed 2.6.0 compatibility shims

  • WithBee260Compatibility handshake option
  • FilterBee260CompatibleUnderlays / Bee260CompatibilityStreamer / IsBee260
  • User-agent–based Bee 2.6.0 detection in libp2p peer registry

Database migrations

Legacy migration step implementations are removed; version indices are retained as NOOPs so existing data directories can still migrate safely (ValidateVersions requires contiguous version numbers). Comments document why these slots must not be removed or renumbered.

Also removes unused legacy helpers (olditems, oldstampindex, obsolete storer migration step files, etc.).

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

#5340

Screenshots (if appropriate):

AI Disclosure

  • This PR contains code that has been generated by an LLM.
  • I have reviewed the AI generated code thoroughly.
  • I possess the technical expertise to responsibly review the code generated in this PR.

@acud

acud commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

@sbackend123 I think that removing all migration code might be a bit too eager - can you make sure that the migration code covers the currently active bee node version still showing up in swarmscan?

@janos

janos commented Mar 31, 2026

Copy link
Copy Markdown
Member

@sbackend123 I think that removing all migration code might be a bit too eager - can you make sure that the migration code covers the currently active bee node version still showing up in swarmscan?

I agree, as there are still 2.6 nodes (not many, but still ~6%), it is early to release bee without compatibility.

This code is intended to be temporary. It is a good effort, but I would suggest to hold on merging, for some time.

@sbackend123

Copy link
Copy Markdown
Contributor Author

@sbackend123 I think that removing all migration code might be a bit too eager - can you make sure that the migration code covers the currently active bee node version still showing up in swarmscan?

I ran the previous bee version (v2.6.0) locally, let it sync to current block height on maiinet and build a real data directory. Then I started this build on the same data directory without clearing it. The node started successfully with no migration errors.

I also ran earlier versions trying to sync with mainnet, but get error connect new stream: incompatible stream: failed to negotiate protocol: protocols not supported.

@acud

acud commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

@sbackend123 I think that removing all migration code might be a bit too eager - can you make sure that the migration code covers the currently active bee node version still showing up in swarmscan?

I ran the previous bee version (v2.6.0) locally, let it sync to current block height on maiinet and build a real data directory. Then I started this build on the same data directory without clearing it. The node started successfully with no migration errors.

I also ran earlier versions trying to sync with mainnet, but get error connect new stream: incompatible stream: failed to negotiate protocol: protocols not supported.

I suppose it does not error the migrations because there's nothing to do in the migrations in the current build. I am not 100% sure about the reasoning behind the necessity of those migrations. But we should definitely leave in place the migration code to support the migration path from the node version still running on the network (2.6.0 definitely). The older ones we can mark as nops.

@mfw78

mfw78 commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

I agree, as there are still 2.6 nodes (not many, but still ~6%), it is early to release bee without compatibility.

Those 2.6.0 nodes are on some dirty tag mostly, so it's on them. They are running custom code - we shouldn't be persisting with cruft in the protocol because someone doesn't update their modifications.

@acud

acud commented Apr 6, 2026

Copy link
Copy Markdown
Contributor

@sbackend123 this needs a rebase

@gacevicljubisa gacevicljubisa linked an issue Apr 9, 2026 that may be closed by this pull request
9 tasks

@acud acud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there's a whole chunk of changes that are in pkg/storer/migration/all_steps.go that is completely not touched but the code can probably also be removed. as for the individual migration steps which are not in use already - there's no need of keeping the files with empty functions that do nothing - those can be removed.


func firstByteString(data []byte) string {
if len(data) == 0 {
return "none"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe just ""?

Comment thread pkg/storer/migration/step_02.go Outdated
// step_02 was a migration step that migrated the cache to a new format.
// It is now a NOOP since all nodes have already run this migration,
// and new nodes start with an empty database.
func step_02(_ transaction.Storage) func() error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if this is not used anywhere - why do we need to keep the file?

…2.6.0

# Conflicts:
#	pkg/p2p/libp2p/version_test.go
#	pkg/statestore/storeadapter/migration.go
#	pkg/storer/internal/reserve/olditems.go
#	pkg/storer/migration/refCntSize.go
#	pkg/storer/migration/step_02.go
#	pkg/storer/migration/step_04_test.go
#	pkg/storer/migration/step_05.go
#	pkg/storer/migration/step_05_test.go
#	pkg/storer/migration/step_06.go
#	pkg/storer/migration/step_06_test.go
Add test. Move db repair tool.
@sbackend123

Copy link
Copy Markdown
Contributor Author

there's a whole chunk of changes that are in pkg/storer/migration/all_steps.go that is completely not touched but the code can probably also be removed. as for the individual migration steps which are not in use already - there's no need of keeping the files with empty functions that do nothing - those can be removed.

If I understand everything correctly, we can't remove it completely because of our migration mechanism.
Existing upgraded nodes may appear fine, but new installations would be broken, because ValidateVersions checks versions are without gaps (added unit test for that) and when we have currentVersion=0 and then next one 8 -> fail.

@sbackend123 sbackend123 requested a review from acud April 28, 2026 09:08
@sbackend123 sbackend123 marked this pull request as ready for review April 28, 2026 09:08
Comment thread pkg/bzz/address.go

func (a *Address) UnmarshalJSON(b []byte) error {
v := &addressJSON{}
err := json.Unmarshal(b, v)

@akrem-chabchoub akrem-chabchoub Apr 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work @sbackend123.

I just have a concern here, what will happen for the 2.6.0 nodes here ? because I see you removed Underlay field from addressJSON, I know this is not needed anymore, but in the Unmarshal the node will have the Underlays filed empty.

Maybe we can keep it in addressJSON (just to for the Unmarshal) and we get that Underlay addr and set it to Underlays slice
Right or maybe I'm missing some parts ?

@sbackend123 sbackend123 Apr 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is main point, we remove compatibility with 2.6.0 so minimal supported version would be 2.7.0. So we force everybody update their versions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, I know.
I’m just thinking out loud, if I were a node operator on 2.6.0 or earlier, I might suddenly get disconnected without really understanding why. That could be a bit confusing.
But as @janos mentioned, those nodes are only around 6%, so the impact seems pretty small.

Comment thread pkg/storer/migration/all_steps.go
5: noop,
6: noop,
7: noop,
8: noop,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

like in the other migrations comment - the sequence and indexes of these migrations should be commented such that they aren't removed in the future

@acud acud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but will need a squash + rebase after the upcoming release

…2.6.0

# Conflicts:
#	pkg/bzz/address.go
#	pkg/bzz/address_test.go
#	pkg/bzz/export_test.go
#	pkg/bzz/underlay.go
#	pkg/bzz/underlay_test.go
#	pkg/hive/hive.go
#	pkg/p2p/libp2p/internal/handshake/handshake.go
#	pkg/p2p/libp2p/libp2p.go
#	pkg/statestore/storeadapter/migration.go
@acud

acud commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

@sbackend123 linter failing

@gacevicljubisa

gacevicljubisa commented Jun 5, 2026

Copy link
Copy Markdown
Member

@sbackend123 can you update the PR description?

Comment thread pkg/bzz/underlay.go Outdated

// For 0 or 2+ addresses, the custom list format with the prefix is used.
// The format is: [prefix_byte][varint_len_1][addr_1_bytes]...
// The format is: [varint_len_1][addr_1_bytes]...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missing the prefix byte here. see comment on the left

Comment thread pkg/bzz/underlay.go Outdated
}
// The result is returned as a single-element slice for a consistent return type.
return []multiaddr.Multiaddr{addr}, nil
return deserializeList(data)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't seem right. with this change, the underlay should always start with the prefix byte. the if block above should check whether the first byte is not a prefix byte and throw if that is the case, then just deserialize like in the current if statement below (happy path not nested).
also some input length validation is needed since the deserialize call can panic on a short buffer (pass just 0x99 as the underlay)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed: in case if array of underlays contains only 0x99 prefix, decerialize will not panic, it just returns prefix (behavior unchanged)


observedUnderlays, err := bzz.DeserializeUnderlays(resp.Syn.ObservedUnderlay)
if err != nil {
s.logger.Debug("handshake invalid synack observed underlay payload", "payload_len", len(resp.Syn.ObservedUnderlay), "first_byte", firstByteString(resp.Syn.ObservedUnderlay), "payload_prefix", payloadPrefix(resp.Syn.ObservedUnderlay), "error", err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lots of stuff in this log line... not sure if it will be just swallowed in the rest of the logs. maybe just better to instrument the error with a tiny bit of information of what went wrong (not all the fields here). the same for the other logline

return fmt.Sprintf("0x%02x", data[0])
}

func payloadPrefix(data []byte) string {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not sure i understand why this is needed... maybe some info about why and how to use this... also why is n=16?

if len(data) < n {
n = len(data)
}
return fmt.Sprintf("%x", data[:n])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you can just fmt.Sprintf("%x",data[:min(16,len(data))])

@acud acud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

last one...

if len(payload) == 0 {
return fmt.Errorf("%w: observed underlay (len=0): %w", ErrInvalidSyn, err)
}
return fmt.Errorf("%w: observed underlay (len=%d, first=0x%02x): %w", ErrInvalidSyn, len(payload), payload[0], err)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need conditional error formatting? this appears to be needed just so you have a sneak peek into the first byte (not sure what is the added value of this in production). i would suggest to get rid of this helper and just stringify the payload in the error formatting (one format). the 0x99 ascii character is the trademark sign so you would be able to see it in the logs correctly (if ever needed)

@sbackend123 sbackend123 requested a review from acud June 8, 2026 15:50
@sbackend123 sbackend123 merged commit 680e893 into master Jun 8, 2026
20 of 21 checks passed
@sbackend123 sbackend123 deleted the chore/remove-backwards-compatibility-with-2.6.0 branch June 8, 2026 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove backwards compatibility code to clean up codebase

6 participants