addrman: Remove CAddrMan::Clear() function #22697

jnewbery · 2021-08-13T11:38:00Z

CAddrMan::Clear() exists to reset the internal state of CAddrMan. It's currently used in two places:

on startup, if deserializing peers.dat fails, Clear() is called to reset to an empty addrman
in tests, Clear() is called to reset the addrman for more tests

In both cases, we can simply destruct the CAddrMan and construct a new, empty addrman. That approach is safer - it's possible that Clear() could 'reset' the addrman to a state that's not equivalent to a freshly constructed addrman (one actual example of this is that Clear() does not clear the m_tried_collisions set). On the other hand, if we destruct and then construct a fresh addrman, we're guaranteed that the new object is empty.

This wasn't possible when addrman was initially implemented, since it was a global, and so it would only be destructed on shutdown. However, addrman is now owned by node.context, so we have control over its destruction/construction.

Zero-1729 · 2021-08-13T12:34:39Z

Concept ACK

Thanks for splitting up the changes!

DrahtBot · 2021-08-13T14:15:57Z

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

Conflicts

No conflicts as of last run.

jonatack

ACK 70147fe

At each commit reviewed, debug-built, launched a signet node, ran the following quick checks

./src/test/test_bitcoin -t addrman_tests
./src/test/test_bitcoin -t net_tests
./src/bench/bench_bitcoin -filter=AddrManAdd

The CAddrMan ctor cleanup is nice.

Edit: I did not test behavior with a corrupt peers.dat on startup; a functional test for this may be good.

src/bench/addrman.cpp

theStack · 2021-08-13T14:19:35Z

Concept ACK

amitiuttarwar · 2021-08-14T00:54:45Z

concept ACK

practicalswift · 2021-08-15T18:33:07Z

Concept ACK

jnewbery · 2021-08-17T08:48:48Z

rebased on master

fanquake

Concept ACK. Change look fairly straight forward.

src/net.cpp

fanquake · 2021-08-18T05:58:24Z

src/addrman.h


    //! list of "new" buckets
    int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);

-    //! last time Good was called (memory only)
-    int64_t nLastGood GUARDED_BY(cs);
+    //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.


What is "never"?

'never' means that Good() has not been called since the CAddrMan object was constructed. I moved this comment from the constructor (where nLastGood was previously being set) to the member declaration (which now has a default initialization value).

jnewbery · 2021-08-18T08:30:57Z

Rebased on master and addressed review comments from @jonatack and @fanquake.

maflcko

left some comments on the test commit

maflcko · 2021-08-18T08:41:17Z

src/test/addrman_tests.cpp

-    BOOST_CHECK(addrman.Add(vAddr, source));
-    BOOST_CHECK(addrman.size() >= 1);
+    BOOST_CHECK(addrman->Add(vAddr, source));
+    BOOST_CHECK(addrman->size() >= 1);


is there a reason why you prefer unique_ptr? unique_ptr will result in a different memory layout, will add an unneeded abstraction and will make the diff larger than needed.

Simply using a new object worked fine for me:

diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index 3c64461605..67c549689d 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -148,18 +148,13 @@ BOOST_AUTO_TEST_CASE(addrman_simple) BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source)); BOOST_CHECK(addrman.size() >= 1); - // Test: AddrMan::Clear() should empty the new table. - addrman.Clear(); - BOOST_CHECK_EQUAL(addrman.size(), 0U); - CAddrInfo addr_null2 = addrman.Select(); - BOOST_CHECK_EQUAL(addr_null2.ToString(), "[::]:0"); - // Test: AddrMan::Add multiple addresses works as expected + CAddrManTest addrman2{}; std::vector<CAddress> vAddr; vAddr.push_back(CAddress(ResolveService("250.1.1.3", 8333), NODE_NONE)); vAddr.push_back(CAddress(ResolveService("250.1.1.4", 8333), NODE_NONE)); - BOOST_CHECK(addrman.Add(vAddr, source)); - BOOST_CHECK(addrman.size() >= 1); + BOOST_CHECK(addrman2.Add(vAddr, source)); + BOOST_CHECK(addrman2.size() >= 1); } BOOST_AUTO_TEST_CASE(addrman_ports)

