addrman: Remove CAddrMan::Clear() function

jnewbery commented at 11:38 am on August 13, 2021: member

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.

DrahtBot added the label P2P on Aug 13, 2021

Zero-1729 commented at 12:34 pm on August 13, 2021: contributor

Concept ACK

Thanks for splitting up the changes!

in src/bench/addrman.cpp:79 in 70147fe0e3 outdated

76-    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
77+    bench.epochs(5).epochIterations(1);
78+    size_t addrman_count = bench.epochs() * bench.epochIterations();
79 
80+    std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count);
81+    for (size_t i{0}; i < addrman_count; i++) {

jonatack commented at 1:42 pm on August 13, 2021:

a8bad2e16ed41c8d9 nit, prefer prefix iterator

0    for (size_t i{0}; i < addrman_count; ++i) {

jnewbery commented at 8:30 am on August 18, 2021:

Done

DrahtBot commented at 2:15 pm on August 13, 2021: member

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

Conflicts

No conflicts as of last run.

jonatack commented at 2:16 pm on August 13, 2021: member

ACK 70147fe0e3e65e28049e9c6171758615e4387119

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.

theStack commented at 2:19 pm on August 13, 2021: member

Concept ACK

mahsanghani approved

amitiuttarwar commented at 0:54 am on August 14, 2021: contributor

concept ACK

practicalswift commented at 6:33 pm on August 15, 2021: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 17, 2021

jnewbery force-pushed on Aug 17, 2021

jnewbery commented at 8:48 am on August 17, 2021: member

rebased on master

DrahtBot removed the label Needs rebase on Aug 17, 2021

jnewbery force-pushed on Aug 17, 2021

DrahtBot added the label Needs rebase on Aug 18, 2021

in src/net.cpp:2535 in 74fb306b58 outdated

2533@@ -2534,18 +2534,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2534     if (clientInterface) {
2535         clientInterface->InitMessage(_("Loading P2P addresses…").translated);

fanquake commented at 5:51 am on August 18, 2021:

Should this message move too?

jnewbery commented at 8:23 am on August 18, 2021:

Yes! Thanks. Moved.

in src/addrman.h:654 in 74fb306b58 outdated

653     //! list of "new" buckets
654     int vvNew[ADDRMAN_NEW_BUCKET_COUNT][ADDRMAN_BUCKET_SIZE] GUARDED_BY(cs);
655 
656-    //! last time Good was called (memory only)
657-    int64_t nLastGood GUARDED_BY(cs);
658+    //! last time Good was called (memory only). Initially set to 1 so that "never" is strictly worse.

fanquake commented at 5:58 am on August 18, 2021:

What is “never”?

jnewbery commented at 8:28 am on August 18, 2021:

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

fanquake commented at 6:00 am on August 18, 2021: member

Concept ACK. Change look fairly straight forward.

jnewbery commented at 8:30 am on August 18, 2021: member

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

jnewbery force-pushed on Aug 18, 2021

in src/test/addrman_tests.cpp:207 in ec132b1f86 outdated

213     vAddr.push_back(CAddress(ResolveService("250.1.1.3", 8333), NODE_NONE));
214     vAddr.push_back(CAddress(ResolveService("250.1.1.4", 8333), NODE_NONE));
215-    BOOST_CHECK(addrman.Add(vAddr, source));
216-    BOOST_CHECK(addrman.size() >= 1);
217+    BOOST_CHECK(addrman->Add(vAddr, source));
218+    BOOST_CHECK(addrman->size() >= 1);

MarcoFalke commented at 8:41 am on August 18, 2021:

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:

 0diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
 1index 3c64461605..67c549689d 100644
 2--- a/src/test/addrman_tests.cpp
 3+++ b/src/test/addrman_tests.cpp
 4@@ -148,18 +148,13 @@ BOOST_AUTO_TEST_CASE(addrman_simple)
 5     BOOST_CHECK(addrman.Add(CAddress(addr2, NODE_NONE), source));
 6     BOOST_CHECK(addrman.size() >= 1);
 7 
 8-    // Test: AddrMan::Clear() should empty the new table.
 9-    addrman.Clear();
10-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
11-    CAddrInfo addr_null2 = addrman.Select();
12-    BOOST_CHECK_EQUAL(addr_null2.ToString(), "[::]:0");
13-
14     // Test: AddrMan::Add multiple addresses works as expected
15+    CAddrManTest addrman2{};
16     std::vector<CAddress> vAddr;
17     vAddr.push_back(CAddress(ResolveService("250.1.1.3", 8333), NODE_NONE));
18     vAddr.push_back(CAddress(ResolveService("250.1.1.4", 8333), NODE_NONE));
19-    BOOST_CHECK(addrman.Add(vAddr, source));
20-    BOOST_CHECK(addrman.size() >= 1);
21+    BOOST_CHECK(addrman2.Add(vAddr, source));
22+    BOOST_CHECK(addrman2.size() >= 1);
23 }
24 
25 BOOST_AUTO_TEST_CASE(addrman_ports)

jnewbery commented at 9:57 am on August 18, 2021:

Done

jnewbery commented at 10:16 am on August 19, 2021:

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

in src/bench/addrman.cpp:78 in ec132b1f86 outdated

75 
76-    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
77+    bench.epochs(5).epochIterations(1);
78+    size_t addrman_count = bench.epochs() * bench.epochIterations();
79+
80+    std::vector<std::unique_ptr<CAddrMan>> addrmans(addrman_count);

MarcoFalke commented at 8:45 am on August 18, 2021:

not a huge difference here, but could use reserve (optional) and emplace to avoid unique_ptr?

jnewbery commented at 9:39 am on August 18, 2021:

I did try that, but couldn’t get it to work. emplace() requires the type to be MoveInsertable, which CAddrMan isn’t since its move constructor is implicitly deleted.

If you have a suggestion for how to do this, I’d be happy to incorporate it.

in src/test/addrman_tests.cpp:795 in ec132b1f86 outdated

807-    addrman_noasmap.Add({addr}, default_source);
808-    stream << addrman_noasmap;
809-    stream >> addrman_asmap1;
810-    std::pair<int, int> bucketAndEntry_asmap1_deser = addrman_asmap1.GetBucketAndEntry(addr);
811+    addrman_asmap1 = std::make_unique<CAddrManTest>(true, asmap1);
812+    addrman_noasmap = std::make_unique<CAddrManTest>();

MarcoFalke commented at 8:46 am on August 18, 2021:

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

jnewbery commented at 9:57 am on August 18, 2021:

Done

jnewbery commented at 10:17 am on August 19, 2021:

Reverted (see #22697 (review))

in src/test/fuzz/addrman.cpp:247 in ec132b1f86 outdated

246     while (fuzzed_data_provider.ConsumeBool()) {
247         CallOneOf(
248             fuzzed_data_provider,
249             [&] {
250-                addr_man.Clear();
251+                addr_man = std::make_unique<CAddrManDeterministic>(fuzzed_data_provider);

MarcoFalke commented at 8:48 am on August 18, 2021:

it might be better to just remove this, as it is equivalent to just starting a fresh fuzz input that fails deser

jnewbery commented at 9:51 am on August 18, 2021:

Removed

MarcoFalke commented at 8:49 am on August 18, 2021: member

left some comments on the test commit

in src/init.cpp:1175 in c4b12a6cbb outdated

1166@@ -1167,6 +1167,19 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1167     assert(!node.addrman);
1168     auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
1169     node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
1170+    {
1171+        // Load addresses from peers.dat
1172+        uiInterface.InitMessage(_("Loading P2P addresses…").translated);
1173+        int64_t nStart = GetTimeMillis();
1174+        CAddrDB adb;
1175+        if (adb.Read(*node.addrman))

MarcoFalke commented at 8:50 am on August 18, 2021:

this line isn’t move-only, so could add { while touching?

jnewbery commented at 9:57 am on August 18, 2021:

Done

in src/test/addrman_tests.cpp:51 in b0befadcc2 outdated

47@@ -48,7 +48,7 @@ class CAddrManCorrupted : public CAddrManSerializationMock
48         unsigned char nVersion = 1;
49         s << nVersion;
50         s << ((unsigned char)32);
51-        s << nKey;
52+        s << ((uint256)1);

MarcoFalke commented at 8:52 am on August 18, 2021:

I slightly prefer {} constructors (uint256{1}) or uint256::ONE.

jnewbery commented at 9:55 am on August 18, 2021:

Yes, I agree that’s better. Changed to uint256::ONE.

jnewbery commented at 9:57 am on August 18, 2021: member

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

jnewbery force-pushed on Aug 18, 2021

DrahtBot removed the label Needs rebase on Aug 18, 2021

DrahtBot added the label Needs rebase on Aug 18, 2021

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

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

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.

e8e7392311

[tests] Remove CAddrMan.Clear() call from CAddrDB::Read()

`bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)` is _only_
called from the tests, and the call to addr.Clear() only exists so that
a test that Clear() is called passes. Remove that test and the call.

ed9ba8af08

jnewbery commented at 1:04 pm on August 18, 2021: member

Rebased

jnewbery force-pushed on Aug 18, 2021

DrahtBot removed the label Needs rebase on Aug 18, 2021

in src/init.cpp:1170 in 3fe1429a33 outdated

1166@@ -1167,6 +1167,20 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1167     assert(!node.addrman);
1168     auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
1169     node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
1170+    {

laanwj commented at 2:46 pm on August 18, 2021:

What is the reasoning for moving this to init.cpp? I think this goes in the wrong direction, of having parts take care of their own initialization instead of init.cpp being a hodge-podge of initialization functionality.

jnewbery commented at 4:22 pm on August 18, 2021:

Since #20228, addrman is owned by the node.context struct and gets constructed inside the AppInitMain() function. That PR includes motivation for why that is.

Since AppInitMain() is responsible for constructing the addrman, it should also be responsible for making sure addrman deserializes from peers.dat (and recovers if there’s an error). Before this PR, addrman peers.dat derserialization is happening in CConnMan::Start(), which doesn’t make a lot of sense.

laanwj commented at 1:18 pm on August 19, 2021:

Ok, I agree. I hope it can be factored out again at some point to a place that makes sense.

MarcoFalke commented at 10:57 am on August 20, 2021:

See #22754

laanwj commented at 2:50 pm on August 18, 2021: member

Concept ACK on getting rid of the Clear() function, seems more robust this way.

amitiuttarwar commented at 8:02 pm on August 18, 2021: contributor

looks like both failing builds are failing in addrman_tests -> addrman_serialization

AppVeyor:

0C:\projects\bitcoin\src\test\addrman_tests.cpp(758): Entering test case "addrman_serialization"
1unknown location(0): fatal error: in "addrman_tests/addrman_serialization": stack overflow

Win64:

0test/addrman_tests.cpp(758): Entering test case "addrman_serialization"
1wine: Unhandled stack overflow at address 0000000001180216 (thread 0018), starting debugger...

in src/bench/addrman.cpp:75 in 3fe1429a33 outdated

69@@ -70,13 +70,22 @@ static void FillAddrMan(CAddrMan& addrman)
70 
71 static void AddrManAdd(benchmark::Bench& bench)
72 {
73-    CreateAddresses();
74+     /* Create many CAddrMan objects - one to be modified at each loop iteration. */
75 
76-    CAddrMan addrman(/* deterministic */ false, /* consistency_check_ratio */ 0);
77+    bench.epochs(5).epochIterations(1);

vasild commented at 7:42 am on August 19, 2021:

Hardcoding the number of runs could cripple the test (results vary too much). It is better to avoid that if possible, so that nanobench chooses how many runs to execute based on variance. In this case it is possible to avoid hardcoding, and also shorter:

0static void AddrManAdd(benchmark::Bench& bench)
1{
2    CreateAddresses();
3
4    bench.run([&] { 
5        auto addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ 0);
6        AddAddressesToAddrMan(*addrman);
7    }); 
8}

MarcoFalke commented at 8:25 am on August 19, 2021:

Could also remove the make_unique and use the beautiful curly brackets?

0static void AddrManAdd(benchmark::Bench& bench)
1{
2    CreateAddresses();
3
4    bench.run([&] { 
5        CAddrMan addrman{/* deterministic */ false, /* consistency_check_ratio */ 0};
6        AddAddressesToAddrMan(addrman);
7    }); 
8}

jnewbery commented at 10:30 am on August 19, 2021:

The reason to construct the addrman objects outside the bench run is to prevent the time spent in constructing the object being included in the bench. I think that would be especially bad and could lead to variance if we’re allocating memory for the object by making a unique_ptr.

I’ve reverted to Marco’s suggested version, which shouldn’t be any worse than the current code, which calls Clear().

in src/init.cpp:1181 in 3fe1429a33 outdated

1176+            LogPrintf("Loaded %i addresses from peers.dat  %dms\n", node.addrman->size(), GetTimeMillis() - nStart);
1177+        } else {
1178+            // Addrman can be in an inconsistent state after failure, reset it
1179+            node.addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
1180+            LogPrintf("Recreating peers.dat\n");
1181+            adb.Write(*node.addrman);

vasild commented at 7:50 am on August 19, 2021:

This would skip the message “Flushed 0 addresses to peers.dat” which was previously printed from CConnman::DumpAddresses(), is that intentional?

jnewbery commented at 10:30 am on August 19, 2021:

Yes, I don’t think that log is necessary or helpful.

in src/test/addrman_tests.cpp:202 in 3fe1429a33 outdated

203-    BOOST_CHECK_EQUAL(addrman.size(), 0U);
204-    CAddrInfo addr_null2 = addrman.Select();
205-    BOOST_CHECK_EQUAL(addr_null2.ToString(), "[::]:0");
206-
207     // Test: AddrMan::Add multiple addresses works as expected
208+    CAddrManTest addrman2{};

vasild commented at 7:52 am on August 19, 2021:

nit: I think {} is just clutter. Also, few objects below are constructed without it.

jnewbery commented at 10:28 am on August 19, 2021:

This has now been reverted so the CAddrManTest objects are constructed using std::make_unique().

in src/test/fuzz/addrman.cpp:240 in 3fe1429a33 outdated

238         try {
239-            ds >> addr_man;
240+            ds >> *addr_man_ptr;
241         } catch (const std::ios_base::failure&) {
242-            addr_man.Clear();
243+            addr_man_ptr = std::make_unique<CAddrManDeterministic>(fuzzed_data_provider);

vasild commented at 8:09 am on August 19, 2021:

Previously the Clear() method would not have consumed anything from fuzzed_data_provider. Now, the CAddrManDeterministic constructor will consume one uint256, one bool and maybe one RandomLengthBitVector. I guess this is ok (it will invalidate the corpus, but I guess that is ok too).

MarcoFalke commented at 8:21 am on August 19, 2021:

The seeds are already invalidated by removing an item in CallOneOf below ;)

vasild commented at 8:17 am on August 19, 2021: member

Approach ACK 3fe1429a33ef181760fd21df06a4030854418be9

jnewbery force-pushed on Aug 19, 2021

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

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.

406be5ff96

[addrman] inline Clear() into CAddrMan ctor

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.

7e6e65918f

[addrman] Clean up ctor

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

4d2fa97031

jnewbery commented at 10:31 am on August 19, 2021: member

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

jnewbery force-pushed on Aug 19, 2021

vasild approved

vasild commented at 11:19 am on August 19, 2021: member

ACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed

Zero-1729 commented at 1:09 pm on August 19, 2021: contributor

crACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed

laanwj commented at 1:34 pm on August 19, 2021: member

Code review ACK 4d2fa97031a6f31da984d4c2c105447ed692c6ed

fanquake merged this on Aug 20, 2021

fanquake closed this on Aug 20, 2021

jnewbery deleted the branch on Aug 20, 2021

sidhujag referenced this in commit 13bd988cc3 on Aug 20, 2021

jonatack referenced this in commit 9fc4938699 on Aug 30, 2021

jonatack referenced this in commit 30f59790b8 on Aug 30, 2021

jonatack referenced this in commit 80dbd9b0f4 on Aug 30, 2021

jonatack referenced this in commit 8e4695c40b on Sep 6, 2021

jonatack referenced this in commit 916d54ac45 on Sep 8, 2021

jonatack referenced this in commit 22a17be0b9 on Sep 10, 2021

jonatack referenced this in commit a04f24d98e on Sep 14, 2021

jonatack referenced this in commit 55d1411a16 on Sep 14, 2021

jonatack referenced this in commit 4de3271b1c on Sep 14, 2021

jonatack referenced this in commit 5593ea1a1d on Sep 14, 2021

jonatack referenced this in commit d58681743a on Sep 14, 2021

jonatack referenced this in commit 71bea5df87 on Sep 15, 2021

jonatack referenced this in commit cdaab90662 on Sep 15, 2021

jonatack referenced this in commit 588a6b4e7e on Sep 18, 2021

MarcoFalke referenced this in commit 223ad2fd0d on Sep 21, 2021

rebroad referenced this in commit 2323faa015 on Oct 13, 2021

DrahtBot locked this on Aug 20, 2022

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

Conflicts