Implement new fields for the type and version of the transaction

[dsaveliev]

Related and intended to fix #1011

TL;DR: Let's keep it as is and reserve our "dead byte" for the future needs.

This PR is an attempt to substitute current uint32_t nVersion combo with uint8_t version and uint8_t type fields inside of CTransaction. The reason to keep version filed is quite simple - it serves the needs of BIP-68 (Relative lock-time using consensus-enforced sequence numbers) and essentially implements some kind of flag. In theory, this logic can be substituted by BIP-112 CHECKSEQUENCEVERIFY but this is another big topic for discussion. In this sense version and type express orthogonal concepts and this is the reason to keep them both in the transaction (otherwise we will get n_versions * m_types elements for unified field).

Even though an idea looks viable at first sight, harsh reality punches us right into face:

• Tests contain a lot of magic numbers, connected to the transaction size.
• Also, tests contain a lot of hardcoded transactions.
• Another concern is transaction_tests, which totally useless since we must rewrite tx_valid.json and tx_invalid.json completely (and better to write some kind of generator to produce this stubs automatically) And even if previous problems can be solved, continuous syncing with bitcoin codebase will force to repeat all this work over and over again.

Looks like it's better in similar cases to stick up to bitcoin protocol, otherwise, we will drown in tech debt very quickly. Consider this PR like an illustration of efforts, needed to introduce a minimal change in the protocol.

As an alternative, I can suggest to split nVersion into four 1-byte fields and preserve the original byte order. Two unused fields will be reserved for future needs.

Signed-off-by: Dmitry Saveliev dima@thirdhash.com

[pep8speaks]

Hello @dsaveliev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file test/functional/feature_segwit.py:

Line 521:121: E501 line too long (134 > 120 characters)

• In the file test/functional/rpc_decodescript.py:

Line 169:121: E501 line too long (457 > 120 characters) Line 176:121: E501 line too long (757 > 120 characters) Line 186:121: E501 line too long (435 > 120 characters) Line 191:121: E501 line too long (757 > 120 characters)

• In the file test/functional/rpc_filtertransactions.py:

Line 145:9: F401 'pprint' imported but unused

• In the file test/functional/rpc_fundrawtransaction.py:

Line 600:14: E221 multiple spaces before operator

• In the file test/functional/rpc_psbt.py:

Line 67:15: E703 statement ends with a semicolon

• In the file test/functional/rpc_rawtransaction.py:

Line 363:121: E501 line too long (149 > 120 characters) Line 368:121: E501 line too long (137 > 120 characters)

• In the file test/functional/rpc_signrawtransaction.py:

Line 56:121: E501 line too long (178 > 120 characters) Line 131:121: E501 line too long (708 > 120 characters)

Comment last updated at 2019-05-06 08:12:46 UTC

[scravy]

Concept ACK 6427ac92c8740462a04cc6b0bb399fd033ca3281

[scravy]

The description feels a bit out-of-place. The "dead byte" referred to in #1011 is the 3rd byte which serves no use currently, which this Pull Request correctly fixes.

TL;DR: Let's keep it as is and reserve our "dead byte" for the future needs.

This seems to refer to the version byte. That byte is not the byte which is referred to as "dead byte".

[dsaveliev]

Right now nVersion looks like this:

01    00     02     00
--------    ----   ----
version     type   unused

And I'm talking about the last byte - it would be easier for us to keep all 4 bytes of nVersion including this unused one.

[scravy]

And I'm talking about the last byte - it would be easier for us to keep all 4 bytes of nVersion including this unused one.

In my opinion just because something is easier is not a good excuse to not have a clean protocol.

[dsaveliev]

So, finally, we decided to keep all four original bytes but split them into four independent fields. It helps to keep compatibility with the bitcoin codebase and removes byte arithmetics with type and version.

[scravy]

We decided to go with this:

As an alternative, I can suggest to split nVersion into four 1-byte fields and preserve the original byte order. Two unused fields will be reserved for future needs.

This will make for 4 fields each 1 byte. This should be ultimately compatible with bitcoin, clearly distinguish version and type and leave room for two reserved fields for future use.