Skip to content

refactor: Serialization parameter cleanups #28473

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 4 commits into from
Sep 15, 2023
Merged

refactor: Serialization parameter cleanups #28473

fanquake merged 4 commits into from
Sep 15, 2023

Conversation

@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Sep 14, 2023

@ajtowns
serialize: move ser_action functions out of global namespace
bf147bf
@ajtowns
serialize: specify type for ParamsWrapper not ref
33203f5
@ajtowns
serialize: add SER_PARAMS_OPFUNC
5e5c8f8
@ajtowns
scripted-diff: use SER_PARAMS_OPFUNC
fb6a2ab 
-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-
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 14, 2023 •
edited
Loading

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.

@ajtowns ajtowns mentioned this pull request Sep 14, 2023
Merged
This was referenced Sep 14, 2023
Merged
Merged
@DrahtBot DrahtBot changed the title Serialization parameter cleanups refactor: Serialization parameter cleanups Sep 14, 2023
@maflcko
Copy link
Member

maflcko commented Sep 14, 2023

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:

maflcko
maflcko approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK fb6a2ab 💨

Show signature

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==

src/serialize.h
};
static constexpr bool ForRead() { return true; }

template<typename Stream, typename... Args>
Copy link
Member

@maflcko maflcko Sep 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in bf147bf: 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)

@fanquake fanquake requested review from sedited and theuni September 14, 2023 10:15
@ajtowns
Copy link
Contributor Author

ajtowns commented Sep 14, 2023

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
Copy link
Member

maflcko commented Sep 14, 2023

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.

sedited
sedited approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fb6a2ab

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 fanquake merged commit 5c7cdda into bitcoin:master Sep 15, 2023
@ajtowns
Copy link
Contributor Author

ajtowns commented Sep 15, 2023

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

Maybe add this to #28451 when rebasing it?

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
@fanquake
Merge bitcoin#28473: refactor: Serialization parameter cleanups
775a2bc
@ajtowns
Copy link
Contributor Author

ajtowns commented Oct 27, 2023

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

@TheCharlatan You might want to review #28503

@bitcoin bitcoin locked and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

@theuni theuni Awaiting requested review from theuni

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ajtowns @DrahtBot @maflcko @sedited @fanquake