Implement new fields for the type and version of the transaction #1062

dsaveliev · 2019-05-05T21:41:26Z

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 [email protected]

pep8speaks · 2019-05-05T21:41:43Z

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

src/proposer/proposer.cpp

src/test/mempool_tests.cpp

src/test/transaction_tests.cpp

scravy · 2019-05-06T05:39:12Z

Concept ACK 6427ac9

scravy · 2019-05-06T05:43:40Z

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".

Signed-off-by: Dmitry Saveliev <[email protected]>

dsaveliev · 2019-05-06T08:27:10Z

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 · 2019-05-06T08:31:43Z

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.

kostyantyn · 2019-05-06T08:35:42Z

src/test/test_unite_block_fixture.cpp

          // tx version
-          "01000000"
+          "01"


I see we had a version and then type before and now it's opposite. I think it makes more sense to keep the previous order as it makes it easier to compare the change.

dsaveliev · 2019-05-06T08:42:10Z

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 · 2019-05-06T08:45:44Z

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.

dsaveliev added technical debt Cleaning up code which is there for historical reasons refactoring Changes which clean up code but don't change the user-visible behavior tests Automated tests labels May 5, 2019

dsaveliev added this to the 0.2 milestone May 5, 2019

dsaveliev requested review from scravy and a team May 5, 2019 21:41

dsaveliev self-assigned this May 5, 2019

scravy reviewed May 6, 2019

View reviewed changes

src/proposer/proposer.cpp Outdated Show resolved Hide resolved

src/test/mempool_tests.cpp Show resolved Hide resolved

src/test/transaction_tests.cpp Show resolved Hide resolved

Implement new fields for the type and version of transaction

6d5c41d

Signed-off-by: Dmitry Saveliev <[email protected]>

dsaveliev force-pushed the protocol-clean-up branch from 6427ac9 to 6d5c41d Compare May 6, 2019 08:12

kostyantyn reviewed May 6, 2019

View reviewed changes

This was referenced May 6, 2019

Minor adjustments for the tests and codestyle #1066

Merged

UIP-0027 - Independent service fields for transaction dtr-org/uips#21

Closed

thothd closed this Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new fields for the type and version of the transaction #1062

Implement new fields for the type and version of the transaction #1062

dsaveliev commented May 5, 2019 •

edited

Loading

pep8speaks commented May 5, 2019 •

edited

Loading

scravy commented May 6, 2019

scravy commented May 6, 2019

dsaveliev commented May 6, 2019 •

edited

Loading

scravy commented May 6, 2019

kostyantyn May 6, 2019

dsaveliev commented May 6, 2019

scravy commented May 6, 2019

Implement new fields for the type and version of the transaction #1062

Implement new fields for the type and version of the transaction #1062

Conversation

dsaveliev commented May 5, 2019 • edited Loading

pep8speaks commented May 5, 2019 • edited Loading

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

scravy commented May 6, 2019

scravy commented May 6, 2019

dsaveliev commented May 6, 2019 • edited Loading

scravy commented May 6, 2019

kostyantyn May 6, 2019

Choose a reason for hiding this comment

dsaveliev commented May 6, 2019

scravy commented May 6, 2019

dsaveliev commented May 5, 2019 •

edited

Loading

pep8speaks commented May 5, 2019 •

edited

Loading

dsaveliev commented May 6, 2019 •

edited

Loading