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

vasild commented at 2:54 pm on November 26, 2020: member

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.

a55a3384ee

vasild commented at 3:03 pm on November 26, 2020: member

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 commented at 4:03 pm on November 26, 2020: member

ACK a55a3384ee5e4ade154965cd5f3ef503817b4a78

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

 0@@ -370,11 +370,11 @@ public:
 1     {
 2         SER_READ(obj, obj.nTime = TIME_INIT);
 3         int nVersion = s.GetVersion();
 4-        if (s.GetType() & SER_DISK) {
 5+        bool is_disk_ser{static_cast<bool>(s.GetType() & SER_DISK)};
 6+        if (is_disk_ser) {
 7             READWRITE(nVersion);
 8         }
 9-        if ((s.GetType() & SER_DISK) ||
10-            (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) {
11+        if (is_disk_ser || (nVersion != INIT_PROTO_VERSION && !(s.GetType() & SER_GETHASH))) {

DrahtBot added the label P2P on Nov 26, 2020

sipa commented at 1:45 am on November 27, 2020: member

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 commented at 2:30 am on November 27, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 5:49 pm on November 30, 2020: member

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

vasild commented at 1:34 pm on December 1, 2020: member

@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:

 0    SERIALIZE_METHODS(CAddress, obj)
 1    {
 2...
 3        int nVersion = s.GetVersion();
 4        if (s.GetType() & SER_DISK) {
 5            READWRITE(nVersion); // Saves the disk contents into nVersion
 6        }
 7...
 8        if (nVersion & ADDRV2_FORMAT) { // Uses nVersion (== disk contents) [1]
 9...
10        }
11        READWRITEAS(CService, obj); // Uses s.GetVersion() [2]
12    }

[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”:

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

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.

laanwj referenced this in commit 7b45c5e875 on Jun 17, 2021

DrahtBot commented at 5:46 pm on June 17, 2021: member

🐙 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”.

DrahtBot added the label Needs rebase on Jun 17, 2021

jonatack commented at 5:57 pm on June 17, 2021: member

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

sidhujag referenced this in commit 77b2f7a0f2 on Jun 18, 2021

MarcoFalke commented at 9:57 am on June 21, 2021: member

@vasild Closing for now

MarcoFalke closed this on Jun 21, 2021

vasild commented at 3:00 pm on June 21, 2021: member

Yes, from above:

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

Thanks!

DrahtBot locked this on Aug 18, 2022

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

Conflicts