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

vasild · 2020-11-26T14:54:12Z

During CAddress deserialization we use nVersion to decide whether to
use CompactSize format for nServices. However, that variable nVersion
is first assigned s.GetVersion() and is later overwritten with
whatever is coming from disk by READWRITE(nVersion).

We should use s.GetVersion() instead, which is also used by
READWRITEAS(CService, obj);, called at the end of CAddress
deserialization.

During `CAddress` deserialization we use `nVersion` to decide whether to use CompactSize format for `nServices`. However, that variable `nVersion` is first assigned `s.GetVersion()` and is later overwritten with whatever is coming from disk by `READWRITE(nVersion)`. We should use `s.GetVersion()` instead, which is also used by `READWRITEAS(CService, obj);`, called at the end of `CAddress` deserialization.

vasild · 2020-11-26T15:03:16Z

This has low impact because, while in theory some disk contents could cause us to deserialize inconsistently, we never create such disk contents. CAddress would serialize as either one of:

a version that contains ADDRV2_FORMAT, nServices in CompactSize, the address in addrv2 format; or
a version that does not contain ADDRV2_FORMAT, nServices in 8 bytes, the address in addrv1 format

Spotted by @sipa.

jonatack

ACK a55a338

while here, maybe hoist and make the implicit narrowing conversion explicit (or just hoist to an int)

@@ -370,11 +370,11 @@ public:
     {
         SER_READ(obj, obj.nTime = TIME_INIT);
         int nVersion = s.GetVersion();
-        if (s.GetType() & SER_DISK) {
+        bool is_disk_ser{static_cast<bool>(s.GetType() & SER_DISK)};
+        if (is_disk_ser) {
             READWRITE(nVersion);
         }
-        if ((s.GetType() & SER_DISK) ||
-            (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) {
+        if (is_disk_ser || (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) {

sipa · 2020-11-27T01:45:09Z

I'd very much have preferred this approach, and just deprecated the use of the CAddress serialized version numbers, but I fear that #20511 leaves us little choice to go the other way (and make everything use the stored version): I've opened #20516 to do so.

DrahtBot · 2020-11-27T02:30:18Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Well-defined CAddress disk serialization, and addrv2 anchors.dat #20516 (Well-defined CAddress disk serialization, and addrv2 anchors.dat by sipa)

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.

luke-jr · 2020-11-30T17:49:14Z

This looks wrong? If the disk has a non-addrv2 format, we shouldn't try to deserialise it as such...

vasild · 2020-12-01T13:34:23Z

@luke-jr well, the assumption is that the version is set properly in the stream by the caller (this is the case in the current master). There is this inconsistency in CAddress deserialization:

    SERIALIZE_METHODS(CAddress, obj)
    {
...
        int nVersion = s.GetVersion();
        if (s.GetType() & SER_DISK) {
            READWRITE(nVersion); // Saves the disk contents into nVersion
        }
...
        if (nVersion & ADDRV2_FORMAT) { // Uses nVersion (== disk contents) [1]
...
        }
        READWRITEAS(CService, obj); // Uses s.GetVersion() [2]
    }

[1] and [2] should use the same variable. This PR changes [1] to use s.GetVersion().

It would also be possible to change [2] to use the "disk contents":

+        const int stream_version_orig = s.GetVersion();
+        SER_READ(obj, if (s.GetType() & SER_DISK) { s.SetVersion(nVersion); });
         READWRITEAS(CService, obj);
+        SER_READ(obj, if (s.GetType() & SER_DISK) { s.SetVersion(stream_version_orig); });
     }

But then one could ask a counter question to yours: "If the stream has a non-addrv2 format, we shouldn't try to deserialise it as such... (in [2])".

Maybe compare the stream format with disk contents and if they don't both have ADDRV2_FORMAT set (or not set), then throw?

If #20516 gets merged, then this PR would be obsolete and can be closed.

…chors.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 #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

DrahtBot · 2021-06-17T17:46:20Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

jonatack · 2021-06-17T17:57:43Z

Following the merge of #20516, maybe this PR can be closed IIUC.

…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-21T09:57:57Z

@vasild Closing for now

vasild · 2021-06-21T15:00:31Z

Yes, from above:

If #20516 gets merged, then this PR would be obsolete and can be closed.

Thanks!

jonatack reviewed Nov 26, 2020

View reviewed changes

DrahtBot added the P2P label Nov 26, 2020

This was referenced Nov 26, 2020

anchors.dat doesn't support V2 addresses #20511

Closed

Use addrv2 serialization in anchors.dat #20514

Closed

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

Merged

DrahtBot mentioned this pull request Dec 2, 2020

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

Merged

DrahtBot added the Needs rebase label Jun 17, 2021

maflcko closed this Jun 21, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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 #20509

Uh oh!

vasild commented Nov 26, 2020

Uh oh!

vasild commented Nov 26, 2020

Uh oh!

jonatack left a comment •

edited

Loading

Uh oh!

sipa commented Nov 27, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Nov 27, 2020 •

edited

Loading

Uh oh!

luke-jr commented Nov 30, 2020

Uh oh!

vasild commented Dec 1, 2020

Uh oh!

DrahtBot commented Jun 17, 2021

Uh oh!

jonatack commented Jun 17, 2021

Uh oh!

maflcko commented Jun 21, 2021

Uh oh!

vasild commented Jun 21, 2021

Uh oh!

Uh oh!

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 #20509

Uh oh!

Conversation

vasild commented Nov 26, 2020

Uh oh!

vasild commented Nov 26, 2020

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!

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!

luke-jr commented Nov 30, 2020

Uh oh!

vasild commented Dec 1, 2020

Uh oh!

DrahtBot commented Jun 17, 2021

Uh oh!

jonatack commented Jun 17, 2021

Uh oh!

maflcko commented Jun 21, 2021

Uh oh!

vasild commented Jun 21, 2021

Uh oh!

Uh oh!

jonatack left a comment •

edited

Loading

sipa commented Nov 27, 2020 •

edited

Loading

DrahtBot commented Nov 27, 2020 •

edited

Loading