addrman: ensure old versions don't parse peers.dat #20284

vasild commented at 11:03 AM on November 2, 2020: member

Even though the format of peers.dat was changed in a backwards incompatible way, it is not guaranteed that old versions will fail to parse it. There is a chance that old versions parse its contents as garbage and use it.

Old versions expect the "key size" field to be 32 and fail the parsing if it is not. Thus, we put something other than 32 in it. This will make versions between 0.11.0 and 0.20.1 deterministically fail on the new format. Versions prior to #5941 will still parse it as garbage.

Also, introduce a way to increment the peers.dat format in a way that does not necessary make older versions refuse to read it.

fanquake added the label P2P on Nov 2, 2020

vasild commented at 11:04 AM on November 2, 2020: member

@jnewbery spotted the potential problem here: #19954 (review)

vasild force-pushed on Nov 2, 2020

jnewbery commented at 1:29 PM on November 2, 2020: member

Concept ACK.

I think this requires a bit more discussion. I'm going to raise it as a meeting topic for the next p2p meeting (Nov 3).

Marking this as a 0.21 milestone, since we'll need to make a decision before that release.

jnewbery added this to the milestone 0.21.0 on Nov 2, 2020

vasild force-pushed on Nov 2, 2020

BARMOLEY007 approved

DrahtBot commented at 2:58 PM on November 2, 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:

#18722 (addrman: improve performance by using more suitable containers 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.

