addrman: Log too low compat value #24312

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2202-logNeg changing 3 files +18 −2
  1. MarcoFalke commented at 4:12 PM on February 10, 2022: member

    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.

  2. MarcoFalke added the label Refactoring on Feb 10, 2022
  3. MarcoFalke added the label Utils/log/libs on Feb 10, 2022
  4. DrahtBot commented at 11:00 AM on February 11, 2022: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #23962 (Use int32_t type for transaction size/weight consistently by hebasto)

    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.

  5. in src/addrman.cpp:249 in fa4f9311c7 outdated 
     245 | @@ -246,12 +246,12 @@ void AddrManImpl::Unserialize(Stream& s_)
 246 |  
 247 |      uint8_t compat;
 248 |      s >> compat;
 249 | -    const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
 250 | -    if (lowest_compatible > FILE_FORMAT) {
 251 | +    const int lowest_compatible{int{compat} - int{INCOMPATIBILITY_BASE}};
    shaavan commented at 12:29 PM on February 11, 2022:

    Doubt: If I understood it correctly, changing the type of lowest_compatible to 32-bit signed integer would have been enough. So was there any specific reason to choose a 64-bit integer for the type?

    MarcoFalke commented at 12:49 PM on February 11, 2022:

    int is 32-bit on all of our supported platforms. See compat/assumptions.h. I could change to int32_t, but is there a reason?

    shaavan commented at 2:48 PM on February 11, 2022:

    is there a reason?

    Not any specific reason. This was more of a question out of curiosity rather than a doubt. I have observed that we avoid going with a higher size type for the variable when we can sufficiently work with a smaller size type; that's why I asked the question.

    See compat/assumptions.h.

    Let me take a look through it.

    vasild commented at 4:01 PM on February 11, 2022:

    I think it should be like this:

    if (compat < INCOMPATIBILITY_BASE) {
    throw std::ios_base::failure(strprintf("Corrupted peers.dat: the compat value is lower than the expected 32"));
}
// original code follows that is now guaranteed not to underflow
const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
...

    The proposed change in this PR is ok in a sense that it will fix the underflow if too low value is read from disk. However, if that happens, then the printed message would be puzzling and we have to adjust that too. Normally, the meaning of the message is this:

    it is compatible with >=7, but I can only read up to 5 (so it is too high for me because 7>5)

    and with a bricked peers.dat it could be:

    it is compatible with >=-10, but I can only read up to 5 (so it is too high for me because -10>5)

    it is puzzling because -10 is not higher than 5.

    MarcoFalke commented at 7:44 PM on February 11, 2022:

    Thx, done

    MarcoFalke commented at 7:44 PM on February 11, 2022:

    Reverted hunk

  6. in src/addrman.cpp:252 in fa4f9311c7 outdated 
     250 | -    if (lowest_compatible > FILE_FORMAT) {
 251 | +    const int lowest_compatible{int{compat} - int{INCOMPATIBILITY_BASE}};
 252 | +    if (lowest_compatible < 0 || lowest_compatible > FILE_FORMAT) {
 253 |          throw std::ios_base::failure(strprintf(
 254 | -            "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
 255 | +            "Unsupported format of addrman database: %i. It is compatible with formats >=%u, "
    shaavan commented at 12:33 PM on February 11, 2022:

    nit:

                "Unsupported format of addrman database: %u. It is compatible with formats >=%i, "
    MarcoFalke commented at 7:44 PM on February 11, 2022:

    Reverted hunk

  7. MarcoFalke force-pushed on Feb 11, 2022
  8. MarcoFalke force-pushed on Feb 11, 2022
  9. MarcoFalke renamed this:
    addrman: Log negative lowest_compatible
    addrman: Log too low compat value
    on Feb 11, 2022
  10. unknown approved
  11. unknown commented at 1:11 AM on February 12, 2022: none

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/24312/commits/fac32775c8e451527b48e855aebd13b7ed0e820d

    I wish we had a hidden RPC for:

    write_addrman(peers_dat, lowest_compatible=-32)
  12. MarcoFalke commented at 7:18 AM on February 12, 2022: member

    I wish we had a hidden RPC for:

    I wish we don't. Generally I am not a fan of baking test code into the production system, unless there is a need for it.

  13. shaavan approved
  14. shaavan commented at 10:25 AM on February 12, 2022: contributor

    ACK fac32775c8e451527b48e855aebd13b7ed0e820d

    Changes since my last review:

    • Instead of changing the type of lowest_compatible from unsigned to signed integer, a condition statement has been added, which removes the need for signed lowest_compatible.

    I think the wording of added failure message is correct and adequate. I experimented with several different values of the lowest_compatible argument in the test, and each time the error message remained logical and consistent, with changing values of compat.

    Note: The test failed with lowest_compatible=-33, as this would make compat=-1, which is out of range, but I don't think this condition would ever arise in real-life situations.

  15. in src/addrman.cpp:252 in fac32775c8 outdated 
     245 | @@ -246,6 +246,11 @@ void AddrManImpl::Unserialize(Stream& s_)
 246 |  
 247 |      uint8_t compat;
 248 |      s >> compat;
 249 | +    if (compat < INCOMPATIBILITY_BASE) {
 250 | +        throw std::ios_base::failure(strprintf("Corrupted addrman database: The compat value (%u) "
 251 | +                                               "is lower than the expected %u.",
 252 | +                                               uint8_t{compat}, uint8_t{INCOMPATIBILITY_BASE}));
    vasild commented at 11:28 AM on February 14, 2022:

    Why is it necessary to cast the variable to the same type it already is!? I read #22879 but it seems that issue is only with enums? I think we have a general problem with strprintf() if it must be used like this (for all int/uintX_t types):

    uint8_t x;
strprintf("%u", uint8_t{x});
    MarcoFalke commented at 12:12 PM on February 14, 2022:

    Yes, it is only needed for FILE_FORMAT. Should I remove it from the rest?

    vasild commented at 1:16 PM on February 14, 2022:

    It is unnecessary, I think it is better without it.

    MarcoFalke commented at 4:36 PM on February 14, 2022:

    Thanks, removed.

  16. vasild commented at 11:30 AM on February 14, 2022: member

    Is the first commit test: Remove no longer needed suppressions related to the low compat issue? The commit message does not explain why the suppressions are no longer needed.

  17. vasild commented at 11:36 AM on February 14, 2022: member

    The test failed with lowest_compatible=-33, as this would make compat=-1, which is out of range, but I don't think this condition would ever arise in real-life situations.

    The test fails with struct.error: ubyte format requires 0 <= number <= 255 because the python test code refuses to write -1 with the B format (see https://docs.python.org/3/library/struct.html) to disk. The addrman deserialization is not even invoked in this case.

  18. MarcoFalke commented at 12:14 PM on February 14, 2022: member

    A compat value of -1 doesn't exist. compat is unsigned in both Bitcoin Core and the python mock.

  19. MarcoFalke force-pushed on Feb 14, 2022
  20. in src/addrman.cpp:259 in fab4df17fc outdated 
     255 |      if (lowest_compatible > FILE_FORMAT) {
 256 |          throw std::ios_base::failure(strprintf(
 257 |              "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
 258 |              "but the maximum supported by this version of %s is %u.",
 259 | -            uint8_t{format}, uint8_t{lowest_compatible}, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
 260 | +            format, lowest_compatible, PACKAGE_NAME, uint8_t{FILE_FORMAT}));
    vasild commented at 8:46 AM on February 15, 2022:

    The format variable is of type enum Format : uint8_t and I think it hit that bug in tinyformat. It printed a symbol with ASCII code 1 instead of the number 1.

    6461 7461 6261 7365 3a20 012e         database: ..
                         ^^                     ^
    MarcoFalke commented at 8:52 AM on February 15, 2022:

    thanks, fixed

  21. MarcoFalke force-pushed on Feb 15, 2022
  22. vasild approved
  23. vasild commented at 1:41 PM on February 15, 2022: member

    ACK faaa6c2af56eedaf8d50b33015d98c7bc289c2f4

    What about #24312#pullrequestreview-881497874?

  24. fanquake requested review from mzumsande on Feb 23, 2022
  25. in src/addrman.cpp:251 in faaa6c2af5 outdated 
     245 | @@ -246,12 +246,17 @@ void AddrManImpl::Unserialize(Stream& s_)
 246 |  
 247 |      uint8_t compat;
 248 |      s >> compat;
 249 | +    if (compat < INCOMPATIBILITY_BASE) {
 250 | +        throw std::ios_base::failure(strprintf("Corrupted addrman database: The compat value (%u) "
 251 | +                                               "is lower than the expected %u.",
    mzumsande commented at 8:48 PM on February 23, 2022:

    INCOMPATIBILITY_BASE isn't the expected compat value but a lower bound: expected are values >= INCOMPATIBILITY_BASE.

    MarcoFalke commented at 7:36 AM on February 24, 2022:

    Ok, I can see how this can be misinterpreted.

    What about:

                                                   "is lower than the expected minimum value %u.",
    mzumsande commented at 1:38 PM on February 24, 2022:

    Yes, I'd prefer that.

    MarcoFalke commented at 1:49 PM on February 24, 2022:

    thanks, done

  26. mzumsande commented at 8:53 PM on February 23, 2022: member

    Code Review ACK faaa6c2af56eedaf8d50b33015d98c7bc289c2f4 - also did some light testing.

  27. MarcoFalke force-pushed on Feb 24, 2022
  28. fanquake requested review from vasild on Feb 24, 2022
  29. fanquake requested review from mzumsande on Feb 24, 2022
  30. mzumsande commented at 3:12 PM on February 24, 2022: member

    ACK fa0fb76b7772eda83ec9c7030911a63331f590c5

    Only change: Rewording of the error message

  31. DrahtBot added the label Needs rebase on Feb 25, 2022
  32. MarcoFalke force-pushed on Feb 25, 2022
  33. DrahtBot removed the label Needs rebase on Feb 25, 2022
  34. addrman: Log too low compat value
    Also remove uint8_t{} casts from values that are already of the same
type.
    fa097d074b
  35. MarcoFalke force-pushed on Feb 25, 2022
  36. mzumsande commented at 1:21 PM on March 2, 2022: member

    re-ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3

  37. vasild approved
  38. vasild commented at 2:40 PM on March 8, 2022: member

    ACK fa097d074bc1afcc2a52976796bb618f7c6a68b3

    Even better wording, thanks @mzumsande!

  39. MarcoFalke deleted a comment on Mar 8, 2022
  40. MarcoFalke merged this on Mar 8, 2022
  41. MarcoFalke closed this on Mar 8, 2022
  42. MarcoFalke deleted the branch on Mar 8, 2022
  43. sidhujag referenced this in commit 65d7e58a8f on Mar 8, 2022
  44. Fabcien referenced this in commit afbc199fea on Oct 21, 2022
  45. DrahtBot locked this on Mar 8, 2023
Contributors
MarcoFalkeDrahtBotshaavanvasildmzumsande


mzumsande

Labels
RefactoringUtils/log/libs
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me