Add parameter feature to serialization and use it for CAddress #19503

sipa · 2020-07-13T04:40:34Z

This introduces a concept of "serialization parameters". Every serializer can define its own parameter type, and a value of that type must be provided when serializing/deserializing, e.g. using ss << WithParams(parameter, value), causing value to be serialized to ss, with parameter parameter. Note these parameters are automatically passed down the stack, so for example a vector can be serialized with a parameter, which will be available to all the elements' serializers.

In this PR, the feature is only used for CAddress serialization, but I plan to also convert SERIALIZE_TRANSACTION_NO_WITNESS in a follow-up. Eventually I hope to get rid of all GetVersion/GetType and replace it with this approach, which is type-safe (code won't compile if the correct parameter isn't passed somewhere), and avoids collisions between flags.

DrahtBot · 2020-07-13T05:51:54Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

tests: Add fuzzing harness for CAddrMan #19065 (tests: Add fuzzing harness for CAddrMan. Fill some fuzzing coverage gaps. by practicalswift)
Add address-based index (attempt 4?) #14053 (Add address-based index (attempt 4?) by marcinja)

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.

practicalswift · 2020-07-13T08:17:04Z

Concept ACK

Getting rid of GetVersion/GetType and replacing them with a compile-time checked approach would be very nice.

jnewbery

Concept ACK. Can be rebased on master now that #19486 is merged to drop the first two commits.

src/protocol.h

maflcko

Concept ACK, no idea why it is not compatible with the live network

8d1b1df474db73d3010324f728cb8b7851036b4f

src/protocol.h

dongcarl · 2020-07-13T19:56:18Z

Concept ACK as it will allow for a previously hacky solution to be neater. See #19477 for more details

