Add “flags” capability to streams #19477

issue vasild openend this issue on July 9, 2020
  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. maflcko added the label Brainstorming on Jul 10, 2020
  8. sipa commented at 4:41 am on July 13, 2020: member
  9. laanwj added the label Utils/log/libs on Sep 3, 2020
  10. maflcko 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. maflcko 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. maflcko closed this on Sep 5, 2023

  13. fanquake referenced this in commit 8e0d9796da on Sep 7, 2023
  14. fanquake referenced this in commit 108462139b on Nov 15, 2023
  15. bitcoin locked this on Sep 4, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-08 16:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me