[addrman] Move serialization code to cpp

amitiuttarwar commented at 5:10 am on August 19, 2021: contributor

Moving the serialization code from the header to the cpp helps clarify interfaces vs internals, as well as speed up the compilation of the whole program with a smaller header file.

fanquake added the label P2P on Aug 19, 2021

jonatack commented at 1:36 pm on August 20, 2021: member

Concept ACK. Ready for rebase now that #22697 is merged.

MarcoFalke commented at 1:45 pm on August 20, 2021: member

Is there a reason to not move every implementation to the cpp file?

amitiuttarwar force-pushed on Aug 20, 2021

amitiuttarwar commented at 7:48 pm on August 20, 2021: contributor

@MarcoFalke

Is there a reason to not move every implementation to the cpp file?

hmm, are you referring to the definitions of the wrapper functions? Most of what’s left in the header are the simple public functions that acquire lock, call check & invoke the private functions.

amitiuttarwar marked this as ready for review on Aug 20, 2021

DrahtBot commented at 2:15 am on August 21, 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:

#22734 (addrman: Avoid crash on corrupt data, Force Check after deserialize by MarcoFalke)
#22094 (p2p: fix ubsan addrman errors, make nTime truncation conversion explicit by jonatack)

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.

in src/test/addrman_tests.cpp:115 in 1e4744cd8b outdated

110@@ -111,8 +111,8 @@ class CAddrManTest : public CAddrMan
111     {
112         LOCK(cs);
113         int nId = mapAddr[addr];
114-        for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
115-            for (int entry = 0; entry < ADDRMAN_BUCKET_SIZE; ++entry) {
116+        for (size_t bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
117+            for (size_t entry = 0; entry < ADDRMAN_BUCKET_SIZE; ++entry) {

jnewbery commented at 3:58 pm on August 25, 2021:

I don’t think these need to change any more, now that you’re defining ADDRMAN_NEW_BUCKET_COUNT and ADDRMAN_BUCKET_SIZE as ints.

amitiuttarwar commented at 3:30 am on August 26, 2021:

fixed, thanks!

jnewbery commented at 4:09 pm on August 25, 2021: member

Concept ACK. A couple of suggestions:

Add the following reviewer hint to commits [addrman] Move CAddrMan::Serialize to cpp file, [addrman] Move CAddrMan::Unserialize to cpp file and [move-only] Extract constants from addrman .h to .cpp:

git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space

The format specifier for printing the signed ints here is incorrect:

 0--- a/src/addrman.cpp
 1+++ b/src/addrman.cpp
 2@@ -258,14 +258,14 @@ void CAddrMan::Unserialize(Stream& s_)
 3 
 4     if (nNew > ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nNew < 0) {
 5         throw std::ios_base::failure(
 6-                strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %u]",
 7+                strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %d]",
 8                     nNew,
 9                     ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE));
10     }
11 
12     if (nTried > ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nTried < 0) {
13         throw std::ios_base::failure(
14-                strprintf("Corrupt CAddrMan serialization: nTried=%d, should be in [0, %u]",
15+                strprintf("Corrupt CAddrMan serialization: nTried=%d, should be in [0, %d]",
16                     nTried,
17                     ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE));
18     }

I haven’t checked format specifiers in other places. This may help in fixing them up: https://en.cppreference.com/w/cpp/io/c/fprintf.

MarcoFalke commented at 4:27 pm on August 25, 2021: member

I haven’t checked format specifiers in other places

We use tinyformat, so when in doubt you can (at no risk) just always use %s.

amitiuttarwar force-pushed on Aug 26, 2021

amitiuttarwar commented at 3:41 am on August 26, 2021: contributor

thanks for the review @jnewbery. took all your suggestions & updated the commit messages with reviewer hints. @MarcoFalke thanks for the tip about tinyformat! I had not realized the specifier interchangeability that provides. I did update the inconsistency that John pointed out, but seems like getting these specifiers right is generally unnecessary in our codebase!? 🤯

in src/addrman.cpp:1036 in 954785dd1d outdated

1032@@ -733,3 +1033,4 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
1033     }
1034     return bits;
1035 }
1036+

