consensus: Store transaction nVersion as uint32

achow101 commented at 9:53 pm on January 25, 2024: member

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 commented at 9:53 pm on January 25, 2024: contributor

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)
#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 added the label Consensus on Jan 25, 2024

achow101 force-pushed on Jan 25, 2024

achow101 renamed this:
~~consensus: Store transaction nVersion as uin32_t~~
consensus: Store transaction nVersion as uint32_t
on Jan 25, 2024

DrahtBot added the label CI failed on Jan 25, 2024

DrahtBot commented at 10:53 pm on January 25, 2024: contributor

🚧 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 commented at 11:06 pm on January 25, 2024: contributor

Concept ACK

Another instance in the tests which could be adapted to 0xffffffff: https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/test/transaction_tests.cpp#L783 Slightly related to this PR: since this currently doesn’t produce a warning, was it ever considered to enable -Wsign-conversion?

sipa commented at 11:09 pm on January 25, 2024: member

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.

in src/core_write.cpp:179 in 59d0b7e2d5 outdated

175@@ -176,7 +176,7 @@ void TxToUniv(const CTransaction& tx, const uint256& block_hash, UniValue& entry
176     entry.pushKV("hash", tx.GetWitnessHash().GetHex());
177     // Transaction version is actually unsigned in consensus checks, just signed in memory,
178     // so cast to unsigned before giving it to the user.
179-    entry.pushKV("version", static_cast<int64_t>(static_cast<uint32_t>(tx.nVersion)));
180+    entry.pushKV("version", static_cast<int64_t>(tx.nVersion));

maflcko commented at 7:46 am on January 26, 2024:

0    entry.pushKV("version", tx.nVersion);

No need for a cast here. Also, the comment above is wrong.

achow101 commented at 8:29 pm on January 26, 2024:

Done, removed the comment.

in test/functional/feature_taproot.py:1413 in 59d0b7e2d5 outdated

1409@@ -1410,7 +1410,7 @@ def test_spenders(self, node, spenders, input_counts):
1410         while left:
1411             # Construct CTransaction with random nVersion, nLocktime
1412             tx = CTransaction()
1413-            tx.nVersion = random.choice([1, 2, random.randint(-0x80000000, 0x7fffffff)])
1414+            tx.nVersion = random.choice([1, 2, random.randrange(0xffffffff)])

maflcko commented at 7:48 am on January 26, 2024:

0            tx.nVersion = random.choice([1, 2, random.randbits(32)])

nit: No need to call the randbits wrapper

achow101 commented at 8:29 pm on January 26, 2024:

Done

maflcko approved

maflcko commented at 7:54 am on January 26, 2024: member

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)

maflcko commented at 8:02 am on January 26, 2024: member

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 commented at 8:07 am on January 26, 2024: none

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.

in src/consensus/tx_verify.cpp:54 in 59d0b7e2d5 outdated

50@@ -51,8 +51,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
51     // tx.nVersion is signed integer so requires cast to unsigned otherwise
52     // we would be doing a signed comparison and half the range of nVersion
53     // wouldn't support BIP 68.
54-    bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2
55-                      && flags & LOCKTIME_VERIFY_SEQUENCE;
56+    bool fEnforceBIP68 = tx.nVersion >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;

vostrnad commented at 8:09 am on January 26, 2024:

Comment above needs updating.

achow101 commented at 8:29 pm on January 26, 2024:

Removed it.

fanquake commented at 10:02 am on January 26, 2024: member

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.

in test/functional/rpc_rawtransaction.py:479 in 59d0b7e2d5 outdated

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

shaavan commented at 12:36 pm on January 26, 2024:

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?

achow101 commented at 8:31 pm on January 26, 2024:

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

shaavan commented at 12:37 pm on January 26, 2024: contributor

Concept ACK

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

achow101 force-pushed on Jan 26, 2024

achow101 commented at 8:32 pm on January 26, 2024: member

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.

achow101 force-pushed on Jan 26, 2024

dergoegge commented at 1:38 pm on January 29, 2024: member

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

in src/bitcoin-tx.cpp:213 in 3a41d8ca84 outdated

209@@ -210,7 +210,7 @@ static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
210         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
211     }
212 
213-    tx.nVersion = (int) newVersion;
214+    tx.version = (int) newVersion;

maflcko commented at 1:48 pm on January 29, 2024:

0    tx.version =  newVersion;

