[addrman] Move serialization code to cpp

[amitiuttarwar]

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.

[jonatack]

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

[maflcko]

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

[amitiuttarwar]

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

[DrahtBot]

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.

[jnewbery]

Concept ACK. A couple of suggestions:

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

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

--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -258,14 +258,14 @@ void CAddrMan::Unserialize(Stream& s_)
 
     if (nNew > ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nNew < 0) {
         throw std::ios_base::failure(
-                strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %u]",
+                strprintf("Corrupt CAddrMan serialization: nNew=%d, should be in [0, %d]",
                     nNew,
                     ADDRMAN_NEW_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE));
     }
 
     if (nTried > ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE || nTried < 0) {
         throw std::ios_base::failure(
-                strprintf("Corrupt CAddrMan serialization: nTried=%d, should be in [0, %u]",
+                strprintf("Corrupt CAddrMan serialization: nTried=%d, should be in [0, %d]",
                     nTried,
                     ADDRMAN_TRIED_BUCKET_COUNT * ADDRMAN_BUCKET_SIZE));
     }

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.

[maflcko]

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]

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!? 🤯

[jnewbery]

Code review ACK 954785d

[amitiuttarwar]

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

[jnewbery]

Code review ACK 85b15dd

[0xB10C]

Code review ACK 85b15dd

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

[amitiuttarwar]

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

[mzumsande]

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

[mzumsande]

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]

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

Suggested change

	/** Maximum number of times an address can be added to the new table */
	/** 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 :)

[maflcko]

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

diff --git a/src/addrman.cpp b/src/addrman.cpp
index 5d9aeec1b1..9e8534e70e 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -5,10 +5,12 @@
 
 #include <addrman.h>
 
+#include <clientversion.h>
 #include <hash.h>
 #include <logging.h>
 #include <netaddress.h>
 #include <serialize.h>
+#include <streams.h>
 
 #include <cmath>
 #include <optional>
diff --git a/src/addrman.h b/src/addrman.h
index 13b53fdc78..561589d413 100644
--- a/src/addrman.h
+++ b/src/addrman.h
@@ -6,23 +6,16 @@
 #ifndef BITCOIN_ADDRMAN_H
 #define BITCOIN_ADDRMAN_H
 
-#include <clientversion.h>
-#include <config/bitcoin-config.h>
 #include <fs.h>
-#include <hash.h>
+#include <logging.h>
 #include <netaddress.h>
 #include <protocol.h>
-#include <random.h>
-#include <streams.h>
 #include <sync.h>
 #include <timedata.h>
-#include <tinyformat.h>
-#include <util/system.h>
 
-#include <iostream>
+#include <cstdint>
 #include <optional>
 #include <set>
-#include <stdint.h>
 #include <unordered_map>
 #include <vector>
 
diff --git a/src/net.cpp b/src/net.cpp
index 9b1e17c587..ddb6a2d34f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -24,6 +24,7 @@
 #include <scheduler.h>
 #include <util/sock.h>
 #include <util/strencodings.h>
+#include <util/system.h>
 #include <util/thread.h>
 #include <util/trace.h>
 #include <util/translation.h>
diff --git a/src/qt/walletframe.cpp b/src/qt/walletframe.cpp
index 30c29eb356..adbf8028ea 100644
--- a/src/qt/walletframe.cpp
+++ b/src/qt/walletframe.cpp
@@ -11,6 +11,7 @@
 #include <qt/psbtoperationsdialog.h>
 #include <qt/walletmodel.h>
 #include <qt/walletview.h>
+#include <util/system.h>
 
 #include <cassert>
 
diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp
index cd5dc2370f..f78001619e 100644
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -5,13 +5,14 @@
 #include <addrdb.h>
 #include <addrman.h>
 #include <chainparams.h>
+#include <clientversion.h>
+#include <hash.h>
+#include <netbase.h>
+#include <random.h>
 #include <test/data/asmap.raw.h>
 #include <test/util/setup_common.h>
 #include <util/asmap.h>
 #include <util/string.h>
-#include <hash.h>
-#include <netbase.h>
-#include <random.h>
 
 #include <boost/test/unit_test.hpp>
 
diff --git a/src/test/fuzz/versionbits.cpp b/src/test/fuzz/versionbits.cpp
index 9186821836..73a7d24971 100644
--- a/src/test/fuzz/versionbits.cpp
+++ b/src/test/fuzz/versionbits.cpp
@@ -6,6 +6,7 @@
 #include <chainparams.h>
 #include <consensus/params.h>
 #include <primitives/block.h>
+#include <util/system.h>
 #include <versionbits.h>
 
 #include <test/fuzz/FuzzedDataProvider.h>

[amitiuttarwar]

@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]

Post-merge ACK