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

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #19065 (tests: Add fuzzing harness for CAddrMan. Fill some fuzzing coverage gaps. by practicalswift)
• #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.

Concept ACK

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

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

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

8d1b1df474db73d3010324f728cb8b7851036b4f

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)

Rebased after #19486 merge. Addressed comments.

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

Concept ACK.

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

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

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

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.

In order to check that this works as intended and to understand it better I wrote some unit tests that exercise it.

Some observations, correct me if I am wrong:

• I think it is impossible to use two separate methods Serialize() and Unserialize() with this mechanism and one has to use:

SERIALIZE_METHODS_PARAMS(...)
{
    if (ser_action.ForRead()) {
        ... all unserialize goes here ...
    } else {
        ... all serialize goes here ...
    }
}

• If there are two classes Base and Derived (the second one derived from the first), both using this mechanism, then for the derived class one has to create a formatter config that specifies how to format both Base and Derived:

Derived derived;
BaseAndDerivedFormat fmt;

fmt.m_base_format = ...
fmt.m_derived_format = ...

stream << WithParams(fmt, derived);

// and then in `Derived`:

SERIALIZE_METHODS_PARAMS(Derived, obj, DerivedAndBaseFormat, fmt)
{
    READWRITE(WithParams(fmt.m_base_format, AsBase<Base>(obj)));

    ... ser/unser this Derived object according to fmt.m_derived_format ...
}

<details> <summary>unit tests, added to src/test/serialize_tests.cpp</summary>

#include <util/strencodings.h>

#include <algorithm>
#include <ios>
#include <string>

enum class BaseFormat {
    DEC,
    HEX,
};

/// (Un)serialize a number as a string either from/to decimal or hexadecimal.
class Base
{
public:
    uint8_t m_base_data;

    Base() : m_base_data(17) {}
    explicit Base(uint8_t data) : m_base_data(data) {}

    SERIALIZE_METHODS_PARAMS(Base, obj, BaseFormat, fmt)
    {
        if (ser_action.ForRead()) {
            std::string str;
            s >> str;
            const uint8_t data = (uint8_t)std::stoul(str, 0, fmt == BaseFormat::DEC ? 10 : 16);
            SER_READ(obj, obj.m_base_data = data);
        } else {
            s << strprintf(fmt == BaseFormat::DEC ? "%02hhu" : "%02hhX", obj.m_base_data);
        }
    }
};

class DerivedAndBaseFormat
{
public:
    BaseFormat m_base_format;

    enum class DerivedFormat {
        LOWER,
        UPPER,
    } m_derived_format;
};

class Derived : public Base
{
public:
    std::string m_derived_data;

    SERIALIZE_METHODS_PARAMS(Derived, obj, DerivedAndBaseFormat, fmt)
    {
        READWRITE(WithParams(fmt.m_base_format, AsBase<Base>(obj)));

        if (ser_action.ForRead()) {
            std::string str;
            s >> str;
            SER_READ(obj, obj.m_derived_data = str);
        } else {
            s << (fmt.m_derived_format == DerivedAndBaseFormat::DerivedFormat::LOWER ?
                      ToLower(obj.m_derived_data) :
                      ToUpper(obj.m_derived_data));
        }
    }
};

BOOST_AUTO_TEST_CASE(with_params_base)
{
    Base b{15};

    CDataStream stream(SER_DISK, PROTOCOL_VERSION);

    stream << WithParams(BaseFormat::DEC, b);
    BOOST_CHECK_EQUAL(stream.str(), "\2" "15");

    b.m_base_data = 0;
    stream >> WithParams(BaseFormat::DEC, b);
    BOOST_CHECK_EQUAL(b.m_base_data, 15);

    stream.clear();

    stream << WithParams(BaseFormat::HEX, b);
    BOOST_CHECK_EQUAL(stream.str(), "\2" "0F");

    b.m_base_data = 0;
    stream >> WithParams(BaseFormat::HEX, b);
    BOOST_CHECK_EQUAL(b.m_base_data, 0x0F);
}

BOOST_AUTO_TEST_CASE(with_params_vector_of_base)
{
    std::vector<Base> v{Base{15}, Base{255}};

    CDataStream stream(SER_DISK, PROTOCOL_VERSION);

    stream << WithParams(BaseFormat::DEC, v);
    BOOST_CHECK_EQUAL(stream.str(), "\2\2" "15" "\3" "255");

    v[0].m_base_data = 0;
    v[1].m_base_data = 0;
    stream >> WithParams(BaseFormat::DEC, v);
    BOOST_CHECK_EQUAL(v[0].m_base_data, 15);
    BOOST_CHECK_EQUAL(v[1].m_base_data, 255);

    stream.clear();

    stream << WithParams(BaseFormat::HEX, v);
    BOOST_CHECK_EQUAL(stream.str(), "\2\2" "0F" "\2" "FF");

    v[0].m_base_data = 0;
    v[1].m_base_data = 0;
    stream >> WithParams(BaseFormat::HEX, v);
    BOOST_CHECK_EQUAL(v[0].m_base_data, 0x0F);
    BOOST_CHECK_EQUAL(v[1].m_base_data, 0xFF);
}

BOOST_AUTO_TEST_CASE(with_params_derived)
{
    Derived d;
    d.m_base_data = 15;
    d.m_derived_data = "xY";

    DerivedAndBaseFormat fmt;

    CDataStream stream(SER_DISK, PROTOCOL_VERSION);

    fmt.m_base_format = BaseFormat::DEC;
    fmt.m_derived_format = DerivedAndBaseFormat::DerivedFormat::LOWER;
    stream << WithParams(fmt, d);

    fmt.m_base_format = BaseFormat::HEX;
    fmt.m_derived_format = DerivedAndBaseFormat::DerivedFormat::UPPER;
    stream << WithParams(fmt, d);

    BOOST_CHECK_EQUAL(stream.str(), "\2" "15" "\2" "xy" "\2" "0F" "\2" "XY");
}

</details>

@vasild:

• It should be possible to use separate Serialize and Unserialize member functions. Do you have a specific example that does not work?

• This should not work in general:

SERIALIZE_METHODS_PARAMS(...)
{
    if (ser_action.ForRead()) {
        ... all unserialize goes here ...
    } else {
        ... all serialize goes here ...
    }
}

because *this will be const during serialization, which means that the "ForRead" branch would not compile in the Serialize version. You can use SER_READ or SER_WRITE though, to specify statements that are only executed during reading or writing (in a type-safe way).

• For Base/Derived classes, an alternative is having DerivedParams inherit from BaseParams (it can't be an enum in that case, though), or otherwise have a conversion operator from one to the other. In that case you don't need WithParams to transmute the parameters.

Will review when travis is green

@MarcoFalke I've been pushing fixes, and GitHub's UI doesn't seem to be picking them up...

https://github.com/sipa/bitcoin/tree/202007_ser_options has commits that aren't here.

No idea what's going on. Since this has no reviews, you could try rebasing once more on master and force push? :man_shrugging:

Ah it was just behind; it picked them up now.

Travis still failing in the fuzzer. Also, needs rebase after #19360 (review)

I think this requires a change to netaddress.h now that #19360 has been merged: https://github.com/bitcoin/bitcoin/pull/19360/files#diff-76d15e11a95be7a4aee1eb89de6098caR165

<!--cf906140f33d8803c4a75a2196329ecb-->

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

Alright, so this:

    SERIALIZE_METHODS_PARAMS(Base, obj, BaseFormat, fmt)
    {
        if (ser_action.ForRead()) {
            std::string str;
            s >> str;
            const int base = fmt == BaseFormat::DEC ? 10 : 16;
            const uint8_t data = (uint8_t)std::stoul(str, 0, base);
            SER_READ(obj, obj.m_base_data = data);
        } else {
            s << strprintf(fmt == BaseFormat::DEC ? "%02hhu" : "%02hhX", obj.m_base_data);
        }
    }

is equivalent to this:

    template <typename Stream>
    void Serialize(Stream& s) const
    {
        s << strprintf(s.GetParams() == BaseFormat::DEC ? "%02hhu" : "%02hhX", m_base_data);
    }

    template <typename Stream>
    void Unserialize(Stream& s)
    {
        std::string str;
        s >> str;
        const int base = s.GetParams() == BaseFormat::DEC ? 10 : 16;
        m_base_data = (uint8_t)std::stoul(str, 0, base);
    }

I pushed the unit tests that exercise this feature at https://github.com/vasild/bitcoin/commit/8d89c1d43, feel free to pick (some of) them into this PR if you find them interesting, or if they may serve as an example of how to use this.

ACK fdf66d325

Just some nits below and another nit at #19503 (review)

Rebased, added a few more comments, renamed AddrSerialization to CAddress::Format, and included @vasild's unit test.

ACK 483ed4574

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

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.

Code review ACK 483ed457411d164ad71115dbfe1d2b4ff8b2e5f3.

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 106d8039b61ff4b771dc996e3ab9156019ba4470 branch) seems to work (only ran tests locally).

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

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

<details><summary>Show signature and timestamp</summary>

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 -

</details>

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
  *

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 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".}

Still needs rebase (trivial)

Needs rebase.

This would resolve #19477.

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

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

<!--13523179cfe9479db18ec6c5d236f789-->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.

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

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

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