[Consensus] Change Elements serialization format

[tdudz]

Since elements is a sidechain and uses segwit, there is no need to support old-style serialization. Furthermore, getting rid of the 0x00 marker allows us to support incomplete mimblewimble transactions with empty inputs that still contain witness data.

The new proposed serialization format is as follows:

nVersion | flags | inputs | outputs | witness | nLockTime

The flag field is kept to allow for possible additional tx serialization features down the line. Currently, the first bit (a flag of 0x01) indicates witness data is present.

Since some of the tests depended on a pre-serialized transaction, I had to change the rawtx used in certain test cases. To do so, I decoded the previous transaction via RPC and then built the same transaction with the new serialization, checking it by hand. The size is now one byte larger due to the flag, so this was adjusted accordingly. It may be useful to double check the test case raw transaction (or add one with witnesses, since this one doesn't contain any), but it should be fine for now. All tests currently pass.

There is more cleanup that can be done in the codebase after this change, specifically regarding the use of the SERIALIZE_TRANSACTION_NO_WITNESS constant. I am working on this.

Be sure to start with a fresh local blockchain when applying this to avoid serialization mixups.

[instagibbs]

concept ACK

This will need reasonable amounts of testing.

[sipa]

You need to check here whether SERIALIZE_TRANSACTION_NO_WITNESS was specified, and if so, leave flags at 0. Otherwise, your txids will commit to the witness data (making your transactions malleable again).

[tdudz]

issue has been addressed

[apoelstra]

Can the flag be rolled into a bit of nVersion?

[maaku]

@apoelstra That seems layer violating, given that the flag governs a property of the serialization not the transaction itself. A transaction always has a witness (in elements at least), but that witness might not be in the serialization. An SPV client, for example, may request transactions without witnesses. Do you clobber a bit of nVersion to indicate that? Or do we permanently burn a bit to leave open for this purpose? I would keep the serialization flags as-is.

Feedback: I would put the witness very last in the serialization, which preserves the property that the hash of the first part of the buffer, from nVersion to nLockTime, is the txid.

[apoelstra]

Good point @maaku, agreed

[tdudz]

I have updated the code, the new proposed serialization format is:

nVersion | flags | inputs | outputs | nLockTime | witness

Tests are passing with this. Remember to delete any blockchains with different serializations saved on disk, and if the python rpc test suite is reporting corrupted database, delete the cache folder as well.

Since the first part of the buffer from nVersion to nLockTime is the txid, I think some of the code can be cleaned up to only serialize the tx once. Then, when we want to differentiate between getting the txid vs. the wtxid of a serialized tx we specify which bytes to hash instead of serializing a second time. I will look into this at some point.

Also, I noticed that the test case in the cpp code for creating/decoding raw transactions is very non-thorough and only tests a version 1 transaction with one input, an OP_RETURN output, and no witness data. After I changed the order of the witness field in the serialization just now, the tests still pass, which should not have been the case with a tx with witness data. I'll update the test file in this same PR once I come up with a better test case (not sure if it makes sense to open a separate PR for it since the serialization of the test case will only match what i've done here).

[tdudz]

Added updated test where the tx that was serialized has witness data in it, so if serialization format is changed again this test should break

[instagibbs]

@tdudz would you mind rebasing this PR? Would help trigger Travis build. Considering getting this merged ASAP to simplify some of our serialization python code.

[instagibbs]

Please clean up the history while you are at it.

[instagibbs]

Could you rebase onto tip of elements-0.14.1? Mininode tests like replace-by-fee.py should fail due to different serializations. You'll have to update how it serializes(I can also do it for you if needed).

[instagibbs]

So I see you've rebased, this must just be Travis being weird. Locally after checking out your branch that test still fails.

[instagibbs]

@tdudz could you rebase one more time onto master, and cherry-pick this? instagibbs@cc914ae

replace-by-fee.py now runs by default(instead of extended tests) and the commit fixes the test for your PR.

[instagibbs]

ACK

[instagibbs]

realized my mininode serialization fix is wrong. Only works because there's currently no test for witness data. We need to swap the witness/nlocktime serialization.

[instagibbs]

nLockTime needs to go before witness serialization

[apoelstra]

utACK aside from the mininode test

[tdudz]

rebased, squashed, and serialization in mini node testing fixed (both in serialize and deserialize)

Untested though, got a new computer and need to get all dependencies and stuff to compile first

[instagibbs]

ACK