init: Fix asmap/addrman initialization order bug #22791

pull jnewbery wants to merge 6 commits into bitcoin:master from jnewbery:2021-08-asmap-addrman-init changing 13 files +92 −92
  1. jnewbery commented at 11:34 am on August 24, 2021: member

    Commit 181a1207 introduced an initialization order bug: CAddrMan’s m_asmap must be set before deserializing peers.dat.

    The first commit restores the correct initialization order. The remaining commits make CAddrMan::m_asmap usage safer:

    • don’t reach into CAddrMan’s internal data from CConnMan
    • set m_asmap in the initializer list and make it const
    • make m_asmap private, and access it (as a reference to const) from a getter.

    This ensures that peers.dat deserialization must happen after setting m_asmap, since m_asmap is set during CAddrMan construction.

  2. fanquake added the label P2P on Aug 24, 2021
  3. jnewbery renamed this:
    [init] Read/decode asmap before constructing addrman
    init: Fix asmap/addrman initialization order bug
    on Aug 24, 2021
  4. jonatack commented at 1:34 pm on August 24, 2021: member

    I’m unable to build (with clang 13, CI seems to agree). The first commit compiles; testing.

     0bench/addrman.cpp:76:18: error: no matching constructor for initialization of 'CAddrMan'
     1        CAddrMan addrman{/* deterministic */ false, /* consistency_check_ratio */ 0};
     2                 ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     3./addrman.h:181:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
     4class CAddrMan
     5      ^
     6./addrman.h:458:14: note: candidate constructor not viable: requires 3 arguments, but 2 were provided
     7    explicit CAddrMan(std::vector<bool>* asmap, bool deterministic, int32_t consistency_check_ratio);
     8             ^
     9bench/addrman.cpp:83:14: error: no matching constructor for initialization of 'CAddrMan'
    10    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
    11             ^                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    12./addrman.h:181:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
    13class CAddrMan
    14      ^
    15./addrman.h:458:14: note: candidate constructor not viable: requires 3 arguments, but 2 were provided
    16    explicit CAddrMan(std::vector<bool>* asmap, bool deterministic, int32_t consistency_check_ratio);
    17             ^
    18bench/addrman.cpp:95:14: error: no matching constructor for initialization of 'CAddrMan'
    19    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
    20             ^                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    21./addrman.h:181:7: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
    22class CAddrMan
    23      ^
    24./addrman.h:458:14: note: candidate constructor not viable: requires 3 arguments, but 2 were provided
    25    explicit CAddrMan(std::vector<bool>* asmap, bool deterministic, int32_t consistency_check_ratio);
    26             ^
    
  5. jnewbery force-pushed on Aug 24, 2021
  6. jnewbery commented at 1:43 pm on August 24, 2021: member

    I’m unable to build (with clang 13, CI seems to agree)

    Oops. I don’t build with bench locally to save a bit of time. Now fixed.

  7. jonatack commented at 2:13 pm on August 24, 2021: member

    Test with only the first commit: the addrman checks pass throughout, but I lost 18k addresses on startup, perhaps because I had the -asmap config option commented out to be able to run bitcoind on mainnet, and I uncommented it again to test.

    02021-08-24T13:39:14Z [init] Opened asmap file "/home/jon/projects/bitcoin/asmap/demo.map" (932999 bytes) from disk
    12021-08-24T13:39:15Z [init] Using asmap version 7ead927689f884bc4021618017cc1438d9328578779edd2adfbed044fb52928e for IP bucketing
    22021-08-24T13:39:15Z [init] init message: Loading P2P addresses…
    32021-08-24T13:39:16Z [init] Bucketing method was updated, re-bucketing addrman entries from disk
    42021-08-24T13:39:25Z [init] addrman lost 18599 new and 1 tried addresses due to collisions or invalid addresses
    52021-08-24T13:39:25Z [init] Addrman checks started: new 26373, tried 72, total 26445
    62021-08-24T13:39:25Z [init] Addrman checks completed successfully
    72021-08-24T13:39:25Z [init] Loaded 26445 addresses from peers.dat  9577ms
    

    Update: deleted my peers.dat and re-tried building on the PR branch after the update; debug build now succeeds and retesting with multiple restarts initially seems fine.

    02021-08-24T14:11:09Z [init] Opened asmap file "/home/jon/projects/bitcoin/asmap/demo.map" (932999 bytes) from disk
    12021-08-24T14:11:11Z [init] Using asmap version 7ead927689f884bc4021618017cc1438d9328578779edd2adfbed044fb52928e for IP bucketing
    22021-08-24T14:11:11Z [init] init message: Loading P2P addresses…
    32021-08-24T14:11:11Z [init] Addrman checks started: new 6828, tried 10, total 6838
    42021-08-24T14:11:12Z [init] Addrman checks completed successfully
    52021-08-24T14:11:12Z [init] Loaded 6838 addresses from peers.dat  756ms
    
     02021-08-24T13:41:49Z [init] Config file arg: blockfilterindex="1"
     12021-08-24T13:41:49Z [init] Config file arg: checkaddrman="1"
     22021-08-24T13:41:49Z [init] Config file arg: checkblocks="12"
     32021-08-24T13:41:49Z [init] Config file arg: checklevel="4"
     42021-08-24T13:41:49Z [init] Config file arg: checkmempool="1"
     52021-08-24T13:41:49Z [init] Config file arg: coinstatsindex="1"
     62021-08-24T13:41:49Z [init] Config file arg: debug="tor"
     72021-08-24T13:41:49Z [init] Config file arg: debug="i2p"
     82021-08-24T13:41:49Z [init] Config file arg: debug="addrman"
     92021-08-24T13:41:49Z [init] Config file arg: i2psam="127.0.0.1:7656"
    102021-08-24T13:41:49Z [init] Config file arg: logips="1"
    112021-08-24T13:41:49Z [init] Config file arg: logthreadnames="1"
    122021-08-24T13:41:49Z [init] Config file arg: par="1"
    132021-08-24T13:41:49Z [init] Config file arg: peerbloomfilters="1"
    142021-08-24T13:41:49Z [init] Config file arg: txindex="1"
    152021-08-24T13:41:49Z [init] Config file arg: walletbroadcast="1"
    162021-08-24T13:41:49Z [init] Config file arg: [main] asmap="/home/jon/projects/bitcoin/asmap/demo.map"
    
  8. jonatack commented at 2:30 pm on August 24, 2021: member

    Restarted after turning off -asmap:

    02021-08-24T14:19:00Z [init] Using /16 prefix for IP bucketing
    12021-08-24T14:19:00Z [init] init message: Loading P2P addresses…
    22021-08-24T14:19:01Z [init] Bucketing method was updated, re-bucketing addrman entries from disk
    32021-08-24T14:19:01Z [init] addrman lost 2147 new and 0 tried addresses due to collisions or invalid addresses
    42021-08-24T14:19:01Z [init] Loaded 9928 addresses from peers.dat  463ms
    

    Restarted again with -asmap off:

    02021-08-24T14:20:59Z [init] Using /16 prefix for IP bucketing
    12021-08-24T14:20:59Z [init] init message: Loading P2P addresses…
    22021-08-24T14:21:00Z [init] Loaded 10760 addresses from peers.dat  169ms
    

    Restarted with -asmap back on:

    02021-08-24T14:22:44Z [init] Opened asmap file "/home/jon/projects/bitcoin/asmap/demo.map" (932999 bytes) from disk
    12021-08-24T14:22:46Z [init] Using asmap version 7ead927689f884bc4021618017cc1438d9328578779edd2adfbed044fb52928e for IP bucketing
    22021-08-24T14:22:46Z [init] init message: Loading P2P addresses…
    32021-08-24T14:22:46Z [init] Bucketing method was updated, re-bucketing addrman entries from disk
    42021-08-24T14:22:48Z [init] addrman lost 510 new and 0 tried addresses due to collisions or invalid addresses
    52021-08-24T14:22:48Z [init] Loaded 10462 addresses from peers.dat  2802ms
    

    Restarted again with -asmap on and reduced frequency of checkaddrman=100, leaving running this way for now:

    02021-08-24T14:24:41Z [init] Opened asmap file "/home/jon/projects/bitcoin/asmap/demo.map" (932999 bytes) from disk
    12021-08-24T14:24:43Z [init] Using asmap version 7ead927689f884bc4021618017cc1438d9328578779edd2adfbed044fb52928e for IP bucketing
    22021-08-24T14:24:43Z [init] init message: Loading P2P addresses…
    32021-08-24T14:24:43Z [init] Loaded 11203 addresses from peers.dat  737ms
    4...
    52021-08-24T14:30:12Z [msghand] Addrman checks started: new 15479, tried 19, total 15498
    62021-08-24T14:30:13Z [msghand] Addrman checks completed successfully
    

    Addrman checks passing throughout.

  9. in src/init.cpp:1179 in 4ffc8b81bc outdated
    1177     {
    1178+        // Initialize addrman
    1179+        assert(!node.addrman);
    1180+
    1181+        // Read asmap file if configured
    1182+        std::vector<bool> asmap{};
    


    vasild commented at 4:01 pm on August 24, 2021:
    What is the point of {}?

    jonatack commented at 4:37 pm on August 24, 2021:

    47ad0fd2644c4de7a11efa9c9e288f73d6c147e5 naming nit: asmap_data would allow more easily grepping for / distinguishing this new data structure in the code

    0        std::vector<bool> asmap_data;
    

    jnewbery commented at 12:24 pm on August 25, 2021:
    Removed

    jnewbery commented at 12:31 pm on August 25, 2021:
    Good suggestion, but will hold off doing it in this PR to reduce the diff. This part is mostly move-only.
  10. in src/addrman.h:580 in 4ffc8b81bc outdated
    576@@ -593,6 +577,8 @@ class CAddrMan
    577         Check();
    578     }
    579 
    580+    const std::vector<bool>& GetAsmap() { return m_asmap; }
    


    jonatack commented at 4:15 pm on August 24, 2021:

    4ffc8b81bc520fe66fb is there any benefit to making m_asmap private rather than a public data member? referencing our previous conversation:

    (otherwise, make the getter const?)

    0    const std::vector<bool>& GetAsmap() const { return m_asmap; }
    

    vasild commented at 4:17 pm on August 24, 2021:

    nit:

    0    const std::vector<bool>& GetAsmap() const { return m_asmap; }
    

    jnewbery commented at 12:24 pm on August 25, 2021:
    Thanks. Done.

    jnewbery commented at 12:31 pm on August 25, 2021:
    I’ve made the getter const. I think removing the only public data member of CAddrMan is an improvement.

    vasild commented at 7:50 am on August 26, 2021:
  11. in src/test/fuzz/addrman.cpp:313 in 4ffc8b81bc outdated
    311-    CAddrManDeterministic addr_man2{fuzzed_data_provider};
    312-    addr_man2.m_asmap = addr_man1.m_asmap;
    313+    std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);
    314+    if (!SanityCheckASMap(asmap)) {
    315+        asmap.clear();
    316+    }
    


    vasild commented at 4:19 pm on August 24, 2021:
    This pattern asmap = Consume(); if (!Sanity()) { asmap.clear(); } is used in two places. Maybe not worth it, but consider adding ConsumeASMap() to deduplicate it.

    jnewbery commented at 12:30 pm on August 25, 2021:
    Done
  12. vasild commented at 4:20 pm on August 24, 2021: member
    ACK 4ffc8b81bc520fe66fbea9a597ab889a2647ca2e
  13. in src/init.cpp:1218 in 4ffc8b81bc outdated
    1210@@ -1183,11 +1211,12 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    1211             LogPrintf("Loaded %i addresses from peers.dat  %dms\n", node.addrman->size(), GetTimeMillis() - nStart);
    1212         } else {
    1213             // Addrman can be in an inconsistent state after failure, reset it
    1214-            node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
    1215+            node.addrman = std::make_unique<CAddrMan>(&asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
    1216             LogPrintf("Recreating peers.dat\n");
    1217             adb.Write(*node.addrman);
    1218         }
    1219     }
    


    jonatack commented at 4:39 pm on August 24, 2021:
    47ad0fd2644c4de7a11efa9c9e288f73d6c147e5 nit, why these outer brackets?

    jnewbery commented at 12:32 pm on August 25, 2021:
    It limits the scope and makes it obvious that local variables like asmap and adb are not used outside this code block.

    vasild commented at 7:53 am on August 26, 2021:
    Functions do that better.
  14. jonatack commented at 4:46 pm on August 24, 2021: member

    Concept ACK 4ffc8b81bc520fe66fbea9a597ab889a2647ca2e modulo comments below and feedback from fuzzing people wrt invalidating affected corpii

    Suggestions:

  15. DrahtBot commented at 4:39 am on August 25, 2021: 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:

    • #22831 (p2p, bugfix: fix addrman tried table corruption on restart with asmap by jonatack)
    • #22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
    • #22766 (refactor: Clarify and disable unused ArgsManager flags by ryanofsky)
    • #22762 (Raise InitError when peers.dat is invalid or corrupted by MarcoFalke)
    • #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
    • #19860 (Improve diversification of new connections: privacy and stability by naumenkogs)
    • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)

    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.

  16. [init] Read/decode asmap before constructing addrman
    Commit 181a1207 introduced an initialization order bug: CAddrMan's
    m_asmap must be set before deserializing peers.dat. Restore that
    ordering.
    
    review hint: use
    
    `git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
    50fd77045e
  17. [net] Remove CConnMan::SetAsmap()
    CAddrMan::m_asmap is now set directly in AppInitMain() so
    CConnMan::SetAsmap() is no longer required.
    593247872d
  18. jnewbery commented at 12:35 pm on August 25, 2021: member
    Thanks for the reviews @vasild @jonatack. I’ve addressed all your inline comments. @jonatack - If you have any suggested wording, I’m happy to incorporate it into the PR description.
  19. jnewbery force-pushed on Aug 25, 2021
  20. in src/test/fuzz/addrman.cpp:232 in 8a4c815dfe outdated
    228 FUZZ_TARGET_INIT(addrman, initialize_addrman)
    229 {
    230     FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
    231     SetMockTime(ConsumeTime(fuzzed_data_provider));
    232-    auto addr_man_ptr = std::make_unique<CAddrManDeterministic>(fuzzed_data_provider);
    233+    std::vector<bool> asmap = ConsumeAsmap(fuzzed_data_provider);
    


    vasild commented at 7:36 am on August 26, 2021:
    Previously the asmap assignment was inside ConsumeBool(), not anymore. To make it identical, this should be vector asmap; if (ConsumeBool()) { asmap = ConsumeAsmap(); }. I think it is fine now too, because in some cases ConsumeAsmap() will return an empty vector.

    jnewbery commented at 9:28 am on August 26, 2021:
    Right, I think ConsumeAsmap() will actually return an empty vector in most cases, since SanityCheckASMap() will fail for most random bool vectors.
  21. in src/addrman.h:663 in 8a4c815dfe outdated
    658+    // especially useful when a large fraction of nodes
    659+    // operate under a couple of cloud providers.
    660+    //
    661+    // If a new asmap was provided, the existing records
    662+    // would be re-bucketed accordingly.
    663+    const std::vector<bool> m_asmap;
    


    vasild commented at 7:41 am on August 26, 2021:

    Commit 8a4c815dfef78eca6e481233195a5cfb171f3687 [fuzz] Add ConsumeAsmap() function contains this change:

    0-    const std::vector<bool>& GetAsmap() { return m_asmap; }
    1+    const std::vector<bool>& GetAsmap() const { return m_asmap; }
    

    which does not belong to it. Instead, that change should be squashed into 314e23e964ab7b333a2aec2cacb7c98b22daa8c2 [addrman] Make m_asmap private which introduced the GetAsmap() method.


    jnewbery commented at 9:28 am on August 26, 2021:
    thanks. Fixed
  22. jnewbery force-pushed on Aug 26, 2021
  23. in src/addrman.h:458 in d4fda4f9b9 outdated
    454@@ -471,7 +455,7 @@ class CAddrMan
    455         Check();
    456     }
    457 
    458-    explicit CAddrMan(bool deterministic, int32_t consistency_check_ratio);
    459+    explicit CAddrMan(std::vector<bool>* asmap, bool deterministic, int32_t consistency_check_ratio);
    


    laanwj commented at 8:32 am on August 27, 2021:
    I’d prefer to use std::optional<std::vector<bool>> instead of a pointer here, as it more clearly communicates what the parameter is (as well as the copying semantics).

    jnewbery commented at 8:46 am on August 27, 2021:
    If we were going to change this, I think it’d be even easier just to pass a std::vector<bool>, and pass the empty vector if there was no asmap (which is what m_asmap gets set to in that case). What do you think?

    vasild commented at 9:35 am on August 27, 2021:
    Given that the class member is of type std::vector<bool> (not a pointer that can be NULL, nor an optional that can be without a value), maybe just passing std::vector<bool> would be ok. The checks whether asmap “is present” are m_asmap.size() != 0.

    jnewbery commented at 10:03 am on August 27, 2021:
    ok, done!

    laanwj commented at 10:04 am on August 27, 2021:
    Sure, that would be more obvious, I hesitated to suggest it though because treating an empty vector as “special” value for absence is a CAddrMan implementation detail that might change later. But I’m ok with it, it’s better than the pointer.
  24. vasild approved
  25. vasild commented at 9:38 am on August 27, 2021: member
    ACK d4fda4f9b9c9e0cebf16381f6f49b39a43ef9ef4
  26. [addrman] Set m_asmap in CAddrMan initializer list
    This allows us to make it const.
    f572f2b204
  27. [net] Rename the copyStats arg from m_asmap to asmap
    The m_ prefix indicates that a variable is a data member. Using it as
    a parameter name is misleading.
    
    Also update the name of the function from copyStats to CopyStats to
    comply with our style guide.
    f9002cb5db
  28. [addrman] Make m_asmap private
    Add a GetAsmap() getter function that returns a reference to const.
    5840476714
  29. [fuzz] Add ConsumeAsmap() function 724c497562
  30. jnewbery force-pushed on Aug 27, 2021
  31. in src/addrman.cpp:84 in 724c497562
    76@@ -77,10 +77,11 @@ double CAddrInfo::GetChance(int64_t nNow) const
    77     return fChance;
    78 }
    79 
    80-CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio)
    81+CAddrMan::CAddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
    82     : insecure_rand{deterministic}
    83     , nKey{deterministic ? uint256{1} : insecure_rand.rand256()}
    84     , m_consistency_check_ratio{consistency_check_ratio}
    85+    , m_asmap{std::move(asmap)}
    


    vasild commented at 2:09 pm on August 27, 2021:

    I think performance is not important here because addrman constructor is not called frequently. Just mentioning: without the std::move it would copy the vector two times - once from the caller to the temporary argument and once from the temporary argument to m_asmap. With the std::move the second copy is avoided (a move is done instead), but the first copy is still done.

    From performance point of view it would be best to have two constructors:

     0// If the caller does not need the asmap afterwards. One move operation. Example:
     1// std::vector<bool> asmap;
     2// ...
     3// CAddrMan addrman{std::move(asmap)};
     4// Now the asmap variable is "empty".
     5CAddrMan::CAddrMan(std::vector<bool>&& asmap) : m_asmap{std::move(asmap)} { ... }
     6
     7// If the caller needs the asmap afterwards. One copy operation. Example:
     8// std::vector<bool> asmap;
     9// ...
    10// CAddrMan addrman{asmap};
    11// Now the asmap variable is untouched.
    12CAddrMan::CAddrMan(const std::vector<bool>& asmap) : m_asmap{asmap} { ... }
    

    jnewbery commented at 2:31 pm on August 27, 2021:

    Right, the reason I originally used a pointer as an argument was that the asmap might be needed afterwards, so we can’t just move from the argument. I wanted to avoid the two copies that you refer to above.

    As you point out, performance is not important since we’ll call this ctor at most twice (and almost always only once).

  32. vasild approved
  33. vasild commented at 2:10 pm on August 27, 2021: member
    ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
  34. mzumsande commented at 6:41 pm on August 28, 2021: member

    Tested ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7

    Code changes look correct to me. I verified that current master failed the addrman checks (-checkaddrman) when switching from no-asmap mode to asmap mode with an existing peers.dat, while I experienced no problems over multiple switches on this branch.

  35. jonatack commented at 7:30 pm on August 29, 2021: member

    Suggestions:

    * link to [erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263](https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263) (up to line 382) in the PR description for context
    
    * describe the symptoms/consequences of the issue and how to reproduce
    
    * maybe save refactoring/renaming for a non-bug-fix
    
    * add a passing/failing test that demonstrates the bug is fixed
    

    Addressed these suggestions in #22831 that pulls in the first commit here. That patch could be merged first and this one rebased on it (or vice-versa), or you could optionally pull in the test commits and use the PR description–as you prefer.

  36. in src/addrman.cpp:81 in f572f2b204 outdated
    76@@ -77,8 +77,9 @@ double CAddrInfo::GetChance(int64_t nNow) const
    77     return fChance;
    78 }
    79 
    80-CAddrMan::CAddrMan(bool deterministic, int32_t consistency_check_ratio)
    81-    : insecure_rand{deterministic}
    82+CAddrMan::CAddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
    83+    : m_asmap{std::move(asmap)}
    


    amitiuttarwar commented at 8:30 pm on August 31, 2021:

    question about use of std::move here -

    If reading peers.dat causes issues, init.cpp will instantiate a CAddrMan twice, using the same asmap both times. Is this ok? I know that std::move is more of a suggestion than a command, so I’m wondering how this code would be interpreted.


    vasild commented at 3:25 pm on September 1, 2021:

    The variable from init.cpp will be copied to the CAddrMan constructor into a temporary, used for the argument (this temporary is what is available inside the body of the constructor and will be automatically destroyed when the constructor returns). Then the constructor moves that temporary into the m_asmap member. So, the caller’s variable from init.cpp will not be modified and can be reused.

    See also #22791 (review)


    amitiuttarwar commented at 1:21 am on September 3, 2021:

    hm interesting, I’ve made some little toy programs to try to understand this behavior, and I see that the caller’s variable can be reused, so that resolves my concern.

    I’m not seeing the behavior of copying the temporary, but I’m going to resolve this conversation and explain further on the comment you linked.

  37. in src/addrman.cpp:84 in 5840476714 outdated
    77@@ -78,10 +78,10 @@ double CAddrInfo::GetChance(int64_t nNow) const
    78 }
    79 
    80 CAddrMan::CAddrMan(std::vector<bool> asmap, bool deterministic, int32_t consistency_check_ratio)
    81-    : m_asmap{std::move(asmap)}
    82-    , insecure_rand{deterministic}
    83+    : insecure_rand{deterministic}
    84     , nKey{deterministic ? uint256{1} : insecure_rand.rand256()}
    85     , m_consistency_check_ratio{consistency_check_ratio}
    86+    , m_asmap{std::move(asmap)}
    


    amitiuttarwar commented at 8:36 pm on August 31, 2021:

    commit: 5840476714ffebb2599999c85a23b52ebcff6090 [addrman] Make m_asmap private

    Is making the ordering of private members last in the initializer list a personal preference? I find it slightly odd that the initializer list no longer matches the order of the ctor args. Also you could have just introduced them in this order? =P


    vasild commented at 3:29 pm on September 1, 2021:
    The order here must be the same as the order in which the variables are declared in the class, otherwise we get a compiler warning.

    amitiuttarwar commented at 3:42 pm on September 1, 2021:
    ah right, thanks

    MarcoFalke commented at 10:38 am on September 6, 2021:
    The order could have been kept the same if it was moved to before insecure_rand in the header file
  38. amitiuttarwar commented at 8:46 pm on August 31, 2021: contributor
    approach ACK, I want to understand the std::move implications & then I’m ready to leave a review ACK. did you consider making m_asmap an optional arg for the addrman constructor? but overall, I like these changes- in addition to fixing the bug, making asmap a private & read-only member of addrman seems like an incremental improvement to me.
  39. amitiuttarwar commented at 1:35 am on September 3, 2021: contributor
    code review but utACK 724c497562
  40. naumenkogs commented at 9:18 am on September 6, 2021: member
    utACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7
  41. in src/addrman.h:580 in 5840476714 outdated
    576@@ -593,6 +577,8 @@ class CAddrMan
    577         Check();
    578     }
    579 
    580+    const std::vector<bool>& GetAsmap() const { return m_asmap; }
    


    MarcoFalke commented at 10:39 am on September 6, 2021:
    please add LIFETIMEBOUND when returning a pointer to memory in this

    jnewbery commented at 2:49 pm on September 7, 2021:

    Good idea.

    I actually plan to fully encapsulate asmap in its own component in #22910, so we’re not passing out references to private data.

  42. MarcoFalke approved
  43. MarcoFalke commented at 10:41 am on September 6, 2021: member

    review ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7 👫

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3review ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7 👫
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUjbBgwAmjfKpl2pPRl+MlWki+w1+qRbTqdMIiBrEu6wt79pkznSY6fNvWXAXCiI
     8xyqHDMf8psDsZiL3+gPq7qW5cBBPTZb8HxYvJAIqo2J5Gz2aqzNrgB4vegom0NgH
     9/QTVsdsgIssn2YmMXE6LpWU2nYaMgPqJPKAu4D3GZ2IEDitSLYYNGcm4M9MDYDQ3
    10FP+QNf+7qTE29UshNXP86iSQ2ITuKN2eXuA+Zm6BpaFmG8lkIsfdjJFQVdDDbCWX
    117lB1M/v5mrIK9LylIv/X6iYJi2FMT4UdqICoudp4L+V1m7lElw3Z/ymMjaPuM0NV
    129o+sJUbL+/lTFH3yncplmwkEZDdv+M4q00csIw5wAYvgpTRw0aP6YJPL1VzUISsK
    13R7CStBUNuIlX42hnGwHqUhs4fxwzevfg1tRTJDkk3+Z/y/E3u+Hj+eEBfoDBnU5C
    14/eqAT4WXAhdVZdqUcxR841Aki67Agsb2bv+pv9sAskG5OTHyynccu3T4Eh4XwsWL
    15MS67bp/6
    16=zBMj
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 9a48951e3c61060be14eb894092e9ac4ae9406fa411fed5ca58fea1d1415078b -

  44. MarcoFalke merged this on Sep 6, 2021
  45. MarcoFalke closed this on Sep 6, 2021

  46. jnewbery deleted the branch on Sep 7, 2021
  47. sidhujag referenced this in commit d6847a12f7 on Sep 7, 2021
  48. jamesob commented at 4:46 pm on September 10, 2021: member

    Addressed these suggestions in #22831 that pulls in the first commit here. That patch could be merged first and this one rebased on it (or vice-versa), or you could optionally pull in the test commits and use the PR description–as you prefer.

    Why was this comment, made 12 days ago and 8 days before merge, ignored?

    It would be one thing if this change was a straightforward revert or a simple reordering, but at face value it is not obvious that this change fixes the fairly severe bug that @jonatack discovered. It is troubling that a patch of this complexity was merged without an accompanying test verifying its efficacy, especially when one was made readily available.

  49. jonatack commented at 6:11 pm on September 10, 2021: member

    Concept ACK 4ffc8b8 modulo comments below and feedback from fuzzing people wrt invalidating affected corpii

    Suggestions:

    * link to [erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263](https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263) (up to line 382) in the PR description for context
    
    * describe the symptoms/consequences of the issue and how to reproduce
    
    * maybe save refactoring/renaming for a non-bug-fix
    
    * add a passing/failing test that demonstrates the bug is fixed
    

    @jamesob I didn’t understand either. The suggestions in #22791#pullrequestreview-737430387 were also essentially ignored.

  50. MarcoFalke commented at 6:59 am on September 11, 2021: member
    There were 5 ACKs, including one tested ACK, before I added the 6th ACK. I think this number of review ACKs (in addition to the tests to verify the fix by people who didn’t leave a comment) is sufficient to merge a pull request. This is overall a higher than average review interest compared to other pull reqeusts. If the alternative pull doesn’t get any ACKs, it would appear slightly odd to me to merge it instead.
  51. jnewbery commented at 12:59 pm on September 13, 2021: member

    Why was this comment, made 12 days ago and 8 days before merge, ignored?

    It would be one thing if this change was a straightforward revert or a simple reordering, but at face value it is not obvious that this change fixes the fairly severe bug that @jonatack discovered. It is troubling that a patch of this complexity was merged without an accompanying test verifying its efficacy, especially when one was made readily available. @jamesob (https://github.com/bitcoin/bitcoin/pull/22791#issuecomment-917050278)

    The suggestions in #22791 (review) were also essentially ignored. @jonatack (https://github.com/bitcoin/bitcoin/pull/22791#issuecomment-917108864)

    Please see #22831 (comment).

  52. fanquake referenced this in commit 505ba39665 on Apr 22, 2022
  53. Fabcien referenced this in commit 98906036ef on Oct 18, 2022
  54. Fabcien referenced this in commit c778701279 on Oct 18, 2022
  55. Fabcien referenced this in commit b525c4ac80 on Oct 18, 2022
  56. Fabcien referenced this in commit 9a1305b071 on Oct 18, 2022
  57. DrahtBot locked this on Oct 30, 2022

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: 2024-12-26 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me