achow101 commented at 5:45 pm on January 29, 2024:

Since newVersion is an int64_t, I think this still needs to be cast to uint32_t, which I’ve done.

in src/primitives/transaction.cpp:66 in 3a41d8ca84 outdated

62@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const
63     return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));
64 }
65 
66-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
67-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
68+CMutableTransaction::CMutableTransaction() : version(CTransaction::CURRENT_VERSION), nLockTime(0) {}

maflcko commented at 1:49 pm on January 29, 2024:

0CMutableTransaction::CMutableTransaction() : version{CTransaction::CURRENT_VERSION}, nLockTime{0} {}

nit: {} cat be used to check at compile time that the integral type does not change the sign or is truncated

achow101 commented at 5:46 pm on January 29, 2024:

Done

in src/primitives/transaction.cpp:67 in 3a41d8ca84 outdated

62@@ -63,8 +63,8 @@ std::string CTxOut::ToString() const
63     return strprintf("CTxOut(nValue=%d.%08d, scriptPubKey=%s)", nValue / COIN, nValue % COIN, HexStr(scriptPubKey).substr(0, 30));
64 }
65 
66-CMutableTransaction::CMutableTransaction() : nVersion(CTransaction::CURRENT_VERSION), nLockTime(0) {}
67-CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), nVersion(tx.nVersion), nLockTime(tx.nLockTime) {}
68+CMutableTransaction::CMutableTransaction() : version(CTransaction::CURRENT_VERSION), nLockTime(0) {}
69+CMutableTransaction::CMutableTransaction(const CTransaction& tx) : vin(tx.vin), vout(tx.vout), version(tx.version), nLockTime(tx.nLockTime) {}

maflcko commented at 1:49 pm on January 29, 2024:

same

achow101 commented at 5:46 pm on January 29, 2024:

Done

achow101 force-pushed on Jan 29, 2024

achow101 commented at 5:46 pm on January 29, 2024: member

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.

DrahtBot removed the label CI failed on Jan 29, 2024

naumenkogs commented at 9:08 am on January 30, 2024: member

Concept ACK

in src/bitcoin-tx.cpp:213 in af5a1fdfa6 outdated

209@@ -210,7 +210,7 @@ static void MutateTxVersion(CMutableTransaction& tx, const std::string& cmdVal)
210         throw std::runtime_error("Invalid TX version requested: '" + cmdVal + "'");
211     }
212 
213-    tx.nVersion = (int) newVersion;
214+    tx.nVersion = static_cast<uint32_t>(newVersion);

maflcko commented at 9:43 am on January 30, 2024:

What is the point of parsing as signed integer and then casting to unsigned? Why not use ParseUInt32 directly?

achow101 commented at 5:20 pm on January 30, 2024:

Ah, didn’t realize we had that. Done

in src/primitives/transaction.h:299 in af5a1fdfa6 outdated

295@@ -296,7 +296,7 @@ class CTransaction
296 {
297 public:
298     // Default transaction version.
299-    static const int32_t CURRENT_VERSION=2;
300+    static const uint32_t CURRENT_VERSION=2;

maflcko commented at 9:45 am on January 30, 2024:

nit: Clang-format while touching this line of code?

0static const uint32_t CURRENT_VERSION{2};

achow101 commented at 5:20 pm on January 30, 2024:

Done

maflcko approved

DrahtBot added the label Needs rebase on Jan 30, 2024

achow101 force-pushed on Jan 30, 2024

DrahtBot removed the label Needs rebase on Jan 30, 2024

in test/functional/test_framework/messages.py:584 in ca7090d43f outdated

580@@ -581,7 +581,7 @@ def __init__(self, tx=None):
581             self.wit = copy.deepcopy(tx.wit)
582 
583     def deserialize(self, f):
584-        self.nVersion = int.from_bytes(f.read(4), "little", signed=True)
585+        self.nVersion = int.from_bytes(f.read(4), "little", signed=False)

maflcko commented at 12:07 pm on February 1, 2024:

nit in ca7090d43fab2013bd396d683f4cf03062666b14: I think it would be consistent to omit the argument here, which is False by default. Otherwise, it could create the impression that other calls that have it omitted are different (signed) serializations. For example, the unsigned locktime below also has it omitted.

One could even argue that anything that has a signed value in this file here is a bug or at a minimum a red flag. See for example: fa3886b7c69cbbe564478f30bb2c35e9e6b1cffa or https://github.com/bitcoin/bitcoin/pull/29363

achow101 commented at 4:35 pm on February 20, 2024:

Done as suggested.

in test/functional/test_framework/script.py:742 in ca7090d43f outdated

738@@ -739,7 +739,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount):
739         hashOutputs = uint256_from_str(hash256(serialize_outputs))
740 
741     ss = bytes()
742-    ss += struct.pack("<i", txTo.nVersion)
743+    ss += struct.pack("<I", txTo.nVersion)

