init: Fix asmap/addrman initialization order bug

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.

fanquake added the label P2P on Aug 24, 2021

jnewbery renamed this:
~~[init] Read/decode asmap before constructing addrman~~
init: Fix asmap/addrman initialization order bug
on Aug 24, 2021

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             ^

jnewbery force-pushed on Aug 24, 2021

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.

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"

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.

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.

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:

I guess this is fine either way. I like the reasoning in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#c131-avoid-trivial-getters-and-setters

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

vasild commented at 4:20 pm on August 24, 2021: member

ACK 4ffc8b81bc520fe66fbea9a597ab889a2647ca2e

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.

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:

link to 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

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.

[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

[net] Remove CConnMan::SetAsmap()

CAddrMan::m_asmap is now set directly in AppInitMain() so
CConnMan::SetAsmap() is no longer required.

593247872d

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.

jnewbery force-pushed on Aug 25, 2021

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.

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

jnewbery force-pushed on Aug 26, 2021

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.

vasild approved

vasild commented at 9:38 am on August 27, 2021: member

ACK d4fda4f9b9c9e0cebf16381f6f49b39a43ef9ef4

[addrman] Set m_asmap in CAddrMan initializer list

This allows us to make it const.

f572f2b204

[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

[addrman] Make m_asmap private

Add a GetAsmap() getter function that returns a reference to const.

5840476714

[fuzz] Add ConsumeAsmap() function 724c497562

jnewbery force-pushed on Aug 27, 2021

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

vasild approved

vasild commented at 2:10 pm on August 27, 2021: member

ACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7

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.

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.

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

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

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.

amitiuttarwar commented at 1:35 am on September 3, 2021: contributor

code review but utACK 724c497562

naumenkogs commented at 9:18 am on September 6, 2021: member

utACK 724c4975622bc22cedc3f3814dfc8e66cf8371f7

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.

MarcoFalke approved

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 -

MarcoFalke merged this on Sep 6, 2021

MarcoFalke closed this on Sep 6, 2021

jnewbery deleted the branch on Sep 7, 2021

sidhujag referenced this in commit d6847a12f7 on Sep 7, 2021

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.

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.

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.

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

fanquake referenced this in commit 505ba39665 on Apr 22, 2022

Fabcien referenced this in commit 98906036ef on Oct 18, 2022

Fabcien referenced this in commit c778701279 on Oct 18, 2022

Fabcien referenced this in commit b525c4ac80 on Oct 18, 2022

Fabcien referenced this in commit 9a1305b071 on Oct 18, 2022

DrahtBot locked this on Oct 30, 2022

init: Fix asmap/addrman initialization order bug #22791

Conflicts