Add “flags” capability to streams #19477

1. 
   vasild commented at 7:43 pm on July 9, 2020: contributor

   Is your feature request related to a problem? Please describe.

   Currently the streams (e.g. CDataStream, CVectorWriter) classes use a “protocol version” which defines how something should be written to or read from the stream.

   This is not flexible enough because in some cases the content does not depend on the protocol, but depends on other things. In order to workaround this limitation the code is sneaking some constants into the “protocol version” to tune the (un)serialize behavior. Two examples:

   1. SERIALIZE_TRANSACTION_NO_WITNESS - to control whether witnesses should be present or not.

   2. ADDRv2_FORMAT (from #19031, not yet merged at the time of writing) to control whether addresses should be (un)serialized in ADDRv2/BIP155 format.

   This is problematic because once such a constant is bitwise-OR-ed into the protocol version then code that checks for a particular version becomes invalid, e.g. version == SENDHEADERS_VERSION or version > CADDR_TIME_VERSION. Also, it is outright confusing.

   Describe the solution you’d like

   Introduce “flags” to the streams, mimicking iostream::setf(), independent from the “protocol version”. And manipulate them like:

   0s.SetFlag(SERIALIZE_TRANSACTION_NO_WITNESS)
   1s.RemoveFlag(SERIALIZE_TRANSACTION_NO_WITNESS)
   2s.IsFlagSet(SERIALIZE_TRANSACTION_NO_WITNESS)
   

2. vasild added the label Feature on Jul 9, 2020
3. 
   dongcarl commented at 8:15 pm on July 9, 2020: contributor

   It seems to me that actually, since we don’t ever need to serialize these serialization flags (confusing I know), we can just add them as bool members to the relevant classes?
4. 
   sipa commented at 8:20 pm on July 9, 2020: member

   My (longer-term) idea here was that we need to get rid of version and hash_type entirely in the serialization framework.

   Instead, every serializer could be declared to have an optional Options class, which needs to be specified when serializing/deserializing, and may be passed down.

   For example there could be a TransactionOptions class that includes a include_witness flag. Blocks, and other things that serialize transactions would be declared to also have a TransactionOptions, or there could be a BlockOptions that includes a TransactionOptions member even.

5. 
   vasild commented at 6:49 am on July 10, 2020: contributor

   @sipa, that sounds even better to me. Is there an issue to track it, or should we use this one?

   How would the Options class be passed during (un)serialization? E.g.:

   0std::vector<Foo> foos;
   1CDataStream stream;
   2Foo::Options options{paint_it_black=true};
   3...
   4// how to pass `options` here?
   5stream >> foos;
   6stream << foos;
   

6. 
   sipa commented at 6:54 am on July 10, 2020: member

   @vasild I’m working on something; I hope to show it soon.
7. MarcoFalke added the label Brainstorming on Jul 10, 2020
8. 
   sipa commented at 4:41 am on July 13, 2020: member

   @vasild See https://github.com/bitcoin/bitcoin/pull/19503
9. laanwj added the label Utils/log/libs on Sep 3, 2020
10. 
    MarcoFalke commented at 1:01 pm on June 6, 2022: member

    I’ve picked up #19503, but made it a bit stronger. #19503 added a WithParams helper to annotate arguments with serialization parameters. However, it still allowed mixed use of stream-params and stream-version/stream-type. I think we should treat serialization parameters as replacement for version/type. This will make the changes more verbose, but safer. It will be enforced at compile time that a “serialization-stack” (for example CAddress->CService->CNetAddr) is atomically either using version/type or params.
11. 
    MarcoFalke commented at 3:06 pm on September 5, 2023: member

    Closing for now. Let’s continue discussion in #25284, which should be wrapping up soon.
12. MarcoFalke closed this on Sep 5, 2023

    ---

Contributors
vasild dongcarl sipa MarcoFalke

Labels
Feature Brainstorming Utils/log/libs