serialization: Support for multiple parameters

ryanofsky commented at 1:31 am on November 23, 2023: contributor

Currently it is only possible to attach one serialization parameter to a stream at a time. For example, it is not possible to set a parameter controlling the transaction format and a parameter controlling the address format at the same time because one parameter will override the other.

This limitation is inconvenient for multiprocess code since it is not possible to create just one type of stream and serialize any object to it. Instead it is necessary to create different streams for different object types, which requires extra boilerplate and makes using the new parameter fields a lot more awkward than the older version and type fields.

Fix this problem by allowing an unlimited number of serialization stream parameters to be set, and allowing them to be requested by type. Later parameters will still override earlier parameters, but only if they have the same type.

For an example of different ways multiple parameters can be set, see the new with_params_multi unit test.

This change requires replacing the stream.GetParams() method with a stream.GetParams<T>() method in order for serialization code to retrieve the desired parameters. The change is more verbose, but probably a good thing for readability because previously it could be difficult to know what type the GetParams() method would return, and now it is more obvious.

This PR is part of the process separation project.

DrahtBot commented at 1:31 am on November 23, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, TheCharlatan, sipa
Concept ACK	dergoegge
Ignored review	ajtowns

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:

#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)

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/primitives/transaction.h:337 in 554a0dd18c outdated

331@@ -332,7 +332,7 @@ class CTransaction
332     template <typename Stream>
333     CTransaction(deserialize_type, const TransactionSerParams& params, Stream& s) : CTransaction(CMutableTransaction(deserialize, params, s)) {}
334     template <typename Stream>
335-    CTransaction(deserialize_type, ParamsStream<TransactionSerParams,Stream>& s) : CTransaction(CMutableTransaction(deserialize, s)) {}
336+    CTransaction(deserialize_type, Stream& s) : CTransaction(CMutableTransaction(deserialize, s)) {}

ajtowns commented at 1:40 am on November 23, 2023:

This is more generalising, than dropping…

ryanofsky commented at 10:50 am on November 23, 2023:

re: #28929 (review)

This is more generalising, than dropping…

I think describing this as generalizing would incorrectly imply that some functionality is being added, when then function definitions aren’t even changing, just the declarations. Motivation for this change is not really to generalize, just to drop an unnecessary reference to the ParamsStream class to simplify the next commit, and to pass streams the same way here as they are passed other places.

in src/protocol.h:412 in 334f75acfd outdated

