consensus: Store transaction nVersion as uint32_t #29325

achow101 · 2024-01-25T21:53:23Z

Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.

Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.

I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.

Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.

DrahtBot · 2024-01-25T21:53:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, glozow, shaavan
Concept ACK	theStack, sipa
Stale ACK	naumenkogs, vostrnad

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30050 (refactor, wallet: get serialized size of CRecipients directly by josibake)
#29564 (wallet: clarify FundTransaction signature by S3RK)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#28676 ([WIP] Cluster mempool implementation by sdaftuar)
#21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2024-01-25T22:53:55Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20879211240}

theStack · 2024-01-25T23:06:35Z

Concept ACK

Another instance in the tests which could be adapted to 0xffffffff:

bitcoin/src/test/transaction_tests.cpp

Line 783 in 717103b

t.nVersion = -1;

Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable -Wsign-conversion?

sipa · 2024-01-25T23:09:53Z

Concept ACK on making transaction's nVersion unsigned, as that is in practice how it already behaves.

Would it make sense to also rename the field to e.g. unsigned_version or so, so that:

Reviewing the diff makes it obvious whether any references to the field exist that aren't modified.
Any future contributor who missed this change won't accidentally assume the field still has signed semantics?

For normal code I'd consider this measure overkill, but this is pretty fundamental consensus-critical code.

maflcko

Agree that a rename of the field should be considered, but my preference would be just version, as opposed to unsigned_version. Otherwise, the code might be annoyingly verbose, even when it is clear to everyone that the transaction version is unsigned. (Maybe in a separate commit, so that if the changes are touching too many lines, or reviewers don't like it, it can be dropped again)

src/core_write.cpp

test/functional/feature_taproot.py

maflcko · 2024-01-26T08:02:03Z

Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable -Wsign-conversion?

I think it is a common convention to write -1 as an alias for std::numeric_limits<unsigned ...>::max(). Also, I expect there will be many places where positive signed integers are sign-preserving converted to unsigned (and vice-versa). So enabling that compiler warning may not be possible without changing a lot more code.

Luckily, if a sign isn't preserved at runtime, during the tests, the sanitizers will catch it. (See the CI failure)

vostrnad · 2024-01-26T08:07:36Z

Concept ACK

As someone quite familiar with Bitcoin's consensus rules but largely unfamiliar with Bitcoin Core's internals, I had no idea until recently that the version was stored as a signed integer. This should bring it in line with expectations of most future contributors.

Also negative versions are just weird.

src/consensus/tx_verify.cpp

fanquake · 2024-01-26T10:02:53Z

So enabling that compiler warning may not be possible without changing a lot more code.

Yea. See here for example output compiling master with GCC 13.2.0 + -Wsign-conversion: https://gist.github.com/fanquake/8e7a49dc1968afd07d89e822ffb4adac.

shaavan

Concept ACK

Notes:

Change the type of nVersion for transactions from int32_t → uint32_t
Appropriately remove casts, from the codebase
Introduce test cases to properly address the change in behavior
Appropriately change the format form <i to <I indicating the change from signed little-endian to unsigned little-endian when serializing and deserializing tx.nVersion.

Suggestions:

In void static RandomTransaction(CMutableTransaction& tx, bool fSingle) in sighash_tests.cpp file:

tx.nVersion = int(InsecureRand32()); → tx.nVersion = InsecureRand32(); since the return type of the function is uint32_t

shaavan · 2024-01-26T12:36:40Z

test/functional/rpc_rawtransaction.py

@@ -478,6 +478,20 @@ def transaction_version_number_tests(self):
        decrawtx = self.nodes[0].decoderawtransaction(rawtx)
        assert_equal(decrawtx['version'], 0x7fffffff)



Question: If we are removing the "signedness" of the tx.nVersion, should we think about removing the test cases that made sense for the signed nVersion?

I think it makes sense to leave them in to make sure that everything still matches previous versions.

achow101 · 2024-01-26T20:32:12Z

I've added a commit that renames nVersion to just version. Note that it is not a scripted diff since a bunch of different variables share the same name.

dergoegge · 2024-01-29T13:38:01Z

CURRENT_VERSION should probably also change to uint32_t
CI failes here due to UB, can be fixed by changing to ConsumeIntegral<uint32_t>()

src/bitcoin-tx.cpp

src/primitives/transaction.cpp

achow101 · 2024-01-29T17:46:32Z

CURRENT_VERSION should probably also change to uint32_t

CI failes here due to UB, can be fixed by changing to ConsumeIntegral<uint32_t>()

Updated these.

achow101 · 2024-06-04T23:15:04Z

@maflcko @vostrnad @shaavan @naumenkogs reACKs would be appreciated

maflcko · 2024-06-05T09:44:08Z

ACK 2431ecf 🔳

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 2431ecff2fb6f4b68d3de0ea36e3bcc4403e94ba 🔳
xur9ZGGma5tikLjekGHRJ+CttEpJwRqGw/j5ke/6G6hfpK7N5+z+NP9z7nBzFBXKeGVFyAdjHuk/SQSxXJKrDQ==

shaavan

ACK 2431ecf 🚀

maflcko · 2024-06-07T07:39:17Z

ACK 659663a 🚋

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 659663af32f02c570e334e8f375fd5f5737258d7 🚋
d3lhhWY8KPIufcgBMSH5+n/TTcdj5RXJZN1O0Qj2O4NCepHVXB3nlSdnbSxKM7sufpYbbH2UrxxgSsPFbEO2Cw==

shaavan

ACK 659663a ⚡️

glozow · 2024-06-07T15:26:50Z

concept ACK + lgtm 659663a

Seems like this is a remaining mention of CTransaction::nVersion?

bitcoin/src/compressor.h

Lines 57 to 60 in 659663a

    
                * make this static for now (there are only 6 special scripts defined) 
        
                * this can potentially be extended together with a new nVersion for 
        
                * transactions, in which case this value becomes dependent on nVersion 
        
                * and nHeight of the enclosing transaction.

Also noting that #29496 unfortunately conflicts and I would advocate for merging that first... but I'm happy to re-review

Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned.

achow101 · 2024-06-07T16:46:59Z

Rebased

Seems like this is a remaining mention of CTransaction::nVersion?

Good catch, fixed.

In order to ensure that the change of nVersion to a uint32_t in the previous commit has no effect, rename nVersion to version in this commit so that reviewers can easily spot if a spot was missed or if there is a check somewhere whose semantics have changed.

maflcko · 2024-06-09T16:44:48Z

ACK 429ec1a 🐿

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
V0zhbzKiZuDg5Jhgt1TaDJ+/E32EhiOwnzfhBu5ZhuK2A8ZNVjfYRFlP0dTywWM3YcW0T2QOm47s8ncGNm7nAw==

glozow · 2024-06-10T08:41:54Z

ACK 429ec1a

Thank you for rebasing!

shaavan

ACK 429ec1a 💯

DrahtBot added the Consensus label Jan 25, 2024

achow101 force-pushed the tx-nversion-uint branch from 0a76440 to 59d0b7e Compare January 25, 2024 21:53

achow101 changed the title ~~consensus: Store transaction nVersion as uin32_t~~ consensus: Store transaction nVersion as uint32_t Jan 25, 2024

DrahtBot added the CI failed label Jan 25, 2024

DrahtBot mentioned this pull request Jan 26, 2024

test: Treat msg_version.relay as unsigned, Remove struct packing in messages.py #29067

Merged

maflcko approved these changes Jan 26, 2024

View reviewed changes

src/core_write.cpp Outdated Show resolved Hide resolved

test/functional/feature_taproot.py Outdated Show resolved Hide resolved

vostrnad reviewed Jan 26, 2024

View reviewed changes

src/consensus/tx_verify.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 26, 2024

Implement BIP 370 PSBTv2 #21283

Open

shaavan reviewed Jan 26, 2024

View reviewed changes

achow101 force-pushed the tx-nversion-uint branch from 59d0b7e to 2026070 Compare January 26, 2024 20:29

achow101 force-pushed the tx-nversion-uint branch 4 times, most recently from d81929a to 3a41d8c Compare January 26, 2024 23:09

This was referenced Jan 26, 2024

policy: enable sibling eviction for v3 transactions #29306

Merged

test: Add and use option for tx-version in MiniWallet methods #28972

Closed

v3 transaction policy for anti-pinning #28948

Merged

Silent Payments: sending #28201

Draft

maflcko reviewed Jan 29, 2024

View reviewed changes

src/bitcoin-tx.cpp Outdated Show resolved Hide resolved

src/primitives/transaction.cpp Outdated Show resolved Hide resolved

src/primitives/transaction.cpp Outdated Show resolved Hide resolved

achow101 force-pushed the tx-nversion-uint branch from 3a41d8c to 54e2d1d Compare January 29, 2024 17:45

DrahtBot mentioned this pull request May 7, 2024

refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition #29086

Merged

DrahtBot added the Needs rebase label May 15, 2024

achow101 force-pushed the tx-nversion-uint branch from cc0d6bd to 24d22af Compare May 15, 2024 19:10

DrahtBot removed the Needs rebase label May 15, 2024

DrahtBot added the Needs rebase label May 23, 2024

achow101 force-pushed the tx-nversion-uint branch from 24d22af to 9355a50 Compare May 23, 2024 17:01

DrahtBot removed the Needs rebase label May 23, 2024

DrahtBot mentioned this pull request May 23, 2024

refactor prep for package rbf #30072

Merged

DrahtBot added the Needs rebase label May 24, 2024

achow101 force-pushed the tx-nversion-uint branch from 9355a50 to 2431ecf Compare May 29, 2024 08:04

DrahtBot removed the Needs rebase label May 29, 2024

shaavan approved these changes Jun 5, 2024

View reviewed changes

achow101 force-pushed the tx-nversion-uint branch from 2431ecf to 659663a Compare June 6, 2024 23:38

DrahtBot requested a review from shaavan June 7, 2024 07:39

shaavan reviewed Jun 7, 2024

View reviewed changes

consensus: Store transaction nVersion as uint32_t

27e70f1

Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned.

achow101 force-pushed the tx-nversion-uint branch from 659663a to 4855976 Compare June 7, 2024 16:46

achow101 force-pushed the tx-nversion-uint branch from 4855976 to 429ec1a Compare June 7, 2024 17:55

DrahtBot requested review from glozow and shaavan June 9, 2024 16:44

glozow approved these changes Jun 10, 2024

View reviewed changes

shaavan reviewed Jun 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consensus: Store transaction nVersion as uint32_t #29325

consensus: Store transaction nVersion as uint32_t #29325

achow101 commented Jan 25, 2024

DrahtBot commented Jan 25, 2024 •

edited

DrahtBot commented Jan 25, 2024

theStack commented Jan 25, 2024

sipa commented Jan 25, 2024

maflcko left a comment

maflcko commented Jan 26, 2024

vostrnad commented Jan 26, 2024

fanquake commented Jan 26, 2024

shaavan left a comment

shaavan Jan 26, 2024

achow101 Jan 26, 2024

achow101 commented Jan 26, 2024

dergoegge commented Jan 29, 2024

achow101 commented Jan 29, 2024 •

edited

achow101 commented Jun 4, 2024

maflcko commented Jun 5, 2024

shaavan left a comment

maflcko commented Jun 7, 2024

shaavan left a comment

glozow commented Jun 7, 2024

achow101 commented Jun 7, 2024

maflcko commented Jun 9, 2024

glozow commented Jun 10, 2024

shaavan left a comment

		@@ -478,6 +478,20 @@ def transaction_version_number_tests(self):
		decrawtx = self.nodes[0].decoderawtransaction(rawtx)
		assert_equal(decrawtx['version'], 0x7fffffff)

consensus: Store transaction nVersion as uint32_t #29325

Are you sure you want to change the base?

consensus: Store transaction nVersion as uint32_t #29325

Conversation

achow101 commented Jan 25, 2024

DrahtBot commented Jan 25, 2024 • edited

Code Coverage

Reviews

Conflicts

DrahtBot commented Jan 25, 2024

theStack commented Jan 25, 2024

sipa commented Jan 25, 2024

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Jan 26, 2024

vostrnad commented Jan 26, 2024

fanquake commented Jan 26, 2024

shaavan left a comment

Choose a reason for hiding this comment

shaavan Jan 26, 2024

Choose a reason for hiding this comment

achow101 Jan 26, 2024

Choose a reason for hiding this comment

achow101 commented Jan 26, 2024

dergoegge commented Jan 29, 2024

achow101 commented Jan 29, 2024 • edited

achow101 commented Jun 4, 2024

maflcko commented Jun 5, 2024

shaavan left a comment

Choose a reason for hiding this comment

maflcko commented Jun 7, 2024

shaavan left a comment

Choose a reason for hiding this comment

glozow commented Jun 7, 2024

achow101 commented Jun 7, 2024

maflcko commented Jun 9, 2024

glozow commented Jun 10, 2024

shaavan left a comment

Choose a reason for hiding this comment

DrahtBot commented Jan 25, 2024 •

edited

achow101 commented Jan 29, 2024 •

edited