refactor: Mark CAddrMan::Select and GetAddr const #21940

pull MarcoFalke wants to merge 5 commits into bitcoin:master from MarcoFalke:2105-addrmanConstSelect changing 5 files +46 −37
  1. MarcoFalke commented at 9:26 AM on May 13, 2021: member

    To clarify that a call to this only changes the random state and nothing else.

  2. MarcoFalke cross-referenced this on May 13, 2021 from issue fuzz: Call const member functions in addrman fuzz test only once by MarcoFalke
  3. DrahtBot added the label Refactoring on May 13, 2021
  4. DrahtBot commented at 3:24 PM on May 13, 2021: contributor

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

    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. DrahtBot cross-referenced this on May 13, 2021 from issue p2p, rpc: enable GetAddr, GetAddresses, and getnodeaddresses by network by jonatack
  6. DrahtBot cross-referenced this on May 13, 2021 from issue addrman: Make consistency checks a runtime option by jnewbery
  7. promag commented at 8:59 AM on May 14, 2021: member

    ACK fa8e3f78751d80fa8bce957387bfc95802af2477.

    Built locally, also tried to avoid mutable members, but all things considered, this approach seems preferable.

  8. in src/addrman.cpp:456 in fa8e3f7875 outdated 
     397 | @@ -394,8 +398,9 @@ CAddrInfo CAddrMan::Select_(bool newOnly)
 398 |                  nUBucketPos = (nUBucketPos + insecure_rand.randbits(ADDRMAN_BUCKET_SIZE_LOG2)) % ADDRMAN_BUCKET_SIZE;
 399 |              }
 400 |              int nId = vvNew[nUBucket][nUBucketPos];
 401 | -            assert(mapInfo.count(nId) == 1);
 402 | -            CAddrInfo& info = mapInfo[nId];
 403 | +            const auto it_found{mapInfo.find(nId)};
    jonatack commented at 10:28 AM on May 14, 2021:

    Here and line 383 above and 506 below, apparently using auto with {} can lead to surprising results and it's recommended to use = rather than {} for objects specified with auto when we don't mean a list (at least, for C++11 per my Stroustrup book, not sure if it changed since in 14 or 17, on first skim of https://en.cppreference.com/w/cpp/language/auto I didn't see a change).

    MarcoFalke commented at 10:42 AM on May 14, 2021:

    Mind to share an example to illustrate the "surprise"?

    jonatack commented at 10:49 AM on May 14, 2021:

    image

    image

    (afk, sent from phone)

    MarcoFalke commented at 11:02 AM on May 14, 2021:

    See https://en.cppreference.com/w/cpp/language/template_argument_deduction#auto_type_deduction

    Also, if this was a list, the code wouldn't compile.

    jonatack commented at 11:07 AM on May 14, 2021:

    Thanks for the link. Seems it changed from 2014: https://isocpp.org/blog/2014/03/n3922, IIUC.

    jonatack commented at 11:09 AM on May 14, 2021:

    "For direct list-initialization:

    For a braced-init-list with only a single element, auto deduction will deduce from that entry;
For a braced-init-list with more than one element, auto deduction will be ill-formed."
  9. theStack approved
  10. theStack commented at 1:47 PM on May 16, 2021: contributor

    Code-review ACK fa8e3f78751d80fa8bce957387bfc95802af2477

  11. DrahtBot added the label Needs rebase on May 20, 2021
  12. MarcoFalke force-pushed on May 22, 2021
  13. MarcoFalke commented at 7:39 AM on May 22, 2021: member

    Rebased

  14. DrahtBot removed the label Needs rebase on May 22, 2021
  15. DrahtBot cross-referenced this on May 23, 2021 from issue refactor: Group and re-order CAddrMan members by access type by hebasto
  16. DrahtBot cross-referenced this on May 24, 2021 from issue refactor: Make CAddrMan::cs non-recursive by hebasto
  17. theStack approved
  18. theStack commented at 3:03 PM on May 24, 2021: contributor

    re-ACK fa845812d94ec6adc13459dca708b0850a3059ca 🦉 Checked that changes since my last ACK are only rebase-related via git range-diff fa8e3f78...fa845812

  19. DrahtBot added the label Needs rebase on May 27, 2021
  20. MarcoFalke force-pushed on May 31, 2021
  21. DrahtBot removed the label Needs rebase on May 31, 2021
  22. theStack approved
  23. theStack commented at 6:40 PM on May 31, 2021: contributor

    re-re-ACK fa49ab3e4075f8402e30507091033a621a6941ac ⚓

    Checked that changes since my last re-ACK are only rebase-related (caused by 5cd7f8abe3996d303774b6cddeb419337d605d02 and 8caf60dbbe16afa3c52574a7f6710d74c0bfd4ab) via git range-diff fa845812...fa49ab3

  24. MarcoFalke referenced this in commit d75a1df617 on Jun 13, 2021
  25. MarcoFalke commented at 7:25 AM on June 14, 2021: member

    Rebased and added one commit

  26. MarcoFalke force-pushed on Jun 14, 2021
  27. DrahtBot added the label Needs rebase on Jun 14, 2021
  28. MarcoFalke force-pushed on Jun 14, 2021
  29. DrahtBot removed the label Needs rebase on Jun 14, 2021
  30. DrahtBot cross-referenced this on Jul 19, 2021 from issue Assert in CAddrMan::Check() by MarcoFalke
  31. MarcoFalke force-pushed on Jul 19, 2021
  32. lsilva01 approved
  33. lsilva01 commented at 10:26 PM on July 19, 2021: contributor

    Code Review ACK and Tested ACK https://github.com/bitcoin/bitcoin/pull/21940/commits/fa47bdddbc7fc153c36f8f700099656ec1ce9d5d on Ubuntu 20.04.

  34. theStack approved
  35. theStack commented at 1:20 AM on July 20, 2021: contributor

    re-ACK fa47bdddbc7fc153c36f8f700099656ec1ce9d5d

    Checked again that changes since my last re-ACK are only rebase-related via git range-diff fa49ab3...fa47bddd (plus the fuzzing related extra commit)

  36. fanquake requested review from jnewbery on Jul 20, 2021
  37. in src/addrman.h:671 in fa47bdddbc outdated 
     670 | @@ -671,7 +671,7 @@ class CAddrMan
 671 |      std::unordered_map<CNetAddr, int, CNetAddrHash> mapAddr GUARDED_BY(cs);
 672 |  
 673 |      //! randomly-ordered vector of all nIds
    jnewbery commented at 10:44 AM on July 20, 2021:

    Perhaps comment why this can be mutable. vRandom is unobservable outside the addrman class, so any changes to it are also unobservable (and hence it can be mutated in const methods).

    MarcoFalke commented at 2:03 PM on July 21, 2021:

    Thanks, done

  38. jnewbery commented at 10:54 AM on July 20, 2021: member

    Concept ACK. vRandom and nRandomPos should be unobservable outside addrman. That means we can mutate them under const methods, and client code should be unaware of those changes.

    Can GetAddr() also be made const?

  39. refactor: Mark CAddrMan::Select const fa02934c8c
  40. refactor: Mark CAddrMan::GetAddr const fae0c79351
  41. fuzz: Actually use const addrman fab755b77f
  42. MarcoFalke force-pushed on Jul 21, 2021
  43. MarcoFalke commented at 2:03 PM on July 21, 2021: member

    Force pushed to add comment

  44. MarcoFalke renamed this:
    refactor: Mark CAddrMan::Select const
    refactor: Mark CAddrMan::Select and GetAddr const
    on Jul 21, 2021
  45. MarcoFalke commented at 2:05 PM on July 21, 2021: member

    Can GetAddr() also be made const?

    It already is. (Adjusted title)

  46. sipa commented at 6:08 PM on July 22, 2021: member

    Concept ACK. Perhaps this deserves a comment in addrman.h though, to state that even const member functions cannot be used without synchronization?

  47. MarcoFalke force-pushed on Jul 22, 2021
  48. MarcoFalke commented at 6:41 PM on July 22, 2021: member

    Added a lock annotation to the mutable member, which can be "read" by compilers and developers

  49. DrahtBot cross-referenced this on Jul 22, 2021 from issue Check that CAddrMan::nKey is not null after deserialize by MarcoFalke
  50. in src/addrman.cpp:159 in 5555a1dcd6 outdated 
     158 | +    assert(it_2 != mapInfo.end());
 159 |  
 160 | -    mapInfo[nId1].nRandomPos = nRndPos2;
 161 | -    mapInfo[nId2].nRandomPos = nRndPos1;
 162 | +    it_1->second.nRandomPos = nRndPos2;
 163 | +    it_2->second.nRandomPos = nRndPos1;
    jnewbery commented at 8:53 AM on July 23, 2021:

    An alternative to changing all of these [] operator calls to find() is to use the at() method, which is const. See https://en.cppreference.com/w/cpp/container/map/operator_at:

    operator[] is non-const because it inserts the key if it doesn't exist. If this behavior is undesirable or if the container is const, at() may be used.

    MarcoFalke commented at 9:05 AM on July 23, 2021:

    I'll leave that behaviour change (Replacing assert with throw) for a follow-up

    jnewbery commented at 9:10 AM on July 23, 2021:

    It's not a behavior change - the code is already asserting that count(nId1) and count(nId2) are non-zero, done under the cs lock so the map can't change from under us.

    MarcoFalke commented at 9:15 AM on July 23, 2021:

    Obviously it doesn't matter here, but one lookup might be faster than two lookups, which is why I changed to find.

    jnewbery commented at 9:29 AM on July 23, 2021:

    Sure. Just a suggestion to minimize the diff (+2/-2 instead of +6/-4 here and similar in other places).

  51. in src/addrman.h:636 in 5555a1dcd6 outdated 
     630 | @@ -631,9 +631,8 @@ class CAddrMan
 631 |      uint256 nKey;
 632 |  
 633 |      //! Source of random numbers for randomization in inner loops
 634 | -    FastRandomContext insecure_rand;
 635 | +    mutable FastRandomContext insecure_rand GUARDED_BY(cs);
 636 |  
 637 | -private:
    jnewbery commented at 9:01 AM on July 23, 2021:

    I don't really like changing all of addrman's private members to be protected. Since this is just to allow the tests to modify nKey and insecure_rand, what do you think about instead adding a bool deterministic argument to CAddrMan's ctor (similar to the deterministic argument in TxRequestTracker::TxRequestTracker).

    MarcoFalke commented at 9:08 AM on July 23, 2021:

    Is there any risk allowing the unit tests access? Happy to add private back (after the Mutex) if you think it helps.

    jnewbery commented at 9:11 AM on July 23, 2021:

    Yeah, I'd prefer for private to be added after the mutex to minimize the change.

    MarcoFalke commented at 9:36 AM on July 23, 2021:

    Force pushed to add private

  52. Add missing GUARDED_BY to CAddrMan::insecure_rand fa32024d51
  53. Fix incorrect whitespace in addrman
    Leaving it as-is would be annoying because some editor fix-up the
spacing when opening a file or editing it.
    fae108ceb5
  54. MarcoFalke force-pushed on Jul 23, 2021
  55. jnewbery commented at 10:05 AM on July 23, 2021: member

    Code review ACK fae108ceb53f61d7338ba205873623ede3c1d3be

  56. DrahtBot cross-referenced this on Jul 27, 2021 from issue fuzz: check that ser+unser produces the same AddrMan by vasild
  57. MarcoFalke commented at 11:06 AM on July 29, 2021: member

    @promag or @theStack interested in donating a re-ACK?

  58. theStack approved
  59. theStack commented at 12:56 PM on July 29, 2021: contributor

    re-ACK fae108ceb53f61d7338ba205873623ede3c1d3be 🍦

    Checked via git range-diff fa47bddd...fae108ce that there were only minor changes (comment on vRandom, as suggested by jnewbery) and rebase on master since my last ACK

  60. MarcoFalke merged this on Aug 2, 2021
  61. MarcoFalke closed this on Aug 2, 2021
  62. MarcoFalke deleted the branch on Aug 2, 2021
  63. jonatack commented at 11:07 AM on August 2, 2021: contributor

    Did any of the reviewers test this change with DEBUG_ADDRMAN defined? It blows up for me.

    addrman.cpp:467:15: error: out-of-line definition of 'Check_' does not match any declaration in 'CAddrMan'
int CAddrMan::Check_()
              ^~~~~~
./addrman.h:746:9: note: member declaration does not match because it is const qualified
    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
        ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
  64. MarcoFalke commented at 11:11 AM on August 2, 2021: member

    @jonatack Good catch. I didn't test this with the CPP flag set and CI doesn't either.

    Luckily the code will be always compiled after #20233, so shouldn't happen again after that pull is merged.

  65. jnewbery commented at 11:44 AM on August 2, 2021: member

    Thanks @jonatack. I'm currently rebasing #20233 and will include a fix for this issue in that PR.

  66. jonatack commented at 11:51 AM on August 2, 2021: contributor

    Thanks @jonatack. I'm currently rebasing #20233 and will include a fix for this issue in that PR.

    Thanks!

  67. MarcoFalke commented at 12:29 PM on August 2, 2021: member

    I created a fix in #22601 , because I didn't see the comment here.

  68. jonatack commented at 12:34 PM on August 2, 2021: contributor

    Thanks, in the meantime will cherry-pick this commit locally.

  69. theStack commented at 4:10 PM on August 2, 2021: contributor

    Did any of the reviewers test this change with DEBUG_ADDRMAN defined? It blows up for me.

    Oops, no, I didn't :( thanks for catching this.

    Would it make sense to have a CI instance with DEBUG_ADDRMAN defined or is that overambitious? 🤔

  70. jnewbery commented at 4:49 PM on August 2, 2021: member

    @theStack please see #20233 which removes the DEBUG_ADDRMAN build option and makes consistency checks a runtime option.

  71. sidhujag referenced this in commit a98ae25583 on Aug 4, 2021
  72. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkeDrahtBotpromagjonatacktheStacklsilva01jnewberysipa
Labels
Refactoring
Linked (view graph)
#21941 fuzz: Call const member functions in addrman fuzz test only once#22601 Add missing const to CAddrMan::Check_()
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-05-20 06:54 UTC