Merge bitcoin/bitcoin#24312: addrman: Log too low compat value

fa097d074bc1afcc2a52976796bb618f7c6a68b3 addrman: Log too low compat value (MarcoFalke)

Pull request description:

  Before this patch, when writing a negative `lowest_compatible` value, it would be read as a positive value. For example `-32` will be read as `224`. There is generally nothing wrong with that. Though, similarly there shouldn't be anything wrong with refusing to read a negative value. I find the code after this patch more logical than before. Also, this allows dropping a file-wide sanitizer suppression.

  In practice none of this should ever happen. Bitcoin Core would never write a negative `lowest_compatible` in normal operation, unless the file storage is later corrupted by external influence.

ACKs for top commit:
  mzumsande:
    re-ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3

Tree-SHA512: 9aae7b8fe666f52f667f149667025e0160cef1a793cc4d392e36608f65c2bee8096da429235118f40a3368f327aabe30f3732ae78c5874648ea6f423f2687b65
This commit is contained in:
MarcoFalke 2022-03-08 16:48:18 +01:00 committed by pasta
parent d96983a327
commit 3383b79049
No known key found for this signature in database
GPG Key ID: E2F3D7916E722D38
3 changed files with 18 additions and 2 deletions

View File

@ -244,12 +244,18 @@ void AddrManImpl::Unserialize(Stream& s_)
uint8_t compat;
s >> compat;
if (compat < INCOMPATIBILITY_BASE) {
throw std::ios_base::failure(strprintf(
"Corrupted addrman database: The compat value (%u) "
"is lower than the expected minimum value %u.",
compat, INCOMPATIBILITY_BASE));
}
const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
if (lowest_compatible > FILE_FORMAT) {
throw InvalidAddrManVersionError(strprintf(
"Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
"but the maximum supported by this version of %s is %u.",
uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
uint8_t{format}, lowest_compatible, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
}
s >> nKey;

View File

@ -67,6 +67,17 @@ class AddrmanTest(BitcoinTestFramework):
self.start_node(0, extra_args=["-checkaddrman=1"])
assert_equal(self.nodes[0].getnodeaddresses(), [])
self.log.info("Check that addrman with negative lowest_compatible cannot be read")
self.stop_node(0)
write_addrman(peers_dat, lowest_compatible=-32)
self.nodes[0].assert_start_raises_init_error(
expected_msg=init_error(
"Corrupted addrman database: The compat value \\(0\\) is lower "
"than the expected minimum value 32.: (.+)"
),
match=ErrorMatch.FULL_REGEX,
)
self.log.info("Check that addrman from future is overwritten with new addrman")
self.stop_node(0)
write_addrman(peers_dat, lowest_compatible=111)

View File

@ -78,7 +78,6 @@ implicit-integer-sign-change:util/strencodings.h
implicit-integer-sign-change:validation.cpp
implicit-signed-integer-truncation,implicit-integer-sign-change:chain.h
implicit-signed-integer-truncation,implicit-integer-sign-change:test/skiplist_tests.cpp
implicit-signed-integer-truncation:addrman.cpp
implicit-signed-integer-truncation:addrman.h
implicit-signed-integer-truncation:chain.h
implicit-signed-integer-truncation:crypto/