refactor: Mark CAddrMan::Select and GetAddr const

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.

DrahtBot added the label Refactoring on May 13, 2021

DrahtBot commented at 3:24 pm on May 13, 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:

#22500 by MarcoFalke
#22498 by MarcoFalke
#21129 by vasild
#20233 by jnewbery

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.

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.

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:

(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."

theStack approved

theStack commented at 1:47 pm on May 16, 2021: member

Code-review ACK fa8e3f78751d80fa8bce957387bfc95802af2477

DrahtBot added the label Needs rebase on May 20, 2021

MarcoFalke force-pushed on May 22, 2021

MarcoFalke commented at 7:39 am on May 22, 2021: member

Rebased

DrahtBot removed the label Needs rebase on May 22, 2021

theStack approved

theStack commented at 3:03 pm on May 24, 2021: member

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

DrahtBot added the label Needs rebase on May 27, 2021

MarcoFalke force-pushed on May 31, 2021

DrahtBot removed the label Needs rebase on May 31, 2021

theStack approved

theStack commented at 6:40 pm on May 31, 2021: member

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

MarcoFalke referenced this in commit d75a1df617 on Jun 13, 2021

MarcoFalke commented at 7:25 am on June 14, 2021: member

Rebased and added one commit

MarcoFalke force-pushed on Jun 14, 2021

DrahtBot added the label Needs rebase on Jun 14, 2021

MarcoFalke force-pushed on Jun 14, 2021

DrahtBot removed the label Needs rebase on Jun 14, 2021

MarcoFalke force-pushed on Jul 19, 2021

lsilva01 approved

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.

theStack approved

theStack commented at 1:20 am on July 20, 2021: member

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)

fanquake requested review from jnewbery on Jul 20, 2021

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

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?

refactor: Mark CAddrMan::Select const fa02934c8c

refactor: Mark CAddrMan::GetAddr const fae0c79351

fuzz: Actually use const addrman fab755b77f

MarcoFalke force-pushed on Jul 21, 2021

MarcoFalke commented at 2:03 pm on July 21, 2021: member

Force pushed to add comment

MarcoFalke renamed this:
~~refactor: Mark CAddrMan::Select const~~
refactor: Mark CAddrMan::Select and GetAddr const
on Jul 21, 2021

MarcoFalke commented at 2:05 pm on July 21, 2021: member

Can GetAddr() also be made const?

It already is. (Adjusted title)

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?

MarcoFalke force-pushed on Jul 22, 2021

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

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).

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

Add missing GUARDED_BY to CAddrMan::insecure_rand fa32024d51

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

MarcoFalke force-pushed on Jul 23, 2021

jnewbery commented at 10:05 am on July 23, 2021: member

Code review ACK fae108ceb53f61d7338ba205873623ede3c1d3be

MarcoFalke commented at 11:06 am on July 29, 2021: member

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

theStack approved

theStack commented at 12:56 pm on July 29, 2021: member

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

MarcoFalke merged this on Aug 2, 2021

MarcoFalke closed this on Aug 2, 2021

MarcoFalke deleted the branch on Aug 2, 2021

jonatack commented at 11:07 am on August 2, 2021: member

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

0addrman.cpp:467:15: error: out-of-line definition of 'Check_' does not match any declaration in 'CAddrMan'
1int CAddrMan::Check_()
2              ^~~~~~
3./addrman.h:746:9: note: member declaration does not match because it is const qualified
4    int Check_() const EXCLUSIVE_LOCKS_REQUIRED(cs);
5        ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
61 error generated.

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.

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.

jonatack commented at 11:51 am on August 2, 2021: member

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

Thanks!

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.

jonatack commented at 12:34 pm on August 2, 2021: member

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

theStack commented at 4:10 pm on August 2, 2021: member

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? 🤔

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.

sidhujag referenced this in commit a98ae25583 on Aug 4, 2021

DrahtBot locked this on Aug 18, 2022

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

Conflicts