jnewbery commented at 1:15 pm on August 26, 2021:

Please remove extra newline

amitiuttarwar commented at 6:56 pm on August 26, 2021:

fixed

in src/addrman.cpp:261 in 954785dd1d outdated

256+        nUBuckets ^= (1 << 30);
257+    }
258+
259+    if (nNew > ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nNew < 0) {
260+        throw std::ios_base::failure(
261+                strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %d]",

jnewbery commented at 1:17 pm on August 26, 2021:

These changes to the format specifiers should be in [addrman] Change addrman #define constants to be constexprs (where you declare the types of the constants) rather than [move-only] Extract constants from addrman .h to .cpp (which should be move-only)

amitiuttarwar commented at 6:57 pm on August 26, 2021:

blergh, my bad. fixed now.

jnewbery commented at 1:17 pm on August 26, 2021: member

Code review ACK 954785dd1d37968aba618743baf25760b8105a4c

[addrman] Move CAddrMan::Serialize to cpp file

Reviewer hint: use `git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space`

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

1622543cf4

[addrman] Move CAddrMan::Unserialize to cpp file

Reviewer hint: use `git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space`

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

a65053f1d4

[addrman] Change addrman #define constants to be constexprs

Co-authored-by: John Newbery <john@johnnewbery.com>

7dc443a62d

[move-only] Extract constants from addrman .h to .cpp

Reviewer hint: use `git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space`

Co-authored-by: John Newbery <john@johnnewbery.com>

af9638a0fb

[refactor] [addrman] Update constant comments

Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>

85b15ddc8f

amitiuttarwar force-pushed on Aug 26, 2021

amitiuttarwar commented at 6:58 pm on August 26, 2021: contributor

thanks for the review @jnewbery, I fixed those small issues.

jnewbery commented at 10:05 am on August 27, 2021: member

Code review ACK 85b15ddc8ff499fe21d8ab35ece3994f8878b3de

0xB10C commented at 12:12 pm on August 30, 2021: member

Code review ACK 85b15ddc8

Thanks for the reviewer hints. I found https://stackoverflow.com/questions/42388077/constexpr-vs-macros/42388687 useful to learn about #define vs constexpr.

amitiuttarwar commented at 4:46 pm on August 30, 2021: contributor

thanks @0xB10C, for the review and the useful link :)

in src/addrman.cpp:22 in 85b15ddc8f

22-//! over how many buckets entries with new addresses originating from a single group are spread
23+/** Over how many buckets entries with new addresses originating from a single group are spread */
24 static constexpr uint32_t ADDRMAN_NEW_BUCKETS_PER_SOURCE_GROUP{64};
25-
26-//! in how many buckets for entries with new addresses a single address may occur
27+/** Maximum number of times an address can be added to the new table */

mzumsande commented at 9:02 pm on August 31, 2021:

nit: I think the old comment was more precise in referring to state rather than process (“added”) - an address could be added more often over time, in the not untypical case where earlier instances were replaced due to new table collisions.

amitiuttarwar commented at 9:42 pm on August 31, 2021:

ah, I see what you’re saying. maybe this would be better?

0/** Maximum number of times an address can occur in the new table */

I’m not planning to address right now, but will keep it in mind for future changes :)

mzumsande commented at 9:32 pm on August 31, 2021: member

Code Review ACK 85b15ddc8ff499fe21d8ab35ece3994f8878b3de (+ performed some light testing)

fanquake merged this on Sep 1, 2021

fanquake closed this on Sep 1, 2021

sidhujag referenced this in commit ab79dcf8f2 on Sep 1, 2021

MarcoFalke commented at 9:40 am on September 2, 2021: member