Perhaps a worthwhile followup: Completing the example documentations of our serialization "helpers" (seems like there's already a few examples, but not all of them are documented)

sipa · 2020-07-13T20:18:47Z

Rebased after #19486 merge. Addressed comments.

src/serialize.h

maflcko · 2020-07-14T07:28:04Z

Looks like the fuzzers don't compile on some of the commits

naumenkogs · 2020-07-14T07:55:03Z

Concept ACK.

sipa · 2020-07-14T17:32:46Z

@dongcarl Agree - I've added more comments to explain the feature for now, but a more comprehensive overview of all the things the serialization framework supports would be a nice further improvement.

@MarcoFalke Hopefully fixed. I also added all variants of deserialization to the fuzzers.

vasild · 2020-07-15T09:35:36Z

Code coverage report for this PR from unit + functional tests (lines not modified by this PR are dimmed and files not modified by this PR are omitted from the report):

report
links to modified and not covered lines
just net_processing.cpp:3942 is "interesting" (it is modified by this PR and not covered by tests).

jnewbery · 2020-07-15T09:48:08Z

just net_processing.cpp:3942 is "interesting" (it is modified by this PR and not covered by tests).

That line is pretty much a no-op and could be removed. It's ensuring that we never send more than 1000 CAddress records in a single ADDR message. However, the ADDR message is populated from vAddrToSend, which itself is limited to 1000 entries (see the PushAddress() function). So we could just remove that entire if block (and potentially replace it with an assert(vAddr.size() <= 1000) after the for loop).

jnewbery

concept ACK and code review ACK of the changes in protocol.h. The syntax/interface for WithParams() and SERIALIZE_METHODS_PARAMS seems reasonable, but I find it very difficult to understand the template code in serialize.h so I wasn't able to give it a thorough review.

src/serialize.h

src/protocol.h

vasild

ACK 483ed45

Thanks for adding input validation to the hex/dec examples in the unit tests!

jonatack

ACK 483ed45 code review over several sessions this week, verified the build at each commit, ran the 3 address deserialization fuzz tests as a sanity check. Thanks for the unit test coverage -- a good addition and demonstrates use of the feature. Edit: am running a node with this branch.

ryanofsky

Code review ACK 483ed45.

This seems all right, but I think it's significantly more complex than it needs to because it is making serialization with params something that can be done in conjunction with using a formatter, instead of just a special case of formatting. Simplification along lines of 106d803 branch) seems to work (only ran tests locally).

src/protocol.h

ryanofsky · 2020-07-22T23:02:12Z

src/test/serialize_tests.cpp

+            uint32_t data;
+            bool ok;
+            if (fmt == BaseFormat::DEC) {
+                ok = ParseUint32(str, &data);


In commit "test: add tests that exercise WithParams()" (483ed45)

Think this needs to capitalize Int: ParseUInt32

sipa · 2020-07-22T23:45:07Z

@ryanofsky That approach works and is a lot simpler, but I think it misses the "automatic recursion" property I was hoping for, that you could e.g. serialize a CBlock with specified CTransaction parameters, and it would transparently pass those parameters down. Or a block could define its own type that can be converted to the CTransaction parameters type, but not need to anything beyond that.

Whether that's a desirable depends how much you treat a higher-level structure as openly containing the lower-level structure, but I think in general most of our complex serializable data structures are like that.

maflcko · 2020-08-04T13:07:26Z

re-ACK 483ed45, though I did not review the serialize.h changes 🚤

Changes since last review:

Rebase
Added comments/doc
Fix "off-by-one" issue/typo bug in commit "12a5a3c032 Disentangle disk address version from client version"
Properly deserialize addrFrom without time (Bugfix in commit "21446c5b5d Use serialization parameters for CAddress serialization")
Fix and extend fuzzers
Add tests from vasild

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 483ed457411d164ad71115dbfe1d2b4ff8b2e5f3, though I did not review the serialize.h changes 🚤

Changes since last review:
* Rebase
* Added comments/doc
* Fix "off-by-one" issue/typo bug in commit "12a5a3c032 Disentangle disk address version from client version"
* Properly deserialize addrFrom without time (Bugfix in commit "21446c5b5d Use serialization parameters for CAddress serialization")
* Fix and extend fuzzers
* Add tests from vasild
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiZTQwAoa37uqI1WTtHnkBHX/T8VKa36Ba6GzwdKgCSLVl7uMr0qw5jL/UrHuG+
tHm4W6GH6fuxYSblm+r7ujigrAkD4/3gPsbOG5mHaYTDOtiL9/ZTHT4HT/nbyZBL
KmaVaLHihfZy8NS05IG8y4sB97ZWXmiAak4An6NdfmjtB5ahWVH5HuLxLERIM7nb
yLjW/5GVbnnMvhirK/nyMcAAXSlTY5twmjT2LYWQ0TnJnNF0djY0VMkvNF8RrvgZ
k3zVnE0J+MW5mv2KcKRCYouGXc+WK777s5gc7bVOfP0J0So9i20LP1pFGqdR+FhV
0WF0aO27EJVePxev36zdaF4CuEFadP99NN5fKnY6rUwpd7+3hIRWZOH2958bbaSH
3oE+M5tLKd+qsAIZ+Lz7W3XU/oK9DE3v8YRasuLzgAFXoiZVkYnISWp/N+FZv+/6
rhAFSyF45HGFkEAa40KdzPV/aT+LWi9UnZe+wa1RtOoiM25jDMTbyLBJiJNz3EoT
vjuegm6r
=lpi2
-----END PGP SIGNATURE-----

Timestamp of file with hash c6299c40a36d62d1a32997018080b37d4feeef88382e17059f749fec43043e26 -

maflcko · 2020-08-04T13:08:33Z

Note to myself, if this gets merged: It would be good to remove the now unused protocol version includes. I.e.

commit 7ffff5b3237fa418637d30ddb59534ab47d3f610 (HEAD)
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Tue Aug 4 14:58:25 2020 +0200

    Remove unused protocol version header from protocol.h

diff --git a/src/bench/rpc_blockchain.cpp b/src/bench/rpc_blockchain.cpp
index 4b45264a3c..09bfcb60a5 100644
--- a/src/bench/rpc_blockchain.cpp
+++ b/src/bench/rpc_blockchain.cpp
@@ -8,6 +8,7 @@
 #include <rpc/blockchain.h>
 #include <streams.h>
 #include <validation.h>
+#include <version.h> // For PROTOCOL_VERSION
 
 #include <univalue.h>
 
diff --git a/src/protocol.h b/src/protocol.h
index a564508f89..58669e20b5 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -14,7 +14,6 @@
 #include <primitives/transaction.h>
 #include <serialize.h>
 #include <uint256.h>
-#include <version.h>
 
 #include <stdint.h>
 #include <string>

and

commit 4550d37e268e7ffe34b3f5ea11d6b3f231d9065a
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Tue Aug 4 12:56:55 2020 +0200

    Remove unused protocol version header from undo.h

diff --git a/src/undo.h b/src/undo.h
index a98f046735..1fb9ac0688 100644
--- a/src/undo.h
+++ b/src/undo.h
@@ -11,7 +11,6 @@
 #include <consensus/consensus.h>
 #include <primitives/transaction.h>
 #include <serialize.h>
-#include <version.h>
 
 /** Formatter for undo information for a CTxIn
  *

DrahtBot · 2020-08-17T11:55:18Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

maflcko

Still needs rebase (trivial)

maflcko · 2020-08-27T15:57:21Z

src/protocol.h

    {
        SER_READ(obj, obj.nTime = TIME_INIT);
-        int nVersion = s.GetVersion();


One include can now be removed, see #19503 (comment)

jonatack · 2020-09-11T10:03:22Z

Needs rebase.

vasild · 2020-10-22T08:30:59Z

This would resolve #19477.

Also, ADDRV2_FORMAT could be replaced with the technique introduced in this PR.

maflcko · 2020-11-25T14:21:18Z

Could also use LIFETIMEBOUND as per #19387 (comment)?

DrahtBot · 2021-12-15T11:21:38Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

maflcko · 2021-12-15T12:19:35Z

@sipa Let us know if someone should pick this up. I might take a stab at it.

sipa · 2021-12-19T15:59:25Z

@MarcoFalke Not going to work on this for the time being.

fanquake · 2022-08-08T08:46:44Z

Removing up for grabs as this has mostly been picked up. i.e #25284.

fanquake added the P2P label Jul 13, 2020

sipa mentioned this pull request Jul 13, 2020

Add "flags" capability to streams #19477

Closed

sipa requested a review from ryanofsky July 13, 2020 04:43

DrahtBot mentioned this pull request Jul 13, 2020

Remove unused constants CADDR_TIME_VERSION and GETHEADERS_VERSION #19486

Merged

DrahtBot added the Needs rebase label Jul 13, 2020

jnewbery reviewed Jul 13, 2020

View reviewed changes

src/protocol.h Outdated Show resolved Hide resolved

maflcko reviewed Jul 13, 2020

View reviewed changes

src/protocol.h Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

sipa force-pushed the 202007_ser_options branch from 2149a5f to f00db5c Compare July 13, 2020 20:12

vasild reviewed Jul 13, 2020

View reviewed changes

src/serialize.h Show resolved Hide resolved

src/serialize.h Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Jul 13, 2020

This was referenced Jul 14, 2020

refactor: replace CConnman pointers by references in net_processing.cpp #19174

Merged

Implement ADDRv2 support (part of BIP155) #19031

Closed

DrahtBot mentioned this pull request Jul 14, 2020

p2p: Unify Send and Receive protocol versions #17785

Merged

sipa force-pushed the 202007_ser_options branch 2 times, most recently from 9ee7bd0 to f58fca6 Compare July 14, 2020 17:27

sipa force-pushed the 202007_ser_options branch 2 times, most recently from 15ba614 to 3d3b095 Compare July 14, 2020 19:29

sipa mentioned this pull request Jul 14, 2020

span: update constructors to match c++20 draft spec and add lifetimebound attribute #19387

Merged

DrahtBot mentioned this pull request Jul 15, 2020

tests: Add fuzzing harness for CAddrMan #19065

Merged

DrahtBot mentioned this pull request Jul 15, 2020

Add address-based index (attempt 4?) #14053

Closed

jnewbery reviewed Jul 15, 2020

View reviewed changes

src/serialize.h Outdated Show resolved Hide resolved

src/protocol.h Show resolved Hide resolved

src/protocol.h Show resolved Hide resolved

src/protocol.h Outdated Show resolved Hide resolved

sipa force-pushed the 202007_ser_options branch from 1f4ad39 to 68822be Compare July 16, 2020 23:55

test: add tests that exercise WithParams()

483ed45

sipa force-pushed the 202007_ser_options branch from 68822be to 483ed45 Compare July 17, 2020 00:16

fanquake removed the Needs rebase label Jul 17, 2020

DrahtBot mentioned this pull request Jul 17, 2020

Revert "refactor: replace CConnman pointers by references in net_processing.cpp" #19542

Closed

vasild approved these changes Jul 17, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 22, 2020

p2p, refactor: Do not over-reserve vAddr capacity #19567

Closed

jonatack reviewed Jul 22, 2020

View reviewed changes

ryanofsky approved these changes Jul 22, 2020

View reviewed changes

DrahtBot added the Needs rebase label Aug 17, 2020

maflcko reviewed Aug 27, 2020

View reviewed changes

vasild mentioned this pull request Sep 1, 2020

net: CNetAddr: add support to (un)serialize as ADDRv2 #19845

Merged

vasild mentioned this pull request Oct 5, 2020

Complete the BIP155 implementation and upgrade to TORv3 #19954

Merged

sipa closed this Dec 19, 2021

sipa added the Up for grabs label Dec 19, 2021

maflcko mentioned this pull request Jun 1, 2022

consensus: move version.h into consensus/ #25260

Closed

fanquake removed the Up for grabs label Aug 8, 2022

jonatack mentioned this pull request Aug 4, 2023

net: Use serialization parameters for CAddress serialization #25284

Merged

bitcoin locked and limited conversation to collaborators Aug 8, 2023

Add parameter feature to serialization and use it for CAddress #19503

Add parameter feature to serialization and use it for CAddress #19503

Uh oh!

Conversation

sipa commented Jul 13, 2020 • edited by jnewbery Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Jul 13, 2020

Uh oh!

jnewbery 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!

dongcarl commented Jul 13, 2020

Uh oh!

sipa commented Jul 13, 2020

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jul 14, 2020

Uh oh!

naumenkogs commented Jul 14, 2020

Uh oh!

sipa commented Jul 14, 2020

Uh oh!

vasild commented Jul 15, 2020

Uh oh!

jnewbery commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 4, 2020

Uh oh!

maflcko commented Aug 4, 2020

Uh oh!

DrahtBot commented Aug 17, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 11, 2020

Uh oh!

vasild commented Oct 22, 2020

Uh oh!

sipa commented Jul 13, 2020 •

edited by jnewbery

Loading

DrahtBot commented Jul 13, 2020 •

edited

Loading

jnewbery commented Jul 15, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

sipa commented Jul 22, 2020 •

edited

Loading