I've reverted this (and the similar change to the addrman_serialization test below). Constructing many large CAddrMan objects on the stack causes a stack overflow on some systems: #22697 (comment).

src/bench/addrman.cpp

maflcko · 2021-08-18T08:46:41Z

src/test/addrman_tests.cpp

-    stream >> addrman_asmap1;
-    std::pair<int, int> bucketAndEntry_asmap1_deser = addrman_asmap1.GetBucketAndEntry(addr);
+    addrman_asmap1 = std::make_unique<CAddrManTest>(true, asmap1);
+    addrman_noasmap = std::make_unique<CAddrManTest>();


Same here? Could use new object with different name or even split the test into two and keep the same name?

Reverted (see #22697 (comment))

src/test/fuzz/addrman.cpp

src/init.cpp

src/test/addrman_tests.cpp

jnewbery · 2021-08-18T09:57:48Z

Thanks for the review @MarcoFalke. I've addressed all your comments.

Now that we manage the lifespan of node.addrman, we can just reset node.addrman to a newly initialized CAddrMan if parsing peers.dat fails, eliminating the possibility that Clear() leaves some old state behind.

Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required. Also make CAddrMan::Clear() private to ensure that no call sites are missed.

Clear() is now only called from the ctor, so just inline the code into that function. The LOCK(cs) can be removed, since there can be no data races in the ctor. Also move the function definition out of the header and into the cpp file.

Use default initialization and initializer lists, and use range-based for loops for resetting the buckets.

jnewbery · 2021-08-19T10:31:38Z

Thank you for the review @laanwj @amitiuttarwar @vasild @MarcoFalke. I believe that I've addressed all of your review comments.

vasild

ACK 4d2fa97

Zero-1729

crACK 4d2fa97

laanwj · 2021-08-19T13:34:20Z

Code review ACK 4d2fa97

PR bitcoin#22697 introduced a severe reproducible bug in commit 181a120 that causes the addrman tried table to become corrupt and significantly lose peer entries when the `-asmap` configuration option is used. The corruption occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error.

PR bitcoin#22697 introduced a severe reproducible bug in commit 181a120 that causes the addrman tried table to become corrupt and significantly lose peer entries when the `-asmap` configuration option is used. The corruption occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error when run from root: `test/functional/feature_asmap.py`.

PR bitcoin#22697 introduced a severe reproducible bug in commit 181a120 that causes the addrman tried table to become corrupt and significantly lose peer entries when the `-asmap` configuration option is used. The corruption occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error.

PR bitcoin#22697 introduced a reproducible bug in commit 181a120 that causes the addrman tried table to fail consistency checks and significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error. Co-authored-by: John Newbery <john@johnnewbery.com>

PR bitcoin#22697 introduced a reproducible issue in commit 181a120 that causes the addrman tried table to fail consistency checks and significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error. Co-authored-by: John Newbery <john@johnnewbery.com>

PR bitcoin#22697 introduced a reproducible issue in commit 181a120 that causes the addrman tried table to fail consistency checks and significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

…n restart with asmap cdaab90 Add test for addrman consistency check on restart with asmap (Jon Atack) 869f136 Add test for rpc addpeeraddress with "tried" argument (Jon Atack) ef242f5 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack) Pull request description: This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue. PR #22697 introduced a reproducible bug in commit 181a120 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled - restart bitcoind - bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()` How to test this pull: - `git checkout 181a120`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output: ``` AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed. ``` ACKs for top commit: jnewbery: utACK cdaab90 mzumsande: re-ACK cdaab90 (based on code review of diff to d586817) vasild: ACK cdaab90 Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399

PR bitcoin#22697 introduced a reproducible issue in commit 181a120 that causes the addrman tried table to fail consistency checks and significantly lose peer entries when the `-asmap` configuration option is used. The issue occurs on bitcoind restart due to an initialization order change in `src/init.cpp` in that commit whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before. Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263. ``` addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted ``` How to reproduce: - `git checkout 181a120` and recompile - launch bitcoind with `-asmap` and `-checkaddrman=1` config options - restart bitcoind - bitcoind aborts on second call to `CAddrMan::Check()` This commit adds a regression test to reproduce the case; it passes or fails with the same error. Co-authored-by: John Newbery <john@johnnewbery.com> Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

