refactor: Serialization parameter cleanups #28473

pull ajtowns wants to merge 4 commits into bitcoin:master from ajtowns:202309-pretty-serparam changing 10 files +87 −71

ajtowns commented at 12:30 AM on September 14, 2023: contributor
Cleanups after #25284:
- ser_action namespacing - #25284 (review)
- make reference implicit - #25284 (review)
- function notation - #25284 (comment)
serialize: move ser_action functions out of global namespace bf147bfffa
serialize: specify type for ParamsWrapper not ref 33203f59b4
serialize: add SER_PARAMS_OPFUNC 5e5c8f86b6

scripted-diff: use SER_PARAMS_OPFUNC

-BEGIN VERIFY SCRIPT-
sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's@\(CNetAddr::V1.CService{}.*\)    //@\1                //@' src/test/util/net.cpp
-END VERIFY SCRIPT-

fb6a2ab63e

DrahtBot commented at 12:30 AM on September 14, 2023: contributor


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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK MarcoFalke, TheCharlatan

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.
in src/serialize.h:1051 in fb6a2ab63e
```
1049 |  struct ActionUnserialize {
1050 | -    constexpr bool ForRead() const { return true; }
1051 | -};
1052 | +    static constexpr bool ForRead() { return true; }
1053 |  
1054 | +    template<typename Stream, typename... Args>
```
maflcko commented at 9:38 AM on September 14, 2023:

nit in bf147bfffa1afb11721f30e83eec1fa829f64d5f: Forgot to run clang-format? (Shouldn't cause any issues, because reviewers will have to use --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space either way)
DrahtBot renamed this:
~~Serialization parameter cleanups~~
refactor: Serialization parameter cleanups
on Sep 14, 2023
DrahtBot added the label Refactoring on Sep 14, 2023

Type	Reviewers
ACK	MarcoFalke, TheCharlatan

maflcko commented at 10:07 AM on September 14, 2023: member

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:

#25284 (review)
and #25284 (review)

maflcko approved

maflcko commented at 10:07 AM on September 14, 2023: member

lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨
ywac3bAMrnfFHQLulX4zEWpRkiedwedVsrfX38Q39iL2lYR4s+D+pEv/I+9r777WVdScxjp7uMSq+J08QgcZDg==

</details>

fanquake requested review from theuni on Sep 14, 2023
fanquake requested review from TheCharlatan on Sep 14, 2023
ajtowns commented at 10:33 AM on September 14, 2023: contributor
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.
maflcko commented at 10:40 AM on September 14, 2023: member

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.
TheCharlatan approved
TheCharlatan commented at 11:01 AM on September 15, 2023: contributor

ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0

I personally don't see a risk in s << ser_params(addr); and it is shorter and consistent with the s << V1(addr); stuff.

Also would have preferred s << ser_params(addr);.
fanquake merged this on Sep 15, 2023
fanquake closed this on Sep 15, 2023
ajtowns commented at 7:06 PM on September 15, 2023: contributor

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

Maybe add this to #28451 when rebasing it?
Frank-GER referenced this in commit 775a2bcf9d on Sep 19, 2023
ajtowns commented at 5:50 AM on October 27, 2023: contributor

Also would have preferred s << ser_params(addr);. @TheCharlatan You might want to review #28503
bitcoin locked this on Oct 26, 2024

Contributors

Review Requested
theuni

Labels

Refactoring

Linked (view graph)

#25284 net: Use serialization parameters for CAddress serialization

refactor: Serialization parameter cleanups #28473

Reviews

Conflicts