It seems confusing that picking a wrong value for ADDRV2_FORMAT could have effects on consensus. (See the docstring of ADDRV2_FORMAT).
Fix this by implementing #19477 (comment) .
This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | TheCharlatan, ajtowns |
| Concept ACK | vasild |
| Stale ACK | jonatack, theuni |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
It seems confusing to have consensus (weakly) depend on low level BIP 155 addrv2 serialization.
You mean the “consensus” module, right, not actual consensus?
133@@ -133,12 +134,29 @@ enum
134 SER_GETHASH = (1 << 2),
135 };
136
137-//! Convert the reference base type to X, without changing constness or reference type.
138-template<typename X> X& ReadWriteAsHelper(X& x) { return x; }
139-template<typename X> const X& ReadWriteAsHelper(const X& x) { return x; }
140+/* Convert any argument to a reference to X, maintaining constness.
ef03a51
0/** Convert any argument to a reference to X, maintaining constness.
also here
0-/* ::GetSerializeSize implementations
1+/** ::GetSerializeSize implementations
277+ s >> str;
278+ assert(str.size() == 2 || (fmt == BaseFormat::DEC && str.size() == 3));
279+ uint32_t data;
280+ bool ok;
281+ if (fmt == BaseFormat::DEC) {
282+ ok = ParseUint32(str, &data);
6a99da9 If the #if directive is inversed
0test/serialize_tests.cpp:281:22: error: use of undeclared identifier 'ParseUint32'; did you mean 'ParseUInt32'?
1 ok = ParseUint32(str, &data);
2 ^~~~~~~~~~~
3 ParseUInt32
4./util/strencodings.h:225:20: note: 'ParseUInt32' declared here
5[[nodiscard]] bool ParseUInt32(std::string_view str, uint32_t *out);
Good catch. The test was written by @vasild. I’ve removed the code, since there is already another unit test checking SERIALIZE_METHODS_PARAMS.
Also, I’ve replaced DEC by RAW to remove the dependency on tinyformat for the unit test and instead only use Bitcoin Core’s own util functions.
154+ *
155+ * static_cast cannot easily be used here, as the type of Obj will be const Child&
156+ * during serialization and Child& during deserialization. AsBase will convert to
157+ * const Base& and Base& appropriately.
158+ */
159+template <typename X> X& AsBase(X& x) { return x; }
std::is_base_of and SFINAE until we get concepts?
std::is_base_of? IIUC the compiler already checks this internally on the argument when the function is called. And if the function call argument is a violating reinterpret_cast we can’t protect against this anyway inside this function or the function signature.
It will try conversion functions, eg:
0template<typename T> T& AsBase(T& x) { return x; }
1
2template<typename T, typename A,
3 typename = typename std::enable_if<
4 std::is_base_of<T, A>::value, void
5 >::type>
6T& AsBaseSafe(A& x) { return x; }
7
8struct Foo { };
9struct Bar : public Foo { };
10
11Bar b;
12Foo& fb = AsBase<Foo>(b); // fine
13Foo& fbsafe = AsBaseSafe<Foo>(b); // fine
14
15Bar g_foobar;
16struct Nope { operator Foo&() { return g_foobar; }; };
17Nope n;
18Foo& f = AsBase<Foo>(n); // fine
19Foo& fsafe = AsBaseSafe<Foo>(n); // error: no matching function for call to 'AsBaseSafe'
20
21struct Nope2 { operator Bar() { return g_foobar; }; };
22Nope2 n2;
23Bar& n2b = AsBase<Bar>(n2); // error: no matching function for call to 'AsBase'
24const Bar& n2bc = AsBase<const Bar>(n2); // fine
25const Foo& n2fc = AsBase<const Foo>(n2); // fine
26const Bar& n2bcsafe = AsBaseSafe<const Bar>(n2); // error: no matching...
27const Foo& n2fcsafe = AsBaseSafe<const Foo>(n2); // error: no matching...
static_assert to turn it into a compile failure.
Edit: Most of the below was written with a misunderstanding of how these params actually work. I was trying (unsuccessfully) to shoe-horn params into the existing stream behavior. I’ll leave my clueless comments in case they help anyone else with the same misunderstanding. See this follow-up comment for a more reasonable take.
What is the future of s.GetType() & SER_GETHASH and s.GetVersion() & SERIALIZE_TRANSACTION_NO_WITNESS ?
I’m working on trying to switch #28327 to this approach, but I’m running into some issues.
CBlockLocator and CDiskBlockIndex both check for SER_GETHASH and conditionally skip some serializing if we’re hashing. So i’ve introduced a bool hashing in SerParams for those classes.
But now we have a problem in net_processing:
https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/net_processing.cpp#L2630
It’s not clear how to set !hashing for locator there.
One solution is to have global params which could apply to any class:
0struct GlobalParams {
1 enum class hashing{
2 on,
3 off
4 };
5 bool is_hashing() const
6 {
7 return do_hashing == hashing::on;
8 }
9 hashing do_hashing;
10};
Then places like netmessagemaker can override for all structures:
0 CSerializedNetMsg Make(int nFlags, std::string msg_type, Args&&... args) const
1 {
2 CSerializedNetMsg msg;
3 msg.m_type = std::move(msg_type);
4 CVectorWriter vecwriter{ SER_NETWORK, nFlags | nVersion, msg.data, 0};
5 GlobalParams ser_params{GlobalParams::hashing::off};
6 ParamsStream s{ser_params, vecwriter};
7 ::SerializeMany(s, std::forward<Args>(args)...);
8 return msg;
9}
That works but:
SER_NETWORK and GlobalParams::hashing::off are redundantCTransaction fails to serialize because ParamsStream deletes GetVersion().Finally.. that could be fixed by turning SERIALIZE_TRANSACTION_NO_WITNESS into a global param as well. But that seems like it puts us back where we started, with everything jumbled up in the same place.
~So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.~
It’s also unclear to me how this should work when the thing-to-be-serialized is a template param: https://github.com/bitcoin/bitcoin/blob/33da5d0eb155993ab0d07c95704a0dd9e981c100/src/dbwrapper.h#L168-L177
In that case we’ll need to set some params for some types of V but not others. How to do that generically?
@theuni This is very generic, and I haven’t tried any of this, but roughly my thinking is the following.
For transactions (CTransaction, CMutableTransaction), I’d expect a TransactionParams with a boolean that indicates whether the witness is to be serialized or not (and whether it’s accepted on deserialization). I’d think we’d want that passed through when serializing blocks, so CBlock would also want a TransactionParams. But “higher” than that (e.g. in CNetMsgMaker or database logic) it would feel wrong to try to support having TransactionParams, because that code shouldn’t know or care about what blocks or transactions are. Instead, whenever there is code that tries to give (say) a transaction to a database writing, that code should instead serialize a WithParams(TransactionParams{.segwit=...}, tx) rather than a tx, because presumably that code knows whether witness serialization is supposed to be used. In a sense, my view is that the real problem is trying to even have a way to “globally” set witness or not - it should be confined to logic that knows what that means.
For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code.
1184+ * Return a wrapper around t that (de)serializes it with specified parameter params.
1185+ *
1186+ * See FORMATTER_METHODS_PARAMS for more information on serialization parameters.
1187+ */
1188+template <typename Params, typename T>
1189+static auto WithParams(const Params& params, T&& t)
ParamsWrapper.
should instead give a
WithParams(TransactionParams{.segwit=...}, tx)rather thantxto be serialized,
Maybe something like:
CTransaction::Serialize(stream) with CTransaction::Serialize(stream, txseropts) – so bare CTransaction isn’t serializable anymorestream << tx you write stream << seropt(tx, {.with_witness=true})In that case, I think the idea could just be to have that wrapper be something like:
0template<typename T, typename O>
1class serializable_with_options
2{
3private:
4 T& obj;
5 O opts;
6public:
7 serializable_with_options(T& obj, O&& opts) : obj{obj}, opts{std::move(opts)} { }
8
9 template<typename Stream>
10 inline void Serialize(Stream& s) const { obj.Serialize(s, opts); }
11
12 template<typename Stream>
13 inline void Unserialize(Stream& s) { obj.Unserialize(s, opts); }
14};
15
16template<typename T>
17auto seropt(T& obj, typename T::SerOptions&& opts)
18{
19 return serializable_with_options<T, typename T::SerOptions>(obj, std::move(opts));
20}
Probably needs some further tweaking to handle constyness?
Here’s a patch that may be fun to play with: https://github.com/ajtowns/bitcoin/commit/19cdc6b66cdf3c27d1fe895c39ab4075372bf99b . Seems workable?
EDIT: On second thoughts, it doesn’t seem very workable – inheriting CNetAddr options through its child classes and across vectors of CNetAddrs and such kinda sucks.
For the distinct serializations of CBlockLocator and CDiskBlockIndex, I suspect there are similar things possible. Yes, introduce a parameter, locally for them, and we’ll find a place not much higher in the stack where the parameter value can be set, without needing to go all the way to very generic code. @sipa Thanks for the explanation. After playing with this some more I realized that I had a pretty significant misunderstanding, having a hard time not still thinking in terms of setting options for the entire stream. I see what you mean now about setting the param at the appropriate level in the stack.
I took another stab at a POC for using params for CBlockLocator as you described, which I think turned out looking quite reasonable (ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
So while I appreciate the approach here for CAddress, it’s not clear to me how it interacts with the last few serialization quirks.
I take that back. Having now implemented some of those quirks in the commit above, it interacts just fine.
(ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the GetType definition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interface CHashWriter are not needed at all, see https://github.com/bitcoin/bitcoin/pull/28341
(ironically I’m not sure if anything ever actually uses hashing mode, I’ll trace that out).
I don’t think anything does, but I am not aware of a simple way to find out at compile time. Maybe use this pull request and remove the
GetTypedefinition and confirm it only fails in CTransaction/CBlock? Slightly related, most uses of the legacy interfaceCHashWriterare not needed at all, see #28341
It’s worth pointing out that, as you mentioned, it’s hard to trace this path in the current code. But a nice feature of the approach in the PR is that the params are now enforced at compile time and closer to the callsite, so it’s trivial to see what’s being used.
So after converting CBLockLocator, compile fails for every serialization because params are missing. That’s fixed by switching CDataStream -> DataStream and handling the params. If none of the deleted CDataStream usages were SER_GETHASH, I think it’s safe to say we don’t need the hashing param at all?
And yes, from a quick check, that’s the case everywhere but CTransaction. So specifically, I propose that we nuke the hashing checks in CDiskBlockIndex/CBLockLocator/CKeyPool. Presumably the lack of usage means that a hash has never had meaning for those. There’s still the question about whether or not to use dummy nVersions though.
From #19503 :
In this PR, the feature is only used for CAddress serialization, but I plan to also convert SERIALIZE_TRANSACTION_NO_WITNESS in a follow-up. @sipa follow-up reminder in case this gets merged :p
Combined with #28341 and #25284 (comment), that would go a long way towards (,if not completely) eliminating the legacy flags.
211@@ -220,13 +212,21 @@ class CNetAddr
212 return IsIPv4() || IsIPv6() || IsTor() || IsI2P() || IsCJDNS();
213 }
214
215+ enum class Encoding {
216+ V1,
217+ V2, //!< BIP155 encoding
218+ };
219+ struct SerParams {
220+ const Encoding enc;
Is this intended to default to V1? If so, be explicit with enc{V1}; if not, maybe const NoDefaultInitializer<Encoding> enc:
0template<typename T>
1struct NoDefaultInitializer
2{
3 T value;
4 NoDefaultInitializer(const T& v) : value{v} { }
5 NoDefaultInitializer(T&& v) : value{std::move(v)} { }
6 NoDefaultInitializer() = delete;
7 NoDefaultInitializer(NoDefaultInitializer&&) = default;
8 NoDefaultInitializer(const NoDefaultInitializer&) = default;
9 operator T&() { return value; }
10 operator const T&() const { return value; }
11 T& operator=(const T& v) { value = v; return value; }
12};
SerParams a; is illegal due to const, so I guess you mean SerParams b{};. But that type of bug seems impossible to fix, because one can just write NoDefaultInitializer<Encoding> d{{}};, or SerParams b{{}};.
So I’ll leave as is for now.
But that type of bug seems impossible to fix
Or rather, this may be a clang-tidy-plugin candidate, no?
{.addrv2=true} and forgetting that you should be specifying .disk=false as well. If you’re writing .disk=NoDefaultInitializaer<bool>{{}} then you’re still at least explicitly aware of the parameter…
Ah, at least for my code, I already get a warning with clang-16:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index 0582da423f..98699fd505 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -5283,7 +5283,7 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
5 msg_type = NetMsgType::ADDR;
6 ser_enc = CNetAddr::Encoding::V1;
7 }
8- m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
9+ m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}}, peer.m_addrs_to_send)));
10 peer.m_addrs_to_send.clear();
11
12 // we only send the big addr message once
0net_processing.cpp:5286:127: warning: missing field 'fmt' initializer [-Wmissing-field-initializers]
1 m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}}, peer.m_addrs_to_send)));
2 ^
31 warning generated.
Or rather, this may be a clang-tidy-plugin candidate, no?
Here’s a very quick POC in case this is desired: https://github.com/theuni/bitcoin/commit/caad1926ab4c4ee7a7f5bc6979228afd8500350d
It’s not tested against all forms of init/inheritance, so we’d likely need to fix it up a bit.
Also the max params are hard-coded because of an AST traversal quirk. That limit could be removed with a less strict match and fancier code in SerParamsCheck::check.
696@@ -697,16 +697,17 @@ BOOST_AUTO_TEST_CASE(addrman_serialization)
697 auto addrman_asmap1_dup = std::make_unique<AddrMan>(netgroupman, DETERMINISTIC, ratio);
698 auto addrman_noasmap = std::make_unique<AddrMan>(EMPTY_NETGROUPMAN, DETERMINISTIC, ratio);
699
700- CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
701+ DataStream stream{};
702+ const CAddress::SerParams ser_params{{CNetAddr::Encoding::V1}, CAddress::Format::Network};
Perhaps:
0class CAddress {
1 ...
2 static constexpr SerParams network_v1{{Encoding::V1}, Format::Network};
3};
would be a bit friendlier?
V1 params a couple of times in net_processing as well, just thought having them already done might be easier? You’d be saying WithParams(CAddress::V1_DISK, info) rather than WithParams(ser_params, info) or whatever, so it’d be slightly clearer inline as well as maybe saving a redeclaration?
Makes sense, pushed a change to reduce verbosity.
The diff should be scripted-diff, roughly:
0sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}|CAddress::V1_DISK|g' $(git grep -l SerParams)
1sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V2}, CAddress::Format::Disk}|CAddress::V2_DISK|g' $(git grep -l SerParams)
2sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V2}, CAddress::Format::Network}|CAddress::V2_NETWORK|g' $(git grep -l SerParams)
3sed -i 's|CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Network}|CAddress::V1_NETWORK|g' $(git grep -l SerParams)
4sed -i 's|CAddress::SerParams ser_params{{CNetAddr::Encoding::V1}, CAddress::Format::Network}|auto ser_params{CAddress::V1_NETWORK}|g' $(git grep -l SerParams)
5sed -i 's|CAddress::SerParams ser_params{{CNetAddr::Encoding::V2}, CAddress::Format::Network}|auto ser_params{CAddress::V2_NETWORK}|g' $(git grep -l SerParams)
6sed -i 's|CNetAddr::SerParams ser_params{CNetAddr::Encoding::V1}|auto ser_params{CNetAddr:V1}|g' $(git grep -l SerParams)
7sed -i 's|CNetAddr::SerParams{CNetAddr::Encoding::V1}|CNetAddr::V1|g' $(git grep -l SerParams)
8sed -i 's|CNetAddr::SerParams{CNetAddr::Encoding::V2}|CNetAddr::V2|g' $(git grep -l SerParams)
5274@@ -5272,15 +5275,15 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
5275 if (peer.m_addrs_to_send.empty()) return;
5276
5277 const char* msg_type;
5278- int make_flags;
5279+ CNetAddr::Encoding ser_params;
ser_params
225 */
226 template <typename Stream>
227 void Serialize(Stream& s) const
228 {
229- if (s.GetVersion() & ADDRV2_FORMAT) {
230+ if (s.GetParams().enc == Encoding::V2) {
If the stream has parameters but not the right parameters this gives a mildly confusing error. Writing it as:
0 template <typename Stream>
1 void Serialize(Stream& s, const SerParams& params) const
2
3 template <typename Stream> void Serialize(Stream& s) const { return Serialize(s, s.GetParams()); }
seems a bit nicer. It seems like there should be some way to have ::Serialize() call this with the parameters directly via some template magic, but I can’t see what it would be.
const SerParams& params{s.GetParams()}; in the first line of the function.
::Serialize to call obj.Serialize(s, s.GetParams()) directly which seemed nicer, but with just C++17 I got myself too confused.
998+ auto ssPeers2{AddrmanToStream(addrman)};
999
1000 AddrMan addrman2{EMPTY_NETGROUPMAN, !DETERMINISTIC, GetCheckRatio(m_node)};
1001 BOOST_CHECK(addrman2.Size() == 0);
1002- ReadFromStream(addrman2, ssPeers2);
1003+ ReadFromStreamUnitTests(WithParams(CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}, addrman2), ssPeers2);
FWIW, I would have written:
0WithParams(CAddress::SerParams{{CNetAddr::Encoding::V1}, CAddress::Format::Disk}, addrman2)
more like:
0WithParams<CAddress>({.addrv2=false, .disk=true}, addrman2)
It is now:
0WithParams(CAddress::V1_DISK, addrman2)
395+ Disk,
396+ Network,
397+ };
398+ struct SerParams : CNetAddr::SerParams {
399+ const Format fmt;
400+ };
{.addrv2=false, .disk=false}, then you can’t use inheritance here, but could instead add an operator CNetAddr::SerParams() const function for pretty much the same result.
Instead of WithParams(Foo::SerParams{Foo::Bar::VALUE}, obj) I had seropt<Foo>(obj, {.var=value}); implemented something like:
0template<typename O, typename T>
1inline auto seropt(T& obj, const SerializationOptions<O>& opts) {
2 return ParamsWrapper<SerializationOptions<O>,T>(opts, obj);
3}
4
5template<typename T>
6inline auto seropt(T& obj, const SerializationOptions<T>& opts) { return seropt<T,T>(obj, opts); }
7
8template<>
9struct SerializationOptions<class CAddress> {
10 bool addrv2{false};
11 bool to_disk{false};
12 operator SerializationOptions<class CNetAddr>() const { return {.addrv2=addrv2}; }
13};
Saves a bit of boiler plate, not sure it’s entirely enough to be worthwhile.
Approach ACK fa452431db62f780abd3e4cb34ab9523214b9d52
Essentially ended up reinventing this PR from scratch trying to figure out a nicer way of doing things
Co-authored-by: Pieter Wuille <pieter@wuille.net>
This allows new code, added in the next commit, to conform to the coding
guideline: No C-prefix for class names.
The moved part can be reviewed with the git options
--ignore-all-space --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
(Modified by Marco Falke)
Co-authored-by: Pieter Wuille <pieter@wuille.net>
1413@@ -1414,9 +1414,10 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
1414 uint64_t your_services{addr.nServices};
1415
1416 const bool tx_relay{!RejectIncomingTxs(pnode)};
1417+ const auto ser_params{CNetAddr::V1};
1418 m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
1419- your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
1420- my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
1421+ your_services, WithParams(ser_params, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
ser_params here (and in ProcessMessage) seems less clear than just using CNetAddr::V1 directly.
Yes, good point. It makes little sense to assign an existing short alias to a const auto alias when it is only used once or twice.
Fixed in all files in this commit.
Re-ACK fafca6b73310a4449918b95e3895ac434a2e096a
Didn’t re-review tests, only the main updates.
1133@@ -1080,4 +1134,61 @@ size_t GetSerializeSizeMany(int nVersion, const T&... t)
1134 return sc.size();
1135 }
1136
1137+/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
GetParams defined that the method here overrides? Does this refer to the underlying type instead?
Good question. I wonder if it makes sense to override the GetParams() function at all. Generally, no underlying stream type has a GetParams() method, so “overriding” really means “defines”.
And if the GetParams() method is truly overridden, it should be rare, and it could make more sense to first require the stream to reveal its underlying unwrapped stream, without the GetParams() method?
I think you could do:
0#ifdef HAVE_CXX20
1template<typename Stream>
2concept ParamsStreamConcept = requires(Stream& s) { s.GetParams(); };
3template<typename Stream>
4concept PlainStreamConcept = !ParamsStreamConcept<Stream>;
5#else
6template<typename Stream>
7bool ParamsStreamConcept = true;
8template<typename Stream>
9bool PlainStreamConcept = true;
10#endif
11
12 /** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
13 template <typename Params, typename SubStream>
14 class ParamsStream
15 {
16 static_assert(PlainStreamConcept<SubStream>, "avoid wrapping a ParamsStream in a ParamsStream");
17 SubStream& GetSubStream() { return m_substream; }
or similar if you wanted, which would catch this sort of case via the C++20 CI builds. At present the only lines that violate this rule are READWRITE(WithParams(ser_params, AsBase<CService>(obj))); in protocol.h and Derived::SERIALIZE_METHOD_PARAMS in serialize_tests.cpp; you need a GetSubStream getter to fix those up.
Just for reference, GetSubStream was present in the original code, but only to avoid too-deeply nested templates, see https://github.com/bitcoin/bitcoin/pull/19503/files#r455147534
Since it wasn’t enforced, I removed it. I’ll add your patch once and if there is C++20.
173@@ -175,13 +174,15 @@ bool CBanDB::Read(banmap_t& banSet)
174 bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr)
175 {
176 const auto pathAddr = args.GetDataDirNet() / "peers.dat";
177- return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
178+ return SerializeFileDB("peers", pathAddr, WithParams(CAddress::V1_DISK, addr));
V1_DISK or V2_DISK should be used), so it shouldn’t be necessary to provide it with a CAddress parameter (and doing so is misleading, as it’d be overridden anyway).
I agree this is misleading, but the unit and fuzz tests would still set Network, as opposed to Disk sometimes. I agree with the cleanup, but I think this is a slight change (improvement) in what the unit and fuzz tests are testing.
Though, as it only affects tests, I think the refactoring label can be kept on this pull request.
+1 Patch with these changes is at https://github.com/ajtowns/bitcoin/commits/202309-serializeaddrmansimp if you want to squash it in
Thanks for the patch! I’ve squashed it and adjusted the commit message to explain the test changes.
191@@ -191,7 +192,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
192 const auto start{SteadyClock::now()};
193 const auto path_addr{args.GetDataDirNet() / "peers.dat"};
194 try {
195- DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
196+ DeserializeFileDB(path_addr, WithParams(CAddress::V1_DISK, *addrman));
170@@ -171,8 +171,8 @@ void AddrManImpl::Serialize(Stream& s_) const
171 */
172
173 // Always serialize in the latest version (FILE_FORMAT).
174-
175- OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
176+ const CAddress::SerParams ser_params{{CNetAddr::Encoding::V2}, s_.GetParams().fmt};
V2_DISK here).
Not in the unit tests, see #25284 (review) and:
0test/addrman_tests.cpp(690): Entering test case "addrman_serialization"
1test_bitcoin: addrman.cpp:175: void AddrManImpl::Serialize(Stream &) const [Stream = ParamsStream<CAddress::SerParams, DataStream>]: Assertion `s_.GetParams().fmt==CAddress::Format::Disk' failed.
2make[3]: *** [Makefile:22464: test/addrman_tests.cpp.test] Error 1
246+ stream_enc = CNetAddr::Encoding::V2;
247 }
248
249- OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
250+ const CAddress::SerParams ser_params{{stream_enc}, s_.GetParams().fmt};
251+ ParamsStream s{ser_params, s_};
V1_DISK and V2_DISK.
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Current list of linter/style questions to explore:
959+ * Support for all macros providing or using the ser_action parameter of the SerializationOps method.
960 */
961-struct CSerActionSerialize
962-{
963+struct ActionSerialize {
964 constexpr bool ForRead() const { return false; }
These could be static constexpr bool ForRead() { return ...; }
Actually, ::SerRead etc could be static member functions of these classes rather than being the global namespace too:
0//#define READWRITE(...) (::SerReadWriteMany(s, ser_action, __VA_ARGS__))
1#define READWRITE(...) (ser_action.SerReadWriteMany(s, __VA_ARGS__))
2
3struct ActionSerialize
4{
5 static constexpr bool ForRead() { return false; }
6
7 template<typename Stream, typename... Args>
8 static void SerReadWriteMany(Stream& s, const Args&... args)
9 {
10 ::SerializeMany(s, args...);
11 }
Maybe something to consider later.
1157+
1158+/** Wrapper that serializes objects with the specified parameters. */
1159+template <typename Params, typename T>
1160+class ParamsWrapper
1161+{
1162+ static_assert(std::is_lvalue_reference<T>::value, "ParamsWrapper needs an lvalue reference type T");
T& m_object and ParamsWrapper(.., T& obj), ParamsWrapper<.., FooObj> (instead of FooObj&) and drop this assert?
Followups / tidy / lint to keep track of:
Since this code is mostly of the form s << WithParams(V1_DISK, data) we could shorten the common case a bit more:
0struct SerParams {
1 ...
2 template <typename T>
3 auto operator()(T& obj) const { return WithParams(*this, obj); }
4};
5
6s << V1_DISK(data)
That way when reading the code you’re really only seeing the info you care about.
5282 msg_type = NetMsgType::ADDR;
5283- make_flags = 0;
5284+ ser_enc = CNetAddr::Encoding::V1;
5285 }
5286- m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, peer.m_addrs_to_send));
5287+ m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
I guess the Make can be written shorter as:
0+const bool peer_v2{peer.m_wants_addrv2};
1-...Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send))
2+...Make(peer_v2 ? NetMsgType::ADDRV2 : NetMsgType::ADDR, peer_v2 ? CAddress::V2_NETWORK : CAddress::V1_NETWORK, peer.m_addrs_to_send))
and dropping the above if.
256+ const auto maybe_na{ConsumeDeserializable<CNetAddr>(fdp, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp))};
257+ if (!maybe_na) return;
258+ const CNetAddr& na{*maybe_na};
259 if (na.IsAddrV1Compatible()) {
260- AssertEqualAfterSerializeDeserialize(na);
261+ AssertEqualAfterSerializeDeserialize(na, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp));
274+ const auto maybe_s{ConsumeDeserializable<CService>(fdp, ser_params)};
275+ if (!maybe_s) return;
276+ const CService& s{*maybe_s};
277 if (s.IsAddrV1Compatible()) {
278- AssertEqualAfterSerializeDeserialize(s);
279+ AssertEqualAfterSerializeDeserialize(s, ConsumeDeserializationParams<CNetAddr::SerParams>(fdp));
326- AssertEqualAfterSerializeDeserialize(a, PROTOCOL_VERSION);
327- AssertEqualAfterSerializeDeserialize(a, 0, SER_DISK);
328+FUZZ_TARGET(address_deserialize, .init = initialize_deserialize)
329+{
330+ FuzzedDataProvider fdp{buffer.data(), buffer.size()};
331+ const auto ser_enc{ConsumeDeserializationParams<CNetAddr::SerParams>(fdp)};
CAddress::SerParams to cover a wider range?
post-merge self-re-review, left some fuzz nits
edit: fixed in https://github.com/bitcoin/bitcoin/pull/28470