405@@ -406,9 +406,10 @@ class CAddress : public CService
406     static constexpr SerParams V1_DISK{{CNetAddr::Encoding::V1}, Format::Disk};
407     static constexpr SerParams V2_DISK{{CNetAddr::Encoding::V2}, Format::Disk};
408 
409-    SERIALIZE_METHODS_PARAMS(CAddress, obj, SerParams, params)
410+    SERIALIZE_METHODS(CAddress, obj)
411     {
412         bool use_v2;
413+        auto& params = SER_PARAMS(SerParams);

ajtowns commented at 2:30 am on November 23, 2023:

Dropping SERIALIZE_METHODS_PARAMS doesn’t seem like an improvement?

I would have expected something more like:

 0// this pr: #define SER_PARAMS(type) (s.template GetParams<type>())
 1// instead:
 2template<typename T, typename Stream>
 3const auto& GetSerParams(const Stream& s)
 4{
 5    return s.template GetParams<T>();
 6}
 7
 8// FORMATTER_METHODS_PARAMS has the same signature, but now uses the new GetSerParams<>()
 9#define FORMATTER_METHODS_PARAMS(cls, obj, paramcls, paramobj) \
10    template<typename Stream> \
11    static void Ser(Stream& s, const cls& obj) { SerializationOps(obj, s, GetSerParams<paramcls>(s), ActionSerialize{}); } \
12    template<typename Stream> \
13    static void Unser(Stream& s, cls& obj) { SerializationOps(obj, s, GetSerParams<paramcls>(s), ActionUnserialize{}); } \
14    template<typename Stream, typename Type, typename Operation> \
15    static void SerializationOps(Type& obj, Stream& s, const paramcls& paramobj, Operation ser_action)
16
17// protocol.h
18// this pr:
19//  SERIALIZE_METHODS(CAddress, obj) {
20//      auto& params = SER_PARAMS(SerParams);
21// instead:
22    SERIALIZE_METHODS_PARAMS(CAddress, obj, SerParams, params) {
23
24
25// netaddress.h:
26// original:
27//      if (s.GetParams().enc == Encoding::V2) {
28// this pr:
29//      if (s.template GetParams<SerParams>().enc == Encoding::V2) {
30// instead:
31        if (GetSerParams<SerParams>(s).enc == Encoding::V2) { ... }

ryanofsky commented at 9:58 am on November 23, 2023:

re: #28929 (review)

Dropping SERIALIZE_METHODS_PARAMS doesn’t seem like an improvement?

I think SER_PARAMS is an improvement over SERIALIZE_METHODS_PARAMS and FORMATTER_METHODS_PARAMS because:

It allows retrieving an arbitrary number of parameters, not just one.
It makes unified SERIALIZE_METHODS and separated Serialize/Unserialize method definitions more similar to each other. In a unified method you use READWRITE(field), and in separated methods you write stream << field and stream >> field. In a unified method, you use SER_PARAMS(type) and in separated methods you write stream.template GetParams<type>(). This way there is a 1:1 correspondence between code in unified and separate methods without unnecessary differences.
It deduplicates code and eliminates confusing macros. FORMATTER_METHODS_PARAMS was a copy of FORMATTER_METHODS and SERIALIZE_METHODS_PARAMS was a copy of SERIALIZE_METHODS. Having duplicate code makes the library confusing to navigate and can become a maintenance burden when the definitions get out of sync or fixing a bug or adding a feature requires updating both sets of the definitions. Our own example code was previously broken on line 208, because it was confusing FORMATTER_METHODS for FORMATTER_METHODS_PARAMS. Having fewer duplicated and similarly named macros is better for users and maintainers of the serialization library.

I think #25284 made a (small) mistake by adding new duplicate variants of macros instead of just extending the existing macros, and this commit cleans that up. I could probably split this commit up into two parts, first deleting the duplicate macros, then adding multiple parameter support. As I was implementing this change, it just seemed easier to delete the duplicate code than to update it, so I did both things at once.

ajtowns commented at 2:31 am on November 23, 2023: contributor

Concept ACK

dergoegge commented at 10:29 am on November 24, 2023: member

Concept ACK

ryanofsky force-pushed on Nov 28, 2023

ryanofsky commented at 9:40 pm on November 28, 2023: contributor

Updated 334f75acfdfd8c8737f0fc5da41d25ec8e6ab588 -> 97338eec9e35401f42c376062ea161a22143d628 (pr/iparams.1 -> pr/iparams.2, compare) adding comments and improving test

DrahtBot added the label CI failed on Nov 29, 2023

DrahtBot added the label Needs rebase on Nov 30, 2023

serialization: Support for multiple parameters

This commit makes a minimal change to the ParamsStream class to let it retrieve
multiple parameters. Followup commits after this commit clean up code using
ParamsStream and make it easier to set multiple parameters.

Currently it is only possible to attach one serialization parameter to a stream
at a time. For example, it is not possible to set a parameter controlling the
transaction format and a parameter controlling the address format at the same
time because one parameter will override the other.

This limitation is inconvenient for multiprocess code since it is not possible
to create just one type of stream and serialize any object to it. Instead it is
necessary to create different streams for different object types, which
requires extra boilerplate and makes using the new parameter fields a lot more
awkward than the older version and type fields.

Fix this problem by allowing an unlimited number of serialization stream
parameters to be set, and allowing them to be requested by type. Later
parameters will still override earlier parameters, but only if they have the
same type.

This change requires replacing the stream.GetParams() method with a
stream.GetParams<T>() method in order for serialization code to retrieve the
desired parameters. This change is more verbose, but probably a good thing for
readability because previously it could be difficult to know what type the
GetParams() method would return, and now it is more obvious.

f3a2b52376

ryanofsky force-pushed on Jan 11, 2024

ryanofsky commented at 11:19 pm on January 11, 2024: contributor

Rebased 97338eec9e35401f42c376062ea161a22143d628 -> ffc1e9fd8cf53cfcf4d7926ca2963958eacf73d7 (pr/iparams.2 -> pr/iparams.3, compare) due to minor conflict with #28451

DrahtBot removed the label Needs rebase on Jan 12, 2024

maflcko commented at 7:19 am on January 12, 2024: member

0test/serialize_tests.cpp:394:11: error: unused variable 'oi2' [-Werror,-Wunused-variable]
1  394 |     Other oi2;
2      |           ^~~

ryanofsky force-pushed on Jan 12, 2024

ryanofsky commented at 12:42 pm on January 12, 2024: contributor

Updated ffc1e9fd8cf53cfcf4d7926ca2963958eacf73d7 -> 3c311734d2fc6a4ca410f254ba3c8e923d58be70 (pr/iparams.3 -> pr/iparams.4, compare) to fix unused variable in test https://cirrus-ci.com/task/4527517751574528?logs=ci#L2260

re: #28929 (comment)

0test/serialize_tests.cpp:394:11: error: unused variable 'oi2' [-Werror,-Wunused-variable]

Fixed, thanks. Also renamed variables in the test to make it more readable

DrahtBot removed the label CI failed on Jan 12, 2024

in src/serialize.h:1114 in 3c311734d2 outdated

1107@@ -1108,10 +1108,15 @@ template <typename SubStream, typename Params>
1108 class ParamsStream
1109 {
1110     const Params& m_params;
1111-    SubStream& m_substream; // private to avoid leaking version/type into serialization code that shouldn't see it
1112+    SubStream m_substream; // private to avoid leaking version/type into serialization code that shouldn't see it
1113 
1114 public:
1115-    ParamsStream(SubStream& substream LIFETIMEBOUND, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{substream} {}
1116+    ParamsStream(SubStream substream LIFETIMEBOUND, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{substream} {}

maflcko commented at 11:34 am on February 9, 2024:

3c311734d2fc6a4ca410f254ba3c8e923d58be70:

I don’t understand this. For example, if SubStream is a DataStream that holds data, this will now create a copy of this data, when previously it didn’t?

It seems odd that the stream has to be passed down the whole “type-stack” anyway. Maybe it is simpler to just create a stack of params only to hold the types?

maflcko commented at 11:56 am on February 9, 2024:

One could consider to completely disallow wrapping streams into each other, see also https://www.github.com/bitcoin/bitcoin/pull/25284/commits/faec591d64e40ba7ec7656cbfdda1a05953bde13#r1315927911

ryanofsky commented at 1:04 pm on February 9, 2024:

re: #28929 (review)

3c31173:

I don’t understand this. For example, if SubStream is a DataStream that holds data, this will now create a copy of this data, when previously it didn’t?

I will add a comment here, but this does not happen due to the “Template deduction guide for a single params argument that’s slightly different from the default generated deduction guide because it stores a reference to the substream inside ParamsStream instead of a copy” on line 1147 below.

With this change, a caller constructing the ParamsStream is in control and can decide whether ParamsStream should hold a value or a reference by specifying DataStream if they want a value, or DataSteam& if they want a reference.

The default behavior for a single parameter is unchanged, and will use a reference. But if there are multiple parameters, the inner ParamsStream instances are stored by value so we have a single tuple-like object containing all the parameters, not multiple ParamsStream objects with unpredictable lifetimes referencing each other.

It seems odd that the stream has to be passed down the whole “type-stack” anyway. Maybe it is simpler to just create a stack of params only to hold the types?

I don’t think it’s odd. Ultimately the ParamsStream wrapper methods (read, write, size, etc) need to call the corresponding methods on the wrapped streams, so types of all the streams need to be known.

One could consider to completely disallow wrapping streams into each other, see also https://www.github.com/bitcoin/bitcoin/pull/25284/commits/faec591d64e40ba7ec7656cbfdda1a05953bde13#r1315927911

That would be a problem because “wrapping a ParamsStream in a ParamsStream” is a useful thing to do, and shouldn’t be forbidden. It’s the most straightforward way to support multiple parameters because each ParamStream can hold one parameter, and ParamsStream::GetParams can return its own parameter if requested, or call the wrapped GetParams method otherwise.

maflcko commented at 1:32 pm on February 9, 2024:

In any case m_substream{substream} still creates a copy when it shouldn’t, due to a missing std::forward, no?

ryanofsky commented at 2:51 pm on February 9, 2024:

re: #28929 (review)

In any case m_substream{substream} still creates a copy when it shouldn’t, due to a missing std::forward, no?

Oh that’s interesting. I wouldn’t think to use std::forward because normally it’s just used for && rvalue reference function template parameters, not class template parameters, and doesn’t do anything useful if && is not used and special template deduction rules for it are not applied.

We are actually ok with copies not moves here in all current cases. Copies and moves in all current cases are identical because ParamStream instances don’t contain any real values, just references to external params and an external substream, so their copy and move methods are the same.

But theoretically if someone did want create a ParamsStream with a nested substream that moved from another instance during construction, probably the code could be changed to support that better, because right now I think you have to write ParamsStream<MySubstream>{std::move(mysubstream), myparams), and wouldn’t be able to shorten it to ParamsStream{std::move(mysubstream), myparams}. Just adding std::forward on this line wouldn’t fix this, but maybe adding std::forward along with switching from & to && in template deduction guides below would work.

I can experiment with this and add a test. I could imagine this being useful if you wanted to write something like ParamsStream mystream{FileStream{"file.bin"}, param1, param2} and have the file automatically closed when the stream was destroyed.

maflcko commented at 2:59 pm on February 9, 2024:

I could imagine this being useful if you wanted to write something like ParamsStream mystream{FileStream{"file.bin"}, param1, param2} and have the file automatically closed when the stream was destroyed.

Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.

0src/net.cpp-    DataStream underlying_stream{vSeedsIn};
1src/net.cpp:    ParamsStream s{underlying_stream, CAddress::V2_NETWORK};

ryanofsky commented at 10:04 pm on February 20, 2024:

re: #28929 (review)

Yes, an example would be in net.cpp, but feel free to ignore, if it is too much hassle.
0src/net.cpp-    DataStream underlying_stream{vSeedsIn};
1src/net.cpp:    ParamsStream s{underlying_stream, CAddress::V2_NETWORK};

Since it didn’t complicate implementation of this PR too much, I implemented support for passing any type of substream as an rvalue to ParamsStream, instead of only passing nested ParamStreams to by value, and I added a test for this. This just required changing & to && and adding std::forward a few places.

Unfortunately, however, I could not find a way to simplify net.cpp without triggering a compiler warning:

 0--- a/src/net.cpp
 1+++ b/src/net.cpp
 2@@ -198,8 +198,7 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
 3     const auto one_week{7 * 24h};
 4     std::vector<CAddress> vSeedsOut;
 5     FastRandomContext rng;
 6-    DataStream underlying_stream{vSeedsIn};
 7-    ParamsStream s{underlying_stream, CAddress::V2_NETWORK};
 8+    ParamsStream s{DataStream{vSeedsIn}, CAddress::V2_NETWORK};
 9     while (!s.eof()) {
10         CService endpoint;
11         s >> endpoint;

This change should work, but unfortunately it triggers a clang -Wdangling warning, which I believe is spurious:

0net.cpp:201:20: error: temporary whose address is used as value of local variable 's' will be destroyed at the end of the full-expression [-Werror,-Wdangling]
1  201 |     ParamsStream s{DataStream{vSeedsIn}, CAddress::V2_NETWORK};
2      |                    ^~~~~~~~~~~~~~~~~~~~

The warning doesn’t make sense and I think it is a compiler bug. It happens for me with clang 17.0.6.

ryanofsky commented at 10:33 pm on February 20, 2024:

re: #28929 (review)

The warning doesn’t make sense and I think it is a compiler bug.

Actually it is not a compiler bug. It happens because of the lifetimebound annotation and goes away if I change:

0-    ParamsStream(SubStream&& substream LIFETIMEBOUND, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{std::forward<SubStream>(substream)} {}
1+    ParamsStream(SubStream&& substream, const Params& params LIFETIMEBOUND) : m_params{params}, m_substream{std::forward<SubStream>(substream)} {}

But that change is potentially less safe, because we really do want the lifetimebound attribute to be specified if Substream is an lvalue reference, just not in other cases. I’m not sure if there is an attribute syntax which would support doing that.

ryanofsky commented at 10:54 pm on February 20, 2024:

re: #28929 (review)

This should be resolved in latest push, which just drops the LIFETIMEBOUND annotation since it isn’t always accurate. The only case where specifying the annotation is actually correct is the case where the argument is an lvalue. And in that case, I think the annotation is probably not useful. The language already doesn’t allow passing temporaries as ~rvalues~ lvalues so there is probably not anything the compiler can helpfully warn about in that case.

maflcko commented at 10:29 am on February 21, 2024:

The language already doesn’t allow passing temporaries as rvalues so there is probably not anything the compiler can helpfully warn about in that case.

I think you meant to say “lvalues”? Also, const lvalues should be fine by the language. For example, the following should cause a use-after-free:

0ParamsStream<const UncopyableStream&,const BaseFormat&> pstream{UncopyableStream {MakeByteSpan(std::string_view{"abc"})}, RAW};
1     BOOST_CHECK_EQUAL(pstream.GetStream().str(), "abc");

Just leaving a comment. Not sure if it is worth it to re-add the LIFETIMEBOUND attribute for this edge case.

ryanofsky commented at 2:05 pm on February 21, 2024:

re: #28929 (review)

I think you meant to say “lvalues”? Also, const lvalues should be fine by the language. For example, the following should cause a use-after-free:

Thanks that was supposed to say lvalues, and that’s a good point, so it’s possible lifetimebound could still help avoid bugs in some cases. I think it should be possible to add back lifetimebound by overloading the constructor using std::is_reference. But it would add a little complexity, so I think it makes sense to omit it for now, and maybe add it later if it looks like there are cases where a const stream would be used.

maflcko commented at 11:38 am on February 9, 2024: member

nit: retreive -> retrieve in the first commit msg

maflcko commented at 11:38 am on February 9, 2024: member

did not review the last commit 3c311734d2fc6a4ca410f254ba3c8e923d58be70 🚔

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: did not review the last commit 3c311734d2fc6a4ca410f254ba3c8e923d58be70 🚔
3HgBPRyZaqsA6IWO/nWpFwiv5/omKRQ3fZmFGkrNfInSb0WBx0SgqbIdQPEkb2NkO/KoxscmLPJRXjk2Ia3ixCA==

ryanofsky force-pushed on Feb 20, 2024

ryanofsky commented at 10:18 pm on February 20, 2024: contributor

Updated 3c311734d2fc6a4ca410f254ba3c8e923d58be70 -> e02ab1577f51a6c9af96b49e29ffaa3918ddae6c (pr/iparams.4 -> pr/iparams.5, compare) using && and std::forward to support rvalue substream arguments and adding tests and documentation

ryanofsky force-pushed on Feb 20, 2024

ryanofsky commented at 10:56 pm on February 20, 2024: contributor

Updated e02ab1577f51a6c9af96b49e29ffaa3918ddae6c -> 33d99bad01637a3454358841b5c1aaf099002afb (pr/iparams.5 -> pr/iparams.6, compare) adding a new commit simplifying usage of ParamsStream in net.cpp, and fixing inaccurate lifetimebound annotations

in src/serialize.h:1110 in af7f90da74 outdated

1102@@ -1103,22 +1103,36 @@ size_t GetSerializeSize(const T& t)
1103     return (SizeComputer() << t).size();
1104 }
1105 
1106-/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
1107-template <typename SubStream, typename Params>
1108+/** Wrapper that overrides the GetParams() function of a stream. */

maflcko commented at 9:53 am on February 21, 2024:

af7f90da74b935c17d986b0f4d21e4bce6f56343: Thanks for removing it, but I think it can be a separate (trivial) 4 line diff commit?

ryanofsky commented at 12:10 pm on February 21, 2024:

re: #28929 (review)

af7f90d: Thanks for removing it, but I think it can be a separate (trivial) 4 line diff commit?

Sure, moved to separate commit

in src/serialize.h:1133 in af7f90da74 outdated

1132     template <typename U> ParamsStream& operator>>(U&& obj) { ::Unserialize(*this, obj); return *this; }
1133     void write(Span<const std::byte> src) { m_substream.write(src); }
1134     void read(Span<std::byte> dst) { m_substream.read(dst); }
1135     void ignore(size_t num) { m_substream.ignore(num); }
1136     bool eof() const { return m_substream.eof(); }
1137     size_t size() const { return m_substream.size(); }

maflcko commented at 10:05 am on February 21, 2024:

nit in https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343: Would it matter if all of these were updated to use GetStream over m_substream? I guess the only difference would be that std::stacktrace::current() would be smaller if called inside of one of the substream methods?

ryanofsky commented at 12:33 pm on February 21, 2024:

re: #28929 (review)

nit in af7f90d: Would it matter if all of these were updated to use GetStream over m_substream? I guess the only difference would be that std::stacktrace::current() would be smaller if called inside of one of the substream methods?

Nice idea. Replaced m_substream with GetStream here. It might make it easier for compiler to inilne these calls functions, too. This required adding a const overload of GetStream.

in src/test/serialize_tests.cpp:425 in af7f90da74 outdated

420+    Base base1{0x20};
421+    pstream << base1;
422+    BOOST_CHECK_EQUAL(pstream.GetStream().str(), "\x20");
423+
424+    Base base2;
425+    pstream >> base2;;

maflcko commented at 10:13 am on February 21, 2024:

;;

ryanofsky commented at 12:24 pm on February 21, 2024:

re: #28929 (review)

;;

Thanks fixed

in src/serialize.h:1162 in af7f90da74 outdated

1159+/**
1160+ * Explicit template deduction guide is required for single-parameter
1161+ * constructor so Substream&& is treated as a forwarding reference, and
1162+ * SubStream is deduced as reference type for lvalue arguments.
1163+ */
1164+template<typename Substream, typename Params>

maflcko commented at 10:14 am on February 21, 2024:

nit: Space after template according to clang-format?

ryanofsky commented at 12:26 pm on February 21, 2024:

re: #28929 (review)

nit: Space after template according to clang-format?

Thanks, fixed

in src/serialize.h:1169 in af7f90da74 outdated

1166+
1167+/**
1168+ * Template deduction guide for multiple params arguments that creates a nested
1169+ * ParamsStream.
1170+ */
1171+template<typename Substream, typename Params1, typename Params2, typename... Params>

maflcko commented at 10:14 am on February 21, 2024:

Same?

ryanofsky commented at 12:26 pm on February 21, 2024:

re: #28929 (review)

Same?

Thanks, fixed

in src/serialize.h:1107 in af7f90da74 outdated

1102@@ -1103,22 +1103,36 @@ size_t GetSerializeSize(const T& t)
1103     return (SizeComputer() << t).size();
1104 }
1105 
1106-/** Wrapper that overrides the GetParams() function of a stream (and hides GetVersion/GetType). */
1107-template <typename SubStream, typename Params>
1108+/** Wrapper that overrides the GetParams() function of a stream. */
1109+template <typename SubStream, typename Params, bool nested = false>

maflcko commented at 10:47 am on February 21, 2024:

nit in https://github.com/bitcoin/bitcoin/commit/af7f90da74b935c17d986b0f4d21e4bce6f56343: I wonder if the nested bool could somehow be derived with a C++20 concept, so that it is true, whenever SubStream::m_substream exists (or whenever SubStream{}.GetStream() exists). This would avoid issues where a dev “manually” nests params-streams and would have to call GetStream for the number of manual nests they did. Using the compiler to deduce the bool makes sure that GetStream would have to be called only ever once to get the most underlying stream.

ryanofsky commented at 12:35 pm on February 21, 2024:

re: #28929 (review)

nit in af7f90d: I wonder if the nested bool could somehow be derived with a C++20 concept […]

I’ll try that out. I haven’t really experimented with concepts much.

ryanofsky commented at 1:34 pm on February 21, 2024:

re: #28929 (review)

I wonder if the nested bool could somehow be derived with a C++20 concept

Nice suggestion, implemented this

in src/serialize.h:1170 in af7f90da74 outdated

1167+/**
1168+ * Template deduction guide for multiple params arguments that creates a nested
1169+ * ParamsStream.
1170+ */
1171+template<typename Substream, typename Params1, typename Params2, typename... Params>
1172+ParamsStream(Substream&& s LIFETIMEBOUND, const Params1& params1 LIFETIMEBOUND, const Params2& params2 LIFETIMEBOUND, const Params&... params LIFETIMEBOUND) ->

maflcko commented at 10:49 am on February 21, 2024:

What is the point of LIFETIMEBOUND in deduction guides? Does it do anything?

ryanofsky commented at 12:23 pm on February 21, 2024:

What is the point of LIFETIMEBOUND in deduction guides? Does it do anything?

Good catch, they can’t do anything, removed these

maflcko approved

maflcko commented at 10:56 am on February 21, 2024: member

left some comments/nits. Feel free to ignore.

very nice. lgtm ACK 33d99bad01 👆

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: very nice. lgtm ACK 33d99bad01 👆
3vYXG8JolRRudeR/EvljO02nNhTpLVh7ae9EjSBP82mgGSRDkUFhYYlnx9S2EOpzD0z1dE4QHOEHx7YYNx8ksAQ==

DrahtBot requested review from dergoegge on Feb 21, 2024

serialization: Drop references to GetVersion/GetType

Drop references to stream GetVersion()/GetType() methods which no longer exist

84502b755b

serialization: Drop unnecessary ParamsStream references

Drop unnecessary ParamsStream references from CTransaction and
CMutableTransaction constructors. This just couples these classes unnecessarily
to the ParamsStream class, making the ParamsStream class harder to modify, and
making the transaction classes in some cases (depending on parameter order)
unable to work with stream classes that have multiple parameters set.

83436d14f0

serialization: Reverse ParamsStream constructor order

Move parameter argument after stream argument so will be possible to accept
multiple variadic parameter arguments in the following commit.

Also reverse template parameter order for consistency.

cb28849a88

serialization: Accept multiple parameters in ParamsStream constructor

Before this change it was possible but awkward to create ParamStream streams
with multiple parameter objects. After this change it is straightforward.

The change to support multiple parameters is implemented by letting
ParamsStream contain substream instances, instead of just references to
external substreams. So a side-effect of this change is that ParamStream can
now accept rvalue stream arguments and be easier to use in some other cases. A
test for rvalues is added in this commit, and some simplifications to non-test
code are made in the next commit.

e6794e475c

net: Simplify ParamsStream usage

Simplify ParamsStream usage in ConvertSeeds now that ParamsStream supports
rvalue substream arguments.

951203bcc4

serialization: Add ParamsStream GetStream() method

Add GetStream() method useful for accessing underlying stream. Use to improve
ParamsStream test coverage.

8d491ae9ec

ryanofsky force-pushed on Feb 21, 2024

ryanofsky commented at 12:59 pm on February 21, 2024: contributor

Updated 33d99bad01637a3454358841b5c1aaf099002afb -> 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a (pr/iparams.6 -> pr/iparams.7, compare) applying various suggestions and splitting commits

maflcko commented at 1:17 pm on February 21, 2024: member

ACK 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a 📇

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a  📇
3l7D2bi74heXcy2zawxMPGghg5EiPAxy6Xi+GITolujanITODEnXLVYYbrun8tyMXi780u9apadIJVoXtoXYaAQ==

ryanofsky force-pushed on Feb 21, 2024

ryanofsky commented at 1:35 pm on February 21, 2024: contributor

Updated 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a -> 40f505583f4edeb2859aeb70da20c6374d331a4f (pr/iparams.7 -> pr/iparams.8, compare) using concept to replace bool nested as suggested

maflcko commented at 1:53 pm on February 21, 2024: member

ACK 40f505583f4edeb2859aeb70da20c6374d331a4f 🎦

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 40f505583f4edeb2859aeb70da20c6374d331a4f  🎦
3jZImqPjGUJwt/833rdSh2DiEjsg6nNxFTHOhUb3GhAl6bvntsIwjLrN4sAtM3nFCw97147fhRhcAtUoKEkNJDg==

sipa commented at 3:58 pm on February 21, 2024: member

utACK 40f505583f4edeb2859aeb70da20c6374d331a4f

sipa closed this on Feb 21, 2024

sipa reopened this on Feb 21, 2024

maflcko added this to the milestone 28.0 on Feb 22, 2024

in src/serialize.h:1159 in 40f505583f outdated

1166+            return m_substream.GetStream();
1167+        } else {
1168+            return m_substream;
1169+        }
1170+    }
1171+    auto& GetStream() const

cbergqvist commented at 12:49 pm on February 28, 2024:

If m_substream is an owned instance, it will become const SubStream in const member functions, and the return value here will be const Substream&. However, if m_substream is a reference/pointer, the constness is applied to the reference part, so the method will return a mutable reference. See https://stackoverflow.com/questions/5055427/returning-non-const-reference-from-a-const-member-function

My suggestion would be to return const auto& here unless the intent is to enable mutability (which only happens when the member is a reference).

Illustrative example

 0class A
 1{
 2    int& b;
 3public:
 4    A(int& n) : b(n) {}
 5    auto& foo() const { return b; }
 6};
 7
 8    ....
 9    int bar = 123;
10    const A a(bar);
11    int& mutating = a.foo();
12    mutating = 2;

ryanofsky commented at 3:20 pm on February 28, 2024:

re: #28929 (review)

My suggestion would be to return const auto& here unless the intent is to enable mutability (which only happens when the member is a reference).

Good catch, added const. This is intended to return a const refernce consistently, not provide mutable access to the substream if the paramsstream is const.

ryanofsky force-pushed on Feb 28, 2024

ryanofsky commented at 3:23 pm on February 28, 2024: contributor

Updated 40f505583f4edeb2859aeb70da20c6374d331a4f -> 8d491ae9ecf1948ea29f67b50ca7259123f602aa (pr/iparams.8 -> pr/iparams.9, compare) just adding const as suggested

maflcko commented at 3:25 pm on February 28, 2024: member

ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa 🔵

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa 🔵
3TqzZNNCRqUT9JcNGY4BCaauByEiHoCa8EyBV4AqQzgBWqs0t3pK98OnLYdtt+8vzsdHbkeivD4o29SOgxiVICA==

DrahtBot requested review from sipa on Feb 28, 2024

DrahtBot added the label CI failed on Feb 28, 2024

DrahtBot removed the label CI failed on Mar 2, 2024

DrahtBot added the label CI failed on Apr 20, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

ryanofsky requested review from theuni on May 14, 2024

ryanofsky commented at 5:51 pm on May 14, 2024: contributor

@theuni any chance you’d be interested in reviewing this? (There are 7 commits so this looks looks like a bigger change, but all the complexity is basically in the first commit, the other commits are small or obvious changes.)

in src/serialize.h:1151 in 8d491ae9ec

1145@@ -1142,6 +1146,24 @@ class ParamsStream
1146             return m_substream.template GetParams<P>();
1147         }
1148     }
1149+
1150+    //! Get reference to underlying stream.
1151+    auto& GetStream()

TheCharlatan commented at 8:40 pm on May 14, 2024:

Are you going to be using this method for other purposes besides tests and as an internal getter?

ryanofsky commented at 9:32 pm on May 14, 2024:

re: #28929 (review)

Are you going to be using this method for other purposes besides tests and as an internal getter?

I could see some other uses. Initially this was just added for testing, but then marco pointed out it could be used as an internal getter in #28929 (review) to avoid recursion in read, write, size, etc methods. The other case it might be useful is if you have have a custom stream type like a file or socket and want to call some method on that stream that ParamsStream() is not wrapping, without having to pass around separate pointers to both streams.

in src/serialize.h:1154 in 8d491ae9ec

1145@@ -1142,6 +1146,24 @@ class ParamsStream
1146             return m_substream.template GetParams<P>();
1147         }
1148     }
1149+
1150+    //! Get reference to underlying stream.
1151+    auto& GetStream()
1152+    {
1153+        if constexpr (ContainsStream<SubStream>) {
1154+            return m_substream.GetStream();

TheCharlatan commented at 8:43 pm on May 14, 2024:

Just a comment: This does indeed not trigger clang-tidy’s recursion check (I’m guessing because of the constexpr condition).

TheCharlatan approved

TheCharlatan commented at 8:49 pm on May 14, 2024: contributor

Nice, reviewing this was fun. ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa

ryanofsky commented at 9:37 pm on May 14, 2024: contributor

Thanks for the review!

ryanofsky commented at 6:15 pm on May 15, 2024: contributor

@sipa, could you re-ack? Only change since your last review was adding a missing const (compare)

sipa commented at 6:27 pm on May 15, 2024: member

utACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa

achow101 merged this on May 15, 2024

achow101 closed this on May 15, 2024

theuni commented at 1:42 am on May 16, 2024: member

@ryanofsky Sorry for not getting to this before merge. Will give it a look tomorrow.

serialization: Support for multiple parameters #28929

Code Coverage

Reviews

Conflicts

Illustrative example