serialization: Support for multiple parameters #28929

ryanofsky · 2023-11-23T01:31:48Z

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 · 2023-11-23T01:31:51Z

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.

src/primitives/transaction.h

ajtowns

Concept ACK

src/protocol.h

dergoegge · 2023-11-24T10:29:35Z

Concept ACK

ryanofsky

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

src/protocol.h

src/primitives/transaction.h

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.

ryanofsky · 2024-01-11T23:19:14Z

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

maflcko · 2024-01-12T07:19:35Z

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

ryanofsky · 2024-01-12T12:42:02Z

Updated ffc1e9f -> 3c31173 (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)

test/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

maflcko · 2024-02-09T11:38:12Z

nit: retreive -> retrieve in the first commit msg

maflcko

did not review the last commit 3c31173 🚔

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: did not review the last commit 3c311734d2fc6a4ca410f254ba3c8e923d58be70 🚔
HgBPRyZaqsA6IWO/nWpFwiv5/omKRQ3fZmFGkrNfInSb0WBx0SgqbIdQPEkb2NkO/KoxscmLPJRXjk2Ia3ixCA==

src/serialize.h

ryanofsky

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

src/serialize.h

ryanofsky

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

src/serialize.h

maflcko

left some comments/nits. Feel free to ignore.

very nice. lgtm ACK 33d99ba 👆

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: very nice. lgtm ACK 33d99bad01 👆
vYXG8JolRRudeR/EvljO02nNhTpLVh7ae9EjSBP82mgGSRDkUFhYYlnx9S2EOpzD0z1dE4QHOEHx7YYNx8ksAQ==

src/serialize.h

src/test/serialize_tests.cpp

src/serialize.h

ryanofsky

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

src/serialize.h

src/test/serialize_tests.cpp

src/serialize.h

maflcko · 2024-02-21T13:17:32Z

ACK 9dc839a 📇

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: ACK 9dc839a7fd1c8c7648f9e0a025d5ebecee8d289a  📇
l7D2bi74heXcy2zawxMPGghg5EiPAxy6Xi+GITolujanITODEnXLVYYbrun8tyMXi780u9apadIJVoXtoXYaAQ==

ryanofsky

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

src/serialize.h

maflcko · 2024-02-21T13:53:44Z

ACK 40f5055 🎦

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: ACK 40f505583f4edeb2859aeb70da20c6374d331a4f  🎦
jZImqPjGUJwt/833rdSh2DiEjsg6nNxFTHOhUb3GhAl6bvntsIwjLrN4sAtM3nFCw97147fhRhcAtUoKEkNJDg==

sipa · 2024-02-21T15:58:19Z

utACK 40f5055

src/serialize.h

ryanofsky

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

src/serialize.h

maflcko · 2024-02-28T15:25:31Z

ACK 8d491ae 🔵

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: ACK 8d491ae9ecf1948ea29f67b50ca7259123f602aa 🔵
TqzZNNCRqUT9JcNGY4BCaauByEiHoCa8EyBV4AqQzgBWqs0t3pK98OnLYdtt+8vzsdHbkeivD4o29SOgxiVICA==

ryanofsky · 2024-05-14T17:51:13Z

@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.)

TheCharlatan

Nice, reviewing this was fun.
ACK 8d491ae

TheCharlatan · 2024-05-14T20:40:36Z

src/serialize.h

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


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

re: #28929 (comment)

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 (comment) 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.

TheCharlatan · 2024-05-14T20:43:09Z

src/serialize.h