in src/addrman.h:439 in 5cde641689 outdated

 422 | +        uint8_t compat;
 423 | +        s >> compat;
 424 | +        const uint8_t lowest_compatible = compat - m_incompatibility_base;
 425 | +        if (lowest_compatible > maximum_supported_format) {
 426 | +            throw std::ios_base::failure(
 427 | +                strprintf("Unsupported format of addrman database: %u. It is compatible with "

laanwj commented at 3:07 PM on November 2, 2020:

Thanks for adding an actual versioning mechanism, at least next time we won't have to resort to a hack like this!

laanwj commented at 3:10 PM on November 2, 2020: member

Concept ACK.

jonatack commented at 3:44 PM on November 2, 2020: member

Concept/light code review ACK. I'll try to test this soon.

sdaftuar commented at 2:40 PM on November 3, 2020: member

Concept ACK.

promag commented at 12:51 AM on November 9, 2020: member

Concept ACK, could test compatibility check with a fake peers.dat file.

in src/addrman.h:351 in 5cde641689 outdated

 338 | @@ -332,7 +339,12 @@ friend class CAddrManTest;
 339 |          OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
 340 |  
 341 |          s << static_cast<uint8_t>(Format::V3_BIP155);

MarcoFalke commented at 6:44 AM on November 10, 2020:

unrelated style nit: Any reason why this can't use CustomUintFormatter (and its run-time sanity checks)?

vasild commented at 2:10 PM on November 10, 2020:

It is possible to use CustomUintFormatter here. This line is not otherwise modified by the patch, so it would be sneaking an unnecessary change, so I left it as is to ease reviewers and future history observers.

vasild commented at 4:32 PM on November 10, 2020:

Now I changed this line for other reasons and switched to CustomUintFormatter :-)

jnewbery commented at 9:42 AM on November 11, 2020:

re #20284 (review). I don't understand why we want this change. The comment for CustomUintFormatter says "This is only intended to implement serializers that are compatible with existing formats, and its use is not recommended for new data structures."

MarcoFalke commented at 9:50 AM on November 11, 2020:

The comment only applies to endianness, see #18317 (review). Generally, I think serialize/deserialze should use the same helpers, so either CustomUintFormatter should be used for both paths or for none.

in src/addrman.h:426 in 5cde641689 outdated

 424 | +        const uint8_t lowest_compatible = compat - m_incompatibility_base;
 425 | +        if (lowest_compatible > maximum_supported_format) {
 426 | +            throw std::ios_base::failure(
 427 | +                strprintf("Unsupported format of addrman database: %u. It is compatible with "
 428 | +                          "formats >=%u, but the maximum supported by this version of %s is %u. "
 429 | +                          "Continuing operation without using the saved list of peers.",

MarcoFalke commented at 6:51 AM on November 10, 2020:

stye-nit: I think the "continue" (and logging) is responsibility (and is already done) by the calling code that also catches the exception. No need to assume this exception is always caught.

vasild commented at 2:12 PM on November 10, 2020:

I agree. Removed the last sentence. Alas, the code that catches this exception is too generic and we can't print "Continuing operation without using the saved list of peers" from it.

MarcoFalke commented at 6:55 AM on November 10, 2020: member

review ACK 5cde64168976dea9e5d1422b23159413de1307dc

just some style questions

in src/addrman.h:192 in 5cde641689 outdated

 187 | +
 188 | +    //! The initial value of a field that is incremented every time an incompatible format
 189 | +    //! change is made. This is 32 instead of 0 because we overtook the "key size" field
 190 | +    //! which was 32 historically and we don't want to start with 0 and bump into 32 at
 191 | +    //! some point in the future.
 192 | +    static constexpr uint8_t m_incompatibility_base = 32;

jnewbery commented at 9:27 AM on November 10, 2020:

We usually use ALL_CAPS for constant names.

MarcoFalke commented at 10:19 AM on November 10, 2020:

nit: Any value equal to or greater than 32 should be fine here. As this is a breaking change (old versions can't read the peers.dat created by this version of Bitcoin Core), might as well bump to 33 right away to avoid confusion with the "magic value" 32.

jnewbery commented at 10:34 AM on November 10, 2020:

Or perhaps use 0x80 as a very obvious offset. We're unlikely to have 128 file versions so we're not going to run out of bits.

vasild commented at 2:39 PM on November 10, 2020:

Changed to INCOMPATIBILITY_BASE.

vasild commented at 3:18 PM on November 10, 2020:

I think it should be one of 29, 30, 31 or 32:

// Unserialize in 0.21
lowest_compatible = byte_read_from_disk - INCOMPATIBILITY_BASE;

0.20 write 32 to disk

So

If INCOMPATIBILITY_BASE is 28 or less then a file written by 0.20 will be rendered unreadable by 0.21 as having too high lowest_compatible: 4 or higher.
If INCOMPATIBILITY_BASE is one of 29, 30, 31 or 32 then a file written by 0.20 will appear to have lowest_compatible 3, 2, 1 or 0 which is ok for 0.21.
If INCOMPATIBILITY_BASE is 33 or greater then a file written by 0.20 will cause 0.21 to calculate a negative lowest_compatible.

So I left it as 32.

in src/addrman.h:417 in 5cde641689 outdated

 412 | @@ -410,9 +413,20 @@ friend class CAddrManTest;
 413 |  
 414 |          OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
 415 |  
 416 | -        unsigned char nKeySize;
 417 | -        s >> nKeySize;
 418 | -        if (nKeySize != 32) throw std::ios_base::failure("Incorrect keysize in addrman deserialization");
 419 | +        //! The maximum format this software can Unserialize().
 420 | +        static constexpr Format maximum_supported_format = Format::V3_BIP155;

jnewbery commented at 9:30 AM on November 10, 2020:

This is a very badly named constant! The whole point of this PR is that we want the code to be forward compatible if possible, and only in exceptional circumstances make format changes that aren't. If the format is bumped, but lowest_compatible is not, then older software will still be able to read it.

I'd suggest moving this to a class-level static constant called FILE_FORMAT or similar, and use it in both the serialize and unserialize functions.

vasild commented at 2:57 PM on November 10, 2020:

Renamed to maximum_known_format.

There is no use for such a constant in Serialize(). It is only necessary in Unserialize(), thus I think it is better to have it as local variable in Unserialize() in order to minimize its span.

jnewbery commented at 3:45 PM on November 10, 2020:

My point is that the "maximum known format" should always the same as the value that is serialized into the format version byte. It's the format that this software uses.

Defining a constant here means that it needs to be updated at the same time as line 351 where the version is serialized into the file.

vasild commented at 4:32 PM on November 10, 2020:

I see you point now. Moved as a member variable of the class, renamed to FILE_FORMAT and used in Serialize() too.

in src/addrman.h:361 in 5cde641689 outdated

 338 | @@ -332,7 +339,12 @@ friend class CAddrManTest;
 339 |          OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
 340 |  
 341 |          s << static_cast<uint8_t>(Format::V3_BIP155);
 342 | -        s << ((unsigned char)32);
 343 | +
 344 | +        // Increment `lowest_compatible` iff a newly introduced format is incompatible with

jnewbery commented at 9:34 AM on November 10, 2020:

I think this requires better documentation. At the very least, the * Serialized format. comment above needs updating. You might also add asmap_version while you're doing that, since the documentation wasn't updated correctly last time either.

I'd suggest something like:

"The lowest version of software that can read this peers.dat file. Update this to be the same as FILE_FORMAT when making a change to the file format that is not forwards compatible."

vasild commented at 2:58 PM on November 10, 2020:

I updated the * Serialized format comment.

in src/addrman.h:191 in 5cde641689 outdated

 186 | +    };
 187 | +
 188 | +    //! The initial value of a field that is incremented every time an incompatible format
 189 | +    //! change is made. This is 32 instead of 0 because we overtook the "key size" field
 190 | +    //! which was 32 historically and we don't want to start with 0 and bump into 32 at
 191 | +    //! some point in the future.

jnewbery commented at 9:46 AM on November 10, 2020:

"we don't want to bump into 32" isn't the reason we start at 32. The reason is that v0.20 and earlier wrote 32 into this field, so if we didn't use this offset, then minimum_version would be greater than the current version and v0.21 software wouldn't be able to read old peers.dat files. I'd just leave the explanation out.

vasild commented at 2:59 PM on November 10, 2020:

I'd just leave the explanation out.

Removed the last part of the comment.

jnewbery approved

jnewbery commented at 9:50 AM on November 10, 2020: member

Concept ACK

The commit log should s/backwards compatible/forwards compatible/. The change in v0.21 is backwards compatible, since the new code can read old files. The change breaks forwards compatibility since old code will not be able to read new files.

This isn't just a pedantic nit. It's the whole point of this PR, so let's get the terminology right.

vasild force-pushed on Nov 10, 2020

vasild commented at 3:21 PM on November 10, 2020: member

The commit log should s/backwards compatible/forwards compatible/.

I think the confusion with forward vs backward compatible <sup>1, 2, 3</sup> is because forward/backward is relative - from the point of view of the new software the old one is backwards and from the point of view of the old software the new one is forwards (in time). Also, there is software version and file format version which makes it further confusing when we consider old software and new file format and new software with old file format.

For example, Wikipedia contains this:

A data format is said to be backward compatible with its predecessor if every message or file that is valid under the old format is still valid, retaining its meaning under the new format.

Which means that our peers.dat format=3 is not backward compatible with its predecessor format=2.

The current master contains this:

0x20 + nKey (serialized as if it were a vector, for backward compatibility)

Do you think that should be s/backward/forward/?

I have reworded the commit message to explain the matters without using the words "backward" or "forward".

vasild commented at 3:24 PM on November 10, 2020: member

Addressed suggestions - reworded comments, a log message and renamed two variables.

in src/addrman.h:323 in 1c60aba1da outdated

 312 | +     *     in Bitcoin Core 0.23 which is compatible with format=3 and it is known that 0.21 will
 313 | +     *     be able to parse it, then 0.23 will write (format=4, lowest_compatible=3) in the
 314 | +     *     first two bytes of the file and so 0.21 will still try to parse it.
 315 | +     *   * Bitcoin Core 0.25 introduces a new incompatible format=5. It will write
 316 | +     *     (format=5, lowest_compatible=5) and so any versions that do not know how to parse
 317 | +     *     format=5 will not try to read the file.

jnewbery commented at 3:47 PM on November 10, 2020:

I'd suggest not using actual version numbers in this comment. At some point we'll have versions 0.23 and 0.25, and they might not use file versions 4 and 5, and this comment will be confusing.

vasild commented at 4:34 PM on November 10, 2020:

Changed to V, V+1 and V+2.

jnewbery commented at 4:17 PM on November 10, 2020: member

I think the confusion with forward vs backward compatible...

The fact that there's confusion is exactly the reason we should precise with our language. We have exact terminology for these concepts, so we should use it consistently to avoid future confusion:

software is backward-compatible if it can read old input formats
software is forward-compatible if it can gracefully read new input formats (ignoring the new parts it can't understand) => a change to an input format breaks forward-compatibility if the new file format can't be read by old software.

The current master contains this:

0x20 + nKey (serialized as if it were a vector, for backward compatibility)

Do you think that should be s/backward/forward/?

No, this is for backwards compatibility. That format change broke forward-compatibility explicitly by XOR'ing the number of new buckets with 2**30. Explicitly having the new software write the same value to this field means that the software can read both v0 and v1 without any branching logic at that point. That's the easiest way to achieve backward-compatibility.

vasild force-pushed on Nov 10, 2020

vasild commented at 4:38 PM on November 10, 2020: member

Addressed suggestions - reworded a comment and moved a local variable as a member of the class.

jnewbery commented at 4:45 PM on November 10, 2020: member

utACK eaa04bec653428091a9434fa08253a9b89e01fd6

Functionally this is fine and can be merged for 0.21. However, I think that the comments are still very confused/confusing and could be tidied up with precise terminology and clearer explanations.

MarcoFalke commented at 5:13 PM on November 10, 2020: member

looks like this doesn't compile:

/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./addrman.h:438: undefined reference to `CAddrMan::FILE_FORMAT'

vasild force-pushed on Nov 10, 2020

vasild commented at 5:14 PM on November 10, 2020: member

looks like this doesn't compile:

/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./addrman.h:438: undefined reference to `CAddrMan::FILE_FORMAT'

Pushed a blind attempt to fix it (it does compile locally).

sipa commented at 5:23 PM on November 10, 2020: member

software is backward-compatible if it can read old input formats

software is forward-compatible if it can gracefully read new input formats (ignoring the new parts it can't understand) => a change to an input format breaks forward-compatibility if the new file format can't be read by old software.

Ok, so backward and forward compatibility is a property of the software, not of the formats.

No, this is for backwards compatibility. That format change broke forward-compatibility explicitly by XOR'ing the number of new buckets with 2**30.

I'm not sure that's true according to your definition above. The writing of the 0x20 prefix is there such that old software can still read the nKey (as it used to be serialized as a vector) - the new software doesn't care about the prefix. So I think your definition above says this is forward compatibility: old software reading the new format gracefully.

The same is true about the XORing of the bucket count with 2**30. It's there to trigger a re-bucketing if read by old software, which means it deals with the new format, albeit with reduced functionality (the bucket assignments aren't retained).

V3 is the first actual incompatibility that was introduced: old software cannot read addrv2 encoding at all. It's arguably still "graceful" in that old Bitcoin Core keeps working when it finds a future peers.dat file, but as it means the same as starting with no peers.dat I think it's fair to call that incompatible.

I think the compatibility table is something like this:

Format version	Software V0	Software V1	Software V2	Software V3
V0	✓	R	R	R
V1	R	✓	R	R
V2	R	R	✓	✓
V3	❌	❌	❌	✓

Where "R" means "compatible but may trigger rebucketing".

The upper right triangle represents backward compatibility (and is all R/✓, so we're good). The bottom left triangle represents forward compatibility, and V3 breaks that.

jnewbery commented at 6:32 PM on November 10, 2020: member

So I think your definition above says this is forward compatibility: old software reading the new format gracefully.

The same is true about the XORing of the bucket count with 2**30. It's there to trigger a re-bucketing if read by old software, which means it deals with the new format, albeit with reduced functionality (the bucket assignments aren't retained).

Ah! Those both make sense. Thanks!

MarcoFalke approved

MarcoFalke commented at 8:48 AM on November 11, 2020: member

review ACK b303dc0d1d487f68220a3a7ba0fea656a69a9b13 🛫

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 4c934b14bbb3c840a64b1314b685fd79bf97c3e4a990a9fcec19dbb0b7b0ea8d -

</details>

jnewbery commented at 10:28 AM on November 11, 2020: member

Pushed a blind attempt to fix it

I think it'd be better to fix this by just casting the enum to an int in the unserialize exception:

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 0067a32994..7636c6bad2 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -71,8 +71,6 @@ double CAddrInfo::GetChance(int64_t nNow) const
     return fChance;
 }
 
-constexpr CAddrMan::Format CAddrMan::FILE_FORMAT;
-
 CAddrInfo* CAddrMan::Find(const CNetAddr& addr, int* pnId)
 {
     std::map<CNetAddr, int>::iterator it = mapAddr.find(addr);
diff --git a/src/addrman.h b/src/addrman.h
index 8c9ed3f563..a18124dcad 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -437,7 +437,7 @@ public:
             throw std::ios_base::failure(
                 strprintf("Unsupported format of addrman database: %u. It is compatible with "
                           "formats >=%u, but the maximum supported by this version of %s is %u.",
-                          format, lowest_compatible, PACKAGE_NAME, FILE_FORMAT));
+                          format, lowest_compatible, PACKAGE_NAME, static_cast<uint8_t>(FILE_FORMAT)));
         }
 
         s >> nKey;

vasild force-pushed on Nov 11, 2020

in doc/release-notes.md:69 in 2ed531dfcd outdated

  64 | @@ -65,8 +65,8 @@ format of this file has been changed in a backwards-incompatible way in order to
  65 |  accommodate the storage of Tor v3 and other BIP155 addresses. This means that if
  66 |  the file is modified by 0.21.0 or newer then older versions will not be able to
  67 |  read it. Those old versions, in the event of a downgrade, will log an error
  68 | -message that deserialization has failed and will continue normal operation
  69 | -as if the file was missing, creating a new empty one. (#19954)
  70 | +message "Incorrect keysize in addrman deserialization" and will continue normal
  71 | +operation as if the file was missing, creating a new empty one. (#19954)

jonatack commented at 11:55 AM on November 11, 2020:

I could be mistaken, but ISTM it's too late to make changes here; the release notes have been migrated to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft where you can make this change manually.

vasild commented at 12:23 PM on November 11, 2020:

ok, if this gets merged then I will also update that wiki

MarcoFalke commented at 3:43 PM on November 11, 2020:

operation as if the file was missing, creating a new empty one. (#19954, [#20284](/bitcoin-bitcoin/20284/))

for the release notes wiki (not this pull)

Edit: done here: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft/_compare/ef1291e4985154b703bc5c87decdf68dbe0fcc20...c98b735a5bbf337f9f7bb065b56f490804cf9ae7

MarcoFalke commented at 3:46 PM on November 11, 2020:

Done here: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.21.0-Release-Notes-Draft/_compare/ef1291e4985154b703bc5c87decdf68dbe0fcc20...c98b735a5bbf337f9f7bb065b56f490804cf9ae7

jnewbery commented at 11:59 AM on November 11, 2020: member

utACK 2ed531dfcd9142cd1d16526dbea0aa45d991786b

in src/addrman.h:316 in 2ed531dfcd outdated

 309 | @@ -295,8 +310,17 @@ friend class CAddrManTest;
 310 |  
 311 |      /**
 312 |       * Serialized format.
 313 | -     * * version byte (@see `Format`)
 314 | -     * * 0x20 + nKey (serialized as if it were a vector, for backward compatibility)
 315 | +     * * format version byte (@see `Format`)
 316 | +     * * lowest compatible format version byte. This is used to help old software decide
 317 | +     *   whether to parse the file. For example:
 318 | +     *   * Bitcoin Core V knows how to parse up to format=3. If a new format=4 is introduced

jonatack commented at 12:08 PM on November 11, 2020:

Bitcoin Core V? These look like Roman numerals (anyone remember the Saturn V rocket?)

Perhaps:

-     *   whether to parse the file. For example:
-     *   * Bitcoin Core V knows how to parse up to format=3. If a new format=4 is introduced
-     *     in Bitcoin Core V+1 which is compatible with format=3 and it is known that V will
-     *     be able to parse it, then V+1 will write (format=4, lowest_compatible=3) in the
-     *     first two bytes of the file and so V will still try to parse it.
-     *   * Bitcoin Core V+2 introduces a new incompatible format=5. It will write
+     *   whether to parse the file.
+     *   For example:
+     *   * Bitcoin Core version n knows how to parse up to format=3. If a new format=4 is introduced
+     *     in version n + 1 that is compatible with format=3 and it is known that version n will
+     *     be able to parse it, then version n + 1 will write (format=4, lowest_compatible=3) in the
+     *     first two bytes of the file, and so version n will still try to parse it.
+     *   * Bitcoin Core version n + 2 introduces a new incompatible format=5. It will write

vasild commented at 12:22 PM on November 11, 2020:

Will tweak if I retouch :)

vasild commented at 1:36 PM on November 11, 2020:

Reworded.

laanwj commented at 1:21 PM on November 12, 2020:

I am somewhat partial to the idea of a roman numerals based version scheme :slightly_smiling_face: upcoming release will be Bitcoin Core XXI lol

jonatack commented at 12:17 PM on November 11, 2020: member

Testing the latest push.

vasild force-pushed on Nov 11, 2020

vasild commented at 1:41 PM on November 11, 2020: member

I think it'd be better to fix this by just casting the enum to an int in the unserialize exception:

That did not compile. Went back to the previous version and addressed Saturn V.

This should be the final version of it.

addrman: ensure old versions don't parse peers.dat

Even though the format of `peers.dat` was changed in an incompatible
way (old software versions <0.21 cannot understand the new file format),
it is not guaranteed that old versions will fail to parse it. There is a
chance that old versions parse its contents as garbage and use it.

Old versions expect the "key size" field to be 32 and fail the parsing
if it is not. Thus, we put something other than 32 in it. This will make
versions between 0.11.0 and 0.20.1 deterministically fail on the new
format. Versions prior to https://github.com/bitcoin/bitcoin/pull/5941
(<0.11.0) will still parse it as garbage.

Also, introduce a way to increment the `peers.dat` format in a way that
does not necessary make older versions refuse to read it.

38ada892ed

vasild force-pushed on Nov 11, 2020

vasild commented at 3:24 PM on November 11, 2020: member

Nothing is final!

MarcoFalke approved

MarcoFalke commented at 3:29 PM on November 11, 2020: member

re-ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419 🥐

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 5da911553b7e1b622981b630b830f42113eaf33856aeae205d8a024c377ec42e -

</details>

jnewbery commented at 3:37 PM on November 11, 2020: member

ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419

Thanks @vasild!

MarcoFalke commented at 12:31 PM on November 12, 2020: member

(the timed-out test run can be ignored)

laanwj commented at 4:05 PM on November 12, 2020: member

Code review ACK 38ada892ed0ed9aaa46b1791db12a371a3c0c419

laanwj merged this on Nov 12, 2020

laanwj closed this on Nov 12, 2020

vasild deleted the branch on Nov 12, 2020

sidhujag referenced this in commit 216ac49972 on Nov 12, 2020

deadalnix referenced this in commit 5be9d5e9c4 on Feb 11, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Feb 15, 2022