When moving code from the h to the cpp, it could make sense to also move the includes:

  0diff --git a/src/addrman.cpp b/src/addrman.cpp
  1index 5d9aeec1b1..9e8534e70e 100644
  2--- a/src/addrman.cpp
  3+++ b/src/addrman.cpp
  4@@ -5,10 +5,12 @@
  5 
  6 #include <addrman.h>
  7 
  8+#include <clientversion.h>
  9 #include <hash.h>
 10 #include <logging.h>
 11 #include <netaddress.h>
 12 #include <serialize.h>
 13+#include <streams.h>
 14 
 15 #include <cmath>
 16 #include <optional>
 17diff --git a/src/addrman.h b/src/addrman.h
 18index 13b53fdc78..561589d413 100644
 19--- a/src/addrman.h
 20+++ b/src/addrman.h
 21@@ -6,23 +6,16 @@
 22 #ifndef BITCOIN_ADDRMAN_H
 23 #define BITCOIN_ADDRMAN_H
 24 
 25-#include <clientversion.h>
 26-#include <config/bitcoin-config.h>
 27 #include <fs.h>
 28-#include <hash.h>
 29+#include <logging.h>
 30 #include <netaddress.h>
 31 #include <protocol.h>
 32-#include <random.h>
 33-#include <streams.h>
 34 #include <sync.h>
 35 #include <timedata.h>
 36-#include <tinyformat.h>
 37-#include <util/system.h>
 38 
 39-#include <iostream>
 40+#include <cstdint>
 41 #include <optional>
 42 #include <set>
 43-#include <stdint.h>
 44 #include <unordered_map>
 45 #include <vector>
 46 
 47diff --git a/src/net.cpp b/src/net.cpp
 48index 9b1e17c587..ddb6a2d34f 100644
 49--- a/src/net.cpp
 50+++ b/src/net.cpp
 51@@ -24,6 +24,7 @@
 52 #include <scheduler.h>
 53 #include <util/sock.h>
 54 #include <util/strencodings.h>
 55+#include <util/system.h>
 56 #include <util/thread.h>
 57 #include <util/trace.h>
 58 #include <util/translation.h>
 59diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp
 60index 30c29eb356..adbf8028ea 100644
 61--- a/src/qt/walletframe.cpp
 62+++ b/src/qt/walletframe.cpp
 63@@ -11,6 +11,7 @@
 64 #include <qt/psbtoperationsdialog.h>
 65 #include <qt/walletmodel.h>
 66 #include <qt/walletview.h>
 67+#include <util/system.h>
 68 
 69 #include <cassert>
 70 
 71diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
 72index cd5dc2370f..f78001619e 100644
 73--- a/src/test/addrman_tests.cpp
 74+++ b/src/test/addrman_tests.cpp
 75@@ -5,13 +5,14 @@
 76 #include <addrdb.h>
 77 #include <addrman.h>
 78 #include <chainparams.h>
 79+#include <clientversion.h>
 80+#include <hash.h>
 81+#include <netbase.h>
 82+#include <random.h>
 83 #include <test/data/asmap.raw.h>
 84 #include <test/util/setup_common.h>
 85 #include <util/asmap.h>
 86 #include <util/string.h>
 87-#include <hash.h>
 88-#include <netbase.h>
 89-#include <random.h>
 90 
 91 #include <boost/test/unit_test.hpp>
 92 
 93diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
 94index 9186821836..73a7d24971 100644
 95--- a/src/test/fuzz/versionbits.cpp
 96+++ b/src/test/fuzz/versionbits.cpp
 97@@ -6,6 +6,7 @@
 98 #include <chainparams.h>
 99 #include <consensus/params.h>
100 #include <primitives/block.h>
101+#include <util/system.h>
102 #include <versionbits.h>
103 
104 #include <test/fuzz/FuzzedDataProvider.h>

amitiuttarwar commented at 2:31 pm on September 2, 2021: contributor

@MarcoFalke good point. I’m planning to open a pull along these lines in the next few days, so will include this patch there. thanks :)

jonatack commented at 3:31 pm on September 2, 2021: member

Post-merge ACK

DrahtBot locked this on Sep 2, 2022

[addrman] Move serialization code to cpp #22740

Conflicts