Well-defined CAddress disk serialization, and addrv2 anchors.dat #20516

sipa · 2020-11-27T01:21:31Z

Alternative to #20509.

This makes the CAddress disk serialization format well defined, and uses it to enable addrv2 support in anchors.dat (in a way that's compatible with older software). The new format is:

The first 4 bytes store a format version number. Its low 19 bits are ignored (as those historically stored the CLIENT_VERSION), but its high 13 bits specify the actual serialization:
- 0x00000000: LE64 encoding for nServices, V1 encoding for CService (like pre-BIP155 network serialization).
- 0x20000000: CompactSize encoding for nServices, V2 encoding for CService (like BIP155 network serialization).
- Any other value triggers an unsupported format error on deserialization, and can be used for future format changes.
The ADDRV2_FORMAT flag in the stream's version does not determine the actual serialization format; it only sets whether or not V2 encoding is permitted.

DrahtBot · 2020-11-27T02:24:20Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

p2p: add time when deserialize file db for ReadAnchors #21483 (p2p: add time when deserialize file db for ReadAnchors by brunoerg)
banman: save the banlist in a JSON format on disk #20966 (banman: save the banlist in a JSON format on disk by vasild)
net: CAddress deser: use stream's version, not what's coming from disk #20509 (net: CAddress deser: use stream's version, not what's coming from disk by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa · 2020-11-27T19:17:45Z

I merged the anchors.dat addrv2 support from #20514 into this PR, as doing it correctly requires changes the Cservice serialization from stream version based to stored version based.

hebasto · 2020-11-27T19:49:42Z

Concept ACK.

From the PR description and quick code reading it follows that there is no hurry to backport these changes into 0.21, right?

sipa · 2020-11-27T19:50:40Z

Indeed, there is probably no hurry, unless we want to support torv3 anchors in 0.21.

src/addrdb.cpp

hebasto

In commit 6d16903c4c384bc15f3f29e5635bb9dccd789cb6 message:

These do no introduce incompatibilities, as no code versions exist that write
any value other than 0 or 0x20000000 in the top 19 bits, and no code paths
where the stream's version differs from the stored version.

s/top 19/high 13/ ?

src/protocol.h

sipa · 2020-11-30T21:41:03Z

Addressed comments.

vasild

Approach ACK 80f5c5442

src/protocol.h

src/addrdb.cpp

src/protocol.h

Before this commit, CAddress disk serialization was messy. It stored CLIENT_VERSION in the first 4 bytes, optionally OR'ed with ADDRV2_FORMAT. - All bits except ADDRV2_FORMAT were ignored, making it hard to use for actual future format changes. - ADDRV2_FORMAT determines whether or not nServices is serialized in LE64 format or in CompactSize format. - Whether or not the embedded CService is serialized in V1 or V2 format is determined by the stream's version having ADDRV2_FORMAT (as opposed to the nServices encoding, which is determined by the disk version). To improve the situation, this commit introduces the following disk serialization format, compatible with earlier versions, but better defined for future changes: - The first 4 bytes store a format version number. Its low 19 bits are ignored (as it historically stored the CLIENT_VERSION), but its high 13 bits specify the serialization exactly: - 0x00000000: LE64 encoding for nServices, V1 encoding for CService - 0x20000000: CompactSize encoding for nServices, V2 encoding for CService - Any other value triggers an unsupported format error on deserialization, and can be used for future format changes. - The ADDRV2_FORMAT flag in the stream's version does not impact the actual serialization format; it only determines whether V2 encoding is permitted; whether it's actually enabled depends solely on the disk version number. Operationally the changes to the deserializer are: - Failure when the stored format version number is unexpected. - The embedded CService's format is determined by the stored format version number rather than the stream's version number. These do no introduce incompatibilities, as no code versions exist that write any value other than 0 or 0x20000000 in the top 13 bits, and no code paths where the stream's version differs from the stored version.

sipa · 2021-05-25T01:08:39Z

Rebased, and addressed comments.

vasild

ACK f8866e8

jonatack

ACK f8866e8 tested rebased to master and built/run/restarted with DEBUG_ADDRMAN, peers.dat and anchors ser/deser seems fine

src/protocol.h

hebasto

Approach ACK

sipa · 2021-06-12T21:38:53Z

@hebasto I think you need to leave a new review.

comment resolved

hebasto

ACK f8866e8, tested on Linux Mint 20.1 (x86_64).

Checked that changes in anchors.dat are forward compatible. They are not backward compatible -- getting "ERROR: DeserializeDB: Deserialize or I/O error - CAutoFile::read: end of file: iostream error", but this is expected.

hebasto · 2021-06-14T03:54:11Z

src/protocol.h

+            uint32_t stored_format_version = DISK_VERSION_INIT;
+            if (s.GetVersion() & ADDRV2_FORMAT) stored_format_version |= DISK_VERSION_ADDRV2;
+            READWRITE(stored_format_version);
+            stored_format_version &= ~DISK_VERSION_IGNORE_MASK; // ignore low bits


nit:

This is the only place (besides two assertions above) where DISK_VERSION_IGNORE_MASK is used, and it is inverted. Isn't it more clear:

Suggested change

stored_format_version &= ~DISK_VERSION_IGNORE_MASK; // ignore low bits

stored_format_version &= DISK_VERSION_MASK; // ignore low bits

?

Will do if I retouch.

achow101 · 2021-06-14T19:20:55Z

ACK f8866e8

laanwj · 2021-06-17T15:41:48Z

Code review ACK f8866e8

…drv2 anchors.dat f8866e8 Add roundtrip fuzz tests for CAddress serialization (Pieter Wuille) e2f0548 Use addrv2 serialization in anchors.dat (Pieter Wuille) 8cd8f37 Introduce well-defined CAddress disk serialization (Pieter Wuille) Pull request description: Alternative to bitcoin#20509. This makes the `CAddress` disk serialization format well defined, and uses it to enable addrv2 support in anchors.dat (in a way that's compatible with older software). The new format is: - The first 4 bytes store a format version number. Its low 19 bits are ignored (as those historically stored the `CLIENT_VERSION`), but its high 13 bits specify the actual serialization: - 0x00000000: LE64 encoding for `nServices`, V1 encoding for `CService` (like pre-BIP155 network serialization). - 0x20000000: CompactSize encoding for `nServices`, V2 encoding for `CService` (like BIP155 network serialization). - Any other value triggers an unsupported format error on deserialization, and can be used for future format changes. - The `ADDRV2_FORMAT` flag in the stream's version does not determine the actual serialization format; it only sets whether or not V2 encoding is permitted. ACKs for top commit: achow101: ACK f8866e8 laanwj: Code review ACK f8866e8 vasild: ACK f8866e8 jonatack: ACK f8866e8 tested rebased to master and built/run/restarted with DEBUG_ADDRMAN, peers.dat and anchors ser/deser seems fine hebasto: ACK f8866e8, tested on Linux Mint 20.1 (x86_64). Tree-SHA512: 3898f8a8c51783a46dd0aae03fa10060521f5dd6e79315fe95ba807689e78f202388ffa28c40bf156c6f7b1fc2ce806b155dcbe56027df73d039a55331723796

maflcko · 2021-06-21T11:37:17Z

review ACK f8866e8 🕑

Show signature and timestamp

Signature:

Timestamp of file with hash 71c25ee05aade76e5311b5e5ffa61e3c8b280b51a49049dba2025a6d50dfe77b -

maflcko · 2021-06-21T11:38:47Z

src/test/fuzz/deserialize.cpp

        (void)mh.IsCommandValid();
 })
