Add hex serialization functions

[svechinsky]

Addressing #2134
Comments about everything are welcome.

[jaspervdm]

Note that this will break sending/receiving transactions between upgraded and non-upgraded wallets

[svechinsky]

hmmm overlooked this, will hopefully add checks tomorrow

[yeastplume]

Yes, this looks like the right approach, but we're going to have to carefully think about when and how we deploy this. I don't think it's trivial (or at least, it's messy) to be able to support both serialisation formats at once.

[svechinsky]

Maybe we can have some separate utility for converting old to new serialization format and vice versa?

The best way I can think of to support both formats is to add a flag for send/receive txs to serialize the output in the old format.

[yeastplume]

Just looking through this in some more detail, as I think I'd like this in place before merging #2441 since I'd like to use the same serialization features there as well.

Can I suggest moving all of the serialization methods into their own separate module rather than spreading them around, perhaps in core/src/libtx for now (until we decide the best place to put it). I think it will be useful to have all these methods to sanely serialise libsecp primitives somewhere common. We may even want to split this PR into 2, with one to add the serialisation module and another to turn them on in a backwards compatible way. That would also help me move on with #2441

[svechinsky]

Ok, so:

1. Reorganize the serialization modules into a separate module.
2. Comment out the serde directives for activation at a later date

[yeastplume]

Yes, sounds like a good plan, thanks!

[svechinsky]

@yeastplume Just in case you missed it, pinging for a review

[antiochp]

I'm not too enthusiastic about merging in commented out code like this.

We should either agree to keep this PR open and track it against master (not a huge fan of this either) or push forward on getting this done in a way that preserves backward compatibility.

[svechinsky]

What are your thoughts about a separate utility for converting between the different serializations (a lot cleaner than having to pass a flag from the command line)?
That way those who upgrade to a new client but must temporarily support unupgraded clients (probably applies to businesses only) could use it for the amount of time it takes everyone to upgrade

[ignopeverell]

You already have custom serializers now, why not make them revert to the old representation on hex errors?

[svechinsky]

The problem is not with deserializing binary/hex but rather with serializing it so that old clients can accept them.

[yeastplume]

@svechinsky Right, we're going to have to take a decision at some point as to what to do about older wallets, obviously they won't be able to handle the new format and will choke rather gracelessly. Also, we should probably take the opportunity here to add a version ID to the slate to help us mitigate future problems, and ensure the wallet takes it into consideration (and keep a minimum version it can handle).

Unfortunately this has gone a little bit worm-canny. Can I suggest the following:

1. Remove the commented out directives, and just commit the serialization lib in this PR. (This will let me get on with Save slate participant messages in database #2441 as well)
2. Another PR for an optional Slate ID (and have to consider how this remains backwards compatible, given it would adds a field that doesn't exist in older clients)
3. Another PR to handle the integration of new slate + backwards compatibility concerns there

[svechinsky]

Ok, will sort out today/tomorrow, will def do 1 today so it won't block #2441

[yeastplume]

Okay, great thank you. Lib functions looks good as they are, so I'll merge.