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

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #28451 (Remove unused SER_DISK, SER_NETWORK, SER_GETHASH by MarcoFalke)
• #26859 (fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses by vasild)

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.

I think the last commit is incomplete? For example, the following removal compiles fine for me locally on top of this pull and master:

diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp
index 036355b5f1..37b670662f 100644
--- a/src/test/net_tests.cpp
+++ b/src/test/net_tests.cpp
@@ -331,7 +331,7 @@ BOOST_AUTO_TEST_CASE(cnetaddr_serialize_v1)
     DataStream s{};
     const auto ser_params{CAddress::V1_NETWORK};
 
-    s << WithParams(ser_params, addr);
+    s << ser_params( addr);
     BOOST_CHECK_EQUAL(HexStr(s), "00000000000000000000000000000000");
     s.clear();
 

I think it may be good to try to nuke WithParams completely. This would also address the outstanding feedback:

• net: Use serialization parameters for CAddress serialization #25284 (comment)
• and net: Use serialization parameters for CAddress serialization #25284 (comment)

I think the last commit is incomplete? For example, the following removal compiles fine for me locally on top of this pull and master:

• s << WithParams(ser_params, addr);

• s << ser_params( addr);

I only changed the ones that had constexpr parameters -- it's pretty clear what V1_DISK(info) means, but if you generalise it to anything(info) it's a bit less clear that it's just applying serialization parameters -- a function call could well be doing just about anything after all. Leaving it as WithParams(...) makes it clear that it's just applying serialization params.

Not sure that's necessarily ideal, but that's what I was thinking, fwiw.

Yeah, up to you. I personally don't see a risk in s << ser_params(addr); and it is shorter and consistent with the s << V1(addr); stuff. So I'll use it in new code, and potentially do the scripted-diff at some point, but no strong opinion.

Also would have preferred s << ser_params(addr);.

Maybe add this to #28451 when rebasing it?

Also would have preferred s << ser_params(addr);.

@TheCharlatan You might want to review #28503