DrahtBot added the P2P label Aug 13, 2021

jonatack reviewed Aug 13, 2021

View reviewed changes

src/bench/addrman.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 13, 2021

p2p: fix ubsan addrman errors, make nTime truncation conversion explicit #22094

Closed

mahsanghani approved these changes Aug 13, 2021

View reviewed changes

This was referenced Aug 16, 2021

[addrman] De-duplicate Add() function #22627

Closed

Check that CAddrMan::nKey is not null after deserialize #22498

Closed

DrahtBot added the Needs rebase label Aug 17, 2021

jnewbery force-pushed the 2021-08-remove-addrman-clear branch from 70147fe to 74fb306 Compare August 17, 2021 08:48

DrahtBot removed the Needs rebase label Aug 17, 2021

jnewbery force-pushed the 2021-08-remove-addrman-clear branch from 64205a1 to 74fb306 Compare August 17, 2021 17:05

DrahtBot mentioned this pull request Aug 17, 2021

MOVEONLY: tests: Move addrman ser/deser tests to addrman_tests.cpp #22725

Merged

DrahtBot added the Needs rebase label Aug 18, 2021

fanquake reviewed Aug 18, 2021

View reviewed changes

jnewbery force-pushed the 2021-08-remove-addrman-clear branch from 74fb306 to c24f81d Compare August 18, 2021 08:31

maflcko reviewed Aug 18, 2021

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

maflcko reviewed Aug 18, 2021

View reviewed changes

src/test/addrman_tests.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2021-08-remove-addrman-clear branch from c24f81d to fdf05e7 Compare August 18, 2021 09:57

DrahtBot added Needs rebase and removed Needs rebase labels Aug 18, 2021

jnewbery added 2 commits August 18, 2021 14:00

[addrman] Move peers.dat parsing to init.cpp

181a120

[addrman] Don't call Clear() if parsing peers.dat fails

e8e7392

Now that we manage the lifespan of node.addrman, we can just reset node.addrman to a newly initialized CAddrMan if parsing peers.dat fails, eliminating the possibility that Clear() leaves some old state behind.

jnewbery added 3 commits August 19, 2021 11:26

[addrman] Remove all public uses of CAddrMan.Clear() from the tests

406be5f

Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required. Also make CAddrMan::Clear() private to ensure that no call sites are missed.

[addrman] inline Clear() into CAddrMan ctor

7e6e659

Clear() is now only called from the ctor, so just inline the code into that function. The LOCK(cs) can be removed, since there can be no data races in the ctor. Also move the function definition out of the header and into the cpp file.

[addrman] Clean up ctor

4d2fa97

Use default initialization and initializer lists, and use range-based for loops for resetting the buckets.

jnewbery force-pushed the 2021-08-remove-addrman-clear branch from fe9b1e5 to 4d2fa97 Compare August 19, 2021 10:33

vasild approved these changes Aug 19, 2021

View reviewed changes

Zero-1729 reviewed Aug 19, 2021

View reviewed changes

fanquake merged commit 602c8eb into bitcoin:master Aug 20, 2021

jnewbery deleted the 2021-08-remove-addrman-clear branch August 20, 2021 06:40

maflcko mentioned this pull request Aug 20, 2021

Move LoadAddrman from init to addrdb #22754

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 20, 2021

Merge bitcoin#22697: addrman: Remove CAddrMan::Clear() function

13bd988

jonatack mentioned this pull request Aug 29, 2021

test: add addpeeraddress "tried", test addrman checks on restart with asmap #22831

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 20, 2022

addrman: Remove CAddrMan::Clear() function #22697

addrman: Remove CAddrMan::Clear() function #22697

Uh oh!

Conversation

jnewbery commented Aug 13, 2021

Uh oh!

Zero-1729 commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Aug 13, 2021

Uh oh!

amitiuttarwar commented Aug 14, 2021

Uh oh!

practicalswift commented Aug 15, 2021

Uh oh!

jnewbery commented Aug 17, 2021

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Aug 18, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Aug 18, 2021

Uh oh!

jnewbery commented Aug 19, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Zero-1729 left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 19, 2021

Uh oh!

Uh oh!

Zero-1729 commented Aug 13, 2021 •

edited

Loading

DrahtBot commented Aug 13, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading