It's kinda a shame to carry this default param forward. But it's also not much better to have a bool at each callsite. An enum class {witness/no_witness} would remove all ambiguity at the callsites, but that may be overkill.

That argument is only needed to support -rpcserialversion, which is deprecated as of #28448, so presumably can/will be pulled out later.

Same comment about default bool.

Nice, this code is much more straightforward now.

Beautiful :)

I guess this makes more sense than defining witness-ness for an empty block, but just to clarify...

This is a no-op and would've been a no-op independent of this PR as well, right?

Yes -- serializing an empty vector is just WriteCompactSize(s, 0) no matter the elements that would have been in the vector, but to serialize a vector of blocks (even an empty one) you need to decide what to do about witness data (even though there is no witness data in an empty block), whereas you don't need to do that for block headers.

If version is no longer used for fileout in these ops, can't we s/CAutoFile/AutoFile/g ?

Little bit of a rabbit hole to do that -- need to change BufferedFile to accept an AutoFile first etc. Doesn't seem worth adding here to me. See https://github.com/ajtowns/bitcoin/commits/202311-autofile

Again, so much more clear. Nice.

I think we could probably drop version.h here now?

Edit: Oooh, we can drop it from some deep internal headers even (hash.h and consensus/validation.h). See theuni@9986da9 . I think I'd prefer to see that added to this PR so that we don't put it off.

Edit2: Updated version here as I missed undo.h the first time: theuni@3762e58

Might be better to do that after dropping version from GetSerializeSize ? see #28438 (comment)

See #28878. Does that seem okay?

These can be a DataStreams now?

None of the psbt bits can switch to DataStream until CVectorWriter is updated.

I realize this isn't new, but it's pretty icky to go to gArgs for something as low-level as serialization. Makes me wonder if we're doing that in any tight loops. Nothing to do with this PR, but it be nice to cache this in a local bool.

This will also go away before the next release, see #28438 (comment)

I'm wondering if TX_WITH_WITNESS calls should be required and specified everywhere (107 places)

If you write stream << object, it seems like it would make sense for the object to be fully serialized by default, with an option to partially serialize it. I'm asking in the context of #28921 / #10102, because before this PR, CTransaction objects could be serialized normally like other objects, and now they require a TX_WITH_WITNESS wrapper to have the same behavior. If TX_WITH_WITNESS were just the default serialization behavior, I wouldn't need a special case for CTransaction objects in multiprocess code, and maybe other code could be simplified too. Does this make sense?

The approach we've got looks like:

    dumb_serializer << SER_PARAMS(object)
    // file, CTransaction

which then gets translated to:

    dumb_serializer << smart_object
    // file, ParamsWrapper<TransactionSerParams, CTransaction>

then

    smart_serializer.Serialize(object, SER_PARAMS)
    // ParamsStream<TransactionSerParams, dumb_serializer>, CTransaction

The advantage of saying TX_WITH_WITNESS everywhere is that way we're being explicit about how objects are serialised to dumb streams everywhere, so we get compile errors if we're ever potentially inconsistent.

But I don't think that makes sense for multiprocess: we don't need to be using dumb streams in the first place there because the whole point is to pass objects between our own processes, no? So afaics, we should be using a smart serializer from the start; ie one that provides a GetParams() that returns something that can be be converted to TransactionSerParams. So doing ParamsStream ss{TX_WITH_WITNESS, stream} in CustomBuildField and CustomReadField would be an easy way to to start?

If you want to pass around both CNetAddr and CAddress objects as well, I think this would work:

// serialize.h
template<typename... SerParams>
class MultiSerParam
{
private:
    const std::tuple<SerParams...> m_params;
public:
    constexpr MultiSerParam(const SerParams&... t) : m_params{t...} { }

    template<typename T>
    constexpr operator const T&() const { return std::get<T>(m_params); }
};

// ipc/capnp/common-types.h ?
static constexpr MultiSerParam INTERNAL_SER_PARAMS(
  TX_WITH_WITNESS,
  CNetAddr::V2,
  CAddress::V2_NETWORK
);

void f(const CNetAddr& netaddr, const CAddress& addr, const CTransaction& tx)
{
    DataStream substream;
    ParamsStream ss{INTERNAL_SER_PARAMS, substream};
    ss << netaddr;
    ss << tx;
    ss << addr;
}

Note that making this actuall compile also requires tweaking CNetAddr like so:

-        if (s.GetParams().enc == Encoding::V2) {
+        CNetAddr::SerParams p(s.GetParams());
+        if (p.enc == Encoding::V2) {

Thanks! That approach of being able to combine different params is clever and solves my problem, avoiding boilerplate from having to create different streams for each object type.

Maybe at some point it would be useful to support default parameter values, too, so parameters like CNetAddr::V2 and CAddress::V2_NETWORK could be assumed if not specified. For some serialization parameters it may be better to be explicit, but for others there could be downsides. For example if I hardcode V2 in multiprocess code and a V3 serialization is added later it could cause subtle bugs if V3 data is silently lost in IPC calls. But there could be other solutions to this problem like having IpcSerParams with an option to fully serialize all data. In any case this is just a theoretical problem, so I have a good idea of what to do now.

If the issue is the separation between the CNetAddr::SerParams declaration and the MultiSerParams(..,CNetAddr::V2,..) use, could instead make it:

class IPCSerParam
{
public:
    template<typename T>
    operator T&() const { return T::GetIPCSerParams(); }
};
static constexpr IPCSerParam IPC_SER_PARAMS;

class CNetAddr {
    struct SerParams {
        const Encoding enc;
        static SerParams GetIPCSerParams() { return {Encoding::V2}; }
        SER_PARAMS_OPFUNC
   };
};

That leaves it explicit for the dumb streams, and provides a clear, localised default for internal serialization of the object?

(I'm more worried about changing a default from v2 to v3 causing subtle bugs when deserialising network data or from a file, so I prefer being explicit everywhere and finding some other way to fix the other annoyances)

I guess the idea of having default parameters is off the table then. So probably the way forward to make IPC code safer is have an IPC serialization parameters like your suggestion.

For now though I left the safety issue aside and implemented a way for IPC code to pass multiple stream parameters to ParamsStream in #28929 (used here in #10102), so the same stream can be used for all serialization, and there is no need for code that creates different streams depending on the object type. This solves the problem of parameters increasing boilerplate in multiprocess code.

I'm just saying what my opinion is; doesn't mean I'm necessarily right. What you've come up with looks pretty good to me though.

re: #28438 (comment)

I think your v2->v3 example provides a convincing reason why defaulting to the latest version is not always safe. Before this, I was thinking that even if default transaction parameters were not a good idea, making the latest versions default would probably be good. But since this isn't the case, relying on defaults really isn't a good general solution and IPC code probably should specify some explicit parameters, even if they are not hardcoded version numbers. Another way to do this wouldn't require adding new IPC parameter type could be to define aliases like static constexpr SerParams MAX_VERSION = V2 and have IPC code use MAX_VERSION.