-FUZZ_TARGET_DESERIALIZE(address_deserialize, {
+FUZZ_TARGET_DESERIALIZE(address_deserialize_v1_notime, {


Copied the fuzz inputs in commit bitcoin-core/qa-assets@836513a, so that the targets have something nice to start with.

sipa requested a review from vasild November 27, 2020 01:27

sipa mentioned this pull request Nov 27, 2020

net: CAddress deser: use stream's version, not what's coming from disk #20509

Closed

DrahtBot added Build system P2P labels Nov 27, 2020

sipa force-pushed the 202011_disk_addr branch from 94fdec1 to 1991447 Compare November 27, 2020 03:03

maflcko added this to the 22.0 milestone Nov 27, 2020

sipa mentioned this pull request Nov 27, 2020

Use addrv2 serialization in anchors.dat #20514

Closed

sipa force-pushed the 202011_disk_addr branch from 1991447 to 04e3733 Compare November 27, 2020 19:11

sipa changed the title ~~Introduce well-defined CAddress disk serialization~~ Well-defined CAddress disk serialization, and addrv2 anchors.dat support Nov 27, 2020

sipa changed the title ~~Well-defined CAddress disk serialization, and addrv2 anchors.dat support~~ Well-defined CAddress disk serialization, and addrv2 anchors.dat Nov 27, 2020

hebasto reviewed Nov 28, 2020

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

hebasto reviewed Nov 28, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

hebasto previously requested changes Nov 28, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

sipa force-pushed the 202011_disk_addr branch 2 times, most recently from 22b8c55 to 80f5c54 Compare November 30, 2020 21:40

This was referenced Dec 2, 2020

Move special CAddress-without-nTime logic to net_processing #20541

Merged

fuzz: Link all targets once #20560

Merged

DrahtBot added the Needs rebase label Dec 15, 2020

vasild reviewed Dec 18, 2020

View reviewed changes

sipa added 3 commits May 24, 2021 18:06

Use addrv2 serialization in anchors.dat

e2f0548

Add roundtrip fuzz tests for CAddress serialization

f8866e8

sipa force-pushed the 202011_disk_addr branch from 80f5c54 to f8866e8 Compare May 25, 2021 01:06

DrahtBot removed the Needs rebase label May 25, 2021

This was referenced May 25, 2021

p2p: add time when deserialize file db for ReadAnchors #21483

Closed

banman: save the banlist in a JSON format on disk #20966

Merged

vasild approved these changes May 28, 2021

View reviewed changes

jonatack reviewed May 28, 2021

View reviewed changes

src/protocol.h Show resolved Hide resolved

hebasto reviewed Jun 12, 2021

View reviewed changes

hebasto self-requested a review June 12, 2021 21:46

hebasto approved these changes Jun 14, 2021

View reviewed changes

laanwj merged commit 7b45c5e into bitcoin:master Jun 17, 2021

maflcko reviewed Jun 21, 2021

View reviewed changes

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 7b45c5e into merged_master (Bitcoin PR bitcoin/bitcoin#20516)

3bef51b

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

	stored_format_version &= ~DISK_VERSION_IGNORE_MASK; // ignore low bits
	stored_format_version &= DISK_VERSION_MASK; // ignore low bits

Well-defined CAddress disk serialization, and addrv2 anchors.dat #20516

Well-defined CAddress disk serialization, and addrv2 anchors.dat #20516

Uh oh!

Conversation

sipa commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

sipa commented Nov 27, 2020

Uh oh!

hebasto commented Nov 27, 2020

Uh oh!

sipa commented Nov 27, 2020

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented Nov 30, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sipa commented May 25, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • 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!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented Jun 12, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

sipa Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 14, 2021

Uh oh!

laanwj commented Jun 17, 2021

Uh oh!

maflcko commented Jun 21, 2021

Uh oh!

maflcko Jun 21, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sipa commented Nov 27, 2020 •

edited

Loading

DrahtBot commented Nov 27, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading