Serialization-deserialization roundtrip of CPubKey does not necessarily result in an equal object #17238

When fuzzing the serialization code for CPubKey I noticed that Deserialize<CPubKey>(Serialize(obj)) == obj does not hold true for all CPubKey obj.

Is there any reason to why CPubKey is deviating from the other serializable classes (for which equality is defined) in this regard? :)

Context:

template <typename T>
CDataStream Serialize(const T& obj)
{
    CDataStream ds(SER_NETWORK, INIT_PROTO_VERSION);
    ds << obj;
    return ds;
}

template <typename T>
T Deserialize(CDataStream ds)
{
    T obj;
    ds >> obj;
    return obj;
}

What are the steps to reproduce? Be reminded that invalid public keys can not be deserialized: https://dev.visucore.com/bitcoin/doxygen/pubkey_8h_source.html#l00140

@MarcoFalke 121272ca0172 can be used as input to pub_key_deserialize in #17225 to reproduce. Make sure to remove the comment from the following line:

// AssertEqualAfterSerializeDeserialize(pub_key);

:)

I might be blind, but that clearly doesn't look like a valid public key

@MarcoFalke

Nothing wrong with being blind (based on my experience), but perhaps a premature close in this case? :)

Don't forget that the deserialisation harnesses use the first four bytes (121272ca in this case) to read the CDataStream version int. Thus the 0172 part is the CPubKey. It deserialises without error (no std::ios_base::failure is thrown).

Example:

CPubKey pk1;
CDataStream s1{std::vector<uint8_t>{'\x01', '\x72'}, SER_NETWORK, INIT_PROTO_VERSION};
s1 >> pk1;

CDataStream s2{SER_NETWORK, INIT_PROTO_VERSION};
s2 << pk1;

CPubKey pk2;
s2 >> pk2;

assert(pk1 == pk2);

This results in:

Assertion `pk1 == pk2' failed.

Is there any reason to why CPubKey is deviating from the other serializable classes (for which equality is defined) in this regard? :)

@MarcoFalke

Thanks for re-opening. Did my example clarify? Are you following my line of reasoning, or am I missing something obvious?

I didn't realize the nVersion was also serialized. Will take another look later.

Equality is not defined for invalid CPubKeys.

There is no good reason why it isn't (except a likely negligiable performance gain perhaps) but also no good reason why it should be, as nothing ever happens with those apart from signalling "no pubkey here".

I'm likely missing something but shouldn't Set(…) and Unserialize(…) match in their invalidation logic?

If I'm reading it correctly Set(…) is a bit more strict when it comes to invalidation (via Invalidate()) compared to Unserialize(…).

This is the current Set(…):

    //! Initialize a public key using begin/end iterators to byte data.
    template <typename T>
    void Set(const T pbegin, const T pend)
    {
        int len = pend == pbegin ? 0 : GetLen(pbegin[0]);
        if (len && len == (pend - pbegin))
            memcpy(vch, (unsigned char*)&pbegin[0], len);
        else
            Invalidate();
    }

This is the current Unserialize(…):

    template <typename Stream>
    void Unserialize(Stream& s)
    {
        unsigned int len = ::ReadCompactSize(s);
        if (len <= PUBLIC_KEY_SIZE) {
            s.read((char*)vch, len);
        } else {
            // invalid pubkey, skip available data
            char dummy;
            while (len--)
                s.read(&dummy, 1);
            Invalidate();
        }
    }

This is the Unserialize(…) I would naïvely assume given how Set(…) works:

    template <typename Stream>
    void Unserialize(Stream& s)
    {
        unsigned int len = ::ReadCompactSize(s);
        if (len == 0) {
            Invalidate();
        } else if (len <= PUBLIC_KEY_SIZE) {
            s.read((char*)vch, len);
            if (GetLen(vch[0]) != len) {
                Invalidate();
            }
        } else {
            // invalid pubkey, skip available data
            char dummy;
            while (len--)
                s.read(&dummy, 1);
            Invalidate();
        }
    }

The more strict Unserialize(…) would give me the expected equivalence after round-trip :)

That is Deserialize<CPubKey>(Serialize(obj)) == obj in the general case, or in this specific case ...

CPubKey pk1;
CDataStream s1{std::vector<uint8_t>{'\x01', '\x72'}, SER_NETWORK, INIT_PROTO_VERSION};
s1 >> pk1;

CDataStream s2{SER_NETWORK, INIT_PROTO_VERSION};
s2 << pk1;

CPubKey pk2;
s2 >> pk2;

assert(pk1 == pk2);

Is there a reason for Unserialize(…) not calling Invalidate() in the cases where Set(…) would?

It is only used in the wallet. I guess it doesn't hurt to be a bit more strict on verification, but I doubt that it is going to solve a real issue.

@MarcoFalke

Just to clarify: I don't think we have a bug here :) At most it seems to be just a small inconsistency.

I guess that this inconsistency doesn't matter much (or at all) except from a fuzzing perspective. From a fuzzing perspective it very nice to have a set of general (non-class dependent) invariants we can check.

These are the naïve assumptions I have been making:

• std::ios_base::failure is the only exception expected to be thrown when serialising or deserialising -- judging from my fuzzing results this seems to hold for all classes
• Deserialize<T>(Serialize(obj)) == obj for all T obj (assuming T::operator== and that std::ios_base::failure is not thrown during Deserialize/Serialize) -- judging from my fuzzing results this seems to hold for all classes with the exception of CPubKey :)

Do these sound reasonable? More invariants worth checking? :)

Without code changes, what you could check for is (!a.IsValid() && !b.IsValid()) || (a == b).

This one could be marked "good first issue" :)

@practicalswift "good first issue" is only for issues where one (and only one) uncontroversial solution exists.

I felt that @sipa's suggested definition for equality was uncontroversial, but I might have been mistaken :) I don't feel strongly about this at all: closing to keep the number of issues down.

Re-opening: #19237 will fix this :)

Fixed by @elichai in #19237 and regression fuzzed by @theStack in #22271.

Nice teamwork friends! :)