+    auto& GetStream()
+    {
+        if constexpr (ContainsStream<SubStream>) {
+            return m_substream.GetStream();


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

ryanofsky

Thanks for the review!

ryanofsky · 2024-05-14T21:32:27Z

src/serialize.h

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


re: #28929 (comment)

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 (comment) 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.

ryanofsky · 2024-05-15T18:15:46Z

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

sipa · 2024-05-15T18:27:36Z

utACK 8d491ae

theuni · 2024-05-16T01:42:36Z

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

This was referenced Nov 23, 2023

Multiprocess tracking issue #28722

Open

Multiprocess bitcoin #10102

Draft

ajtowns reviewed Nov 23, 2023

View reviewed changes

src/primitives/transaction.h Show resolved Hide resolved

ryanofsky mentioned this pull request Nov 23, 2023

Use serialization parameters for CTransaction #28438

Merged

ajtowns reviewed Nov 23, 2023

View reviewed changes

src/protocol.h Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 23, 2023

refactor: Remove unused SER_DISK, SER_NETWORK, CDataStream #28451

Merged

ryanofsky force-pushed the pr/iparams branch from 334f75a to 97338ee Compare November 28, 2023 21:36

ryanofsky commented Nov 28, 2023

View reviewed changes

src/protocol.h Show resolved Hide resolved

src/primitives/transaction.h Show resolved Hide resolved

DrahtBot added CI failed Needs rebase labels Nov 29, 2023

ryanofsky force-pushed the pr/iparams branch from 97338ee to ffc1e9f Compare January 11, 2024 23:17

DrahtBot removed the Needs rebase label Jan 12, 2024

ryanofsky force-pushed the pr/iparams branch from ffc1e9f to 3c31173 Compare January 12, 2024 12:41

DrahtBot removed the CI failed label Jan 12, 2024

ryanofsky mentioned this pull request Feb 8, 2024

multiprocess: Add capnp wrapper for Chain interface #29409

Open

maflcko mentioned this pull request Feb 9, 2024

net: Use serialization parameters for CAddress serialization #25284

Merged

maflcko reviewed Feb 9, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/iparams branch from 3c31173 to e02ab15 Compare February 20, 2024 22:04

ryanofsky commented Feb 20, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/iparams branch from e02ab15 to 33d99ba Compare February 20, 2024 22:55

ryanofsky commented Feb 20, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

maflcko approved these changes Feb 21, 2024

View reviewed changes

DrahtBot requested a review from dergoegge February 21, 2024 10:56

ryanofsky force-pushed the pr/iparams branch from 33d99ba to 9dc839a Compare February 21, 2024 12:58

ryanofsky commented Feb 21, 2024

View reviewed changes

ryanofsky force-pushed the pr/iparams branch from 9dc839a to 40f5055 Compare February 21, 2024 13:33

ryanofsky commented Feb 21, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

sipa closed this Feb 21, 2024

sipa reopened this Feb 21, 2024

maflcko added this to the 28.0 milestone Feb 22, 2024

cbergqvist reviewed Feb 28, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/iparams branch from 40f5055 to 8d491ae Compare February 28, 2024 15:22

ryanofsky commented Feb 28, 2024

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

DrahtBot requested a review from sipa February 28, 2024 15:25

DrahtBot added CI failed and removed CI failed labels Feb 28, 2024

Extheoisah mentioned this pull request Mar 14, 2024

Multiprocess bitcoin Extheoisah/bitcoin#38

Closed

adamjonas mentioned this pull request Mar 14, 2024

Multiprocess bitcoin adamjonas/bitcoin#7

Open

DrahtBot added CI failed and removed CI failed labels Apr 20, 2024

ryanofsky requested a review from theuni May 14, 2024 17:50

TheCharlatan approved these changes May 14, 2024

View reviewed changes

ryanofsky commented May 14, 2024

View reviewed changes

achow101 merged commit 71f0f22 into bitcoin:master May 15, 2024

bitcoin locked and limited conversation to collaborators May 16, 2025

serialization: Support for multiple parameters #28929

serialization: Support for multiple parameters #28929

Uh oh!

Conversation

ryanofsky commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dergoegge commented Nov 24, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Jan 11, 2024

Uh oh!

maflcko commented Jan 12, 2024

Uh oh!

ryanofsky commented Jan 12, 2024

Uh oh!

maflcko commented Feb 9, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Feb 21, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 21, 2024

Uh oh!

sipa commented Feb 21, 2024

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 23, 2023 •

edited

Loading

DrahtBot commented Nov 23, 2023 •

edited

Loading