maflcko commented at 12:26 pm on February 1, 2024:

unrelated nit: Would be nice to use the python built-in int.to_bytes feature, because the struct interface is confusing and has lead to bugs in the past, see above.

(Can be done in a follow-up, as a good-first issue)

achow101 commented at 4:35 pm on February 20, 2024:

Done as suggested.

in src/primitives/transaction.cpp:118 in 6acfcbfe24 outdated

116@@ -117,7 +117,7 @@ std::string CTransaction::ToString() const
117     std::string str;
118     str += strprintf("CTransaction(hash=%s, ver=%d, vin.size=%u, vout.size=%u, nLockTime=%u)\n",

maflcko commented at 12:33 pm on February 1, 2024:

review note: This is fine for tinyformat, but if you re-touch it could make sense to change the “signed decimal” to %s or %u.

achow101 commented at 4:35 pm on February 20, 2024:

Done

in test/functional/p2p_segwit.py:1167 in 6acfcbfe24 outdated

1163@@ -1164,7 +1164,7 @@ def serialize_with_witness(self):
1164                 if not self.wit.is_null():
1165                     flags |= 1
1166                 r = b""
1167-                r += struct.pack("<i", self.nVersion)
1168+                r += struct.pack("<i", self.version)

maflcko commented at 12:39 pm on February 1, 2024:

unrelated follow-up: (Same here: Remove struct)

achow101 commented at 4:36 pm on February 20, 2024:

Done

maflcko approved

maflcko commented at 12:40 pm on February 1, 2024: member

ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌵

Signature:

0untrusted 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}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f 🌵
3rWI9ANXeLccWSbPTbyKsnso1o6VZc+jr+YkpUEQVUs4ZzPer/ucIHhkSP4LZEz8sZYOUbXyFmxFoSAGyuvN4DA==

DrahtBot requested review from theStack on Feb 1, 2024

DrahtBot requested review from sipa on Feb 1, 2024

DrahtBot requested review from naumenkogs on Feb 1, 2024

DrahtBot requested review from shaavan on Feb 1, 2024

DrahtBot requested review from vostrnad on Feb 1, 2024

DrahtBot removed review request from vostrnad on Feb 1, 2024

DrahtBot removed review request from shaavan on Feb 1, 2024

DrahtBot requested review from shaavan on Feb 1, 2024

DrahtBot requested review from vostrnad on Feb 1, 2024

DrahtBot added the label Needs rebase on Feb 10, 2024

in src/test/transaction_tests.cpp:783 in ca7090d43f outdated

779@@ -780,7 +780,7 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
780     CheckIsStandard(t);
781 
782     // Disallowed nVersion
783-    t.nVersion = -1;
784+    t.nVersion = static_cast<uint32_t>(-1);

naumenkogs commented at 8:48 am on February 20, 2024:

ca7090d43fab2013bd396d683f4cf03062666b14

Does this still look meaningful? If you want to test a large number, then better check against TX_MAX_STANDARD_VERSION + 1?

maflcko commented at 11:04 am on February 20, 2024:

If you want to test a large number, then better check against TX_MAX_STANDARD_VERSION + 1?

This check already exists two lines below.

Does this still look meaningful?

Yes, the goal is to check the edge cases (minimum and maximum possible number)

sipa commented at 11:20 am on February 20, 2024:

It’d look a bit more natural using = 0xffffffff or = std::numeric_limits<uint32_t>::max().

achow101 commented at 4:36 pm on February 20, 2024:

Changed to std::numeric_limits<uint32_t>::max()

naumenkogs approved

naumenkogs commented at 8:53 am on February 20, 2024: member

ACK 6acfcbfe2487f683e8f62606d195a9974bc2234f

DrahtBot removed review request from shaavan on Feb 20, 2024

DrahtBot removed review request from vostrnad on Feb 20, 2024

DrahtBot requested review from shaavan on Feb 20, 2024

DrahtBot requested review from vostrnad on Feb 20, 2024