From 26c0c24433ec9d587e782cdb2214284d16ba6c19 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 16 Jul 2020 21:33:13 +0200 Subject: [PATCH] Merge #16525: Dump transaction version as an unsigned integer in RPC/TxToUniv e80259f1976545e4f1ab6a420644be0c32261773 Additionally treat Tx.nVersion as unsigned in joinpsbts (Matt Corallo) 970de70bdd3542e75b73c79b06f143168c361494 Dump transaction version as an unsigned integer in RPC/TxToUniv (Matt Corallo) Pull request description: Consensus-wise we already treat it as an unsigned integer (the only rules around it are in CSV/locktime handling), but changing the underlying data type means touching consensus code for a simple cleanup change, which isn't really worth it. See-also, https://github.com/rust-bitcoin/rust-bitcoin/pull/299 ACKs for top commit: sipa: ACK e80259f1976545e4f1ab6a420644be0c32261773 practicalswift: ACK e80259f1976545e4f1ab6a420644be0c32261773 ajtowns: ACK e80259f1976545e4f1ab6a420644be0c32261773 code review -- checked all other uses of tx.nVersion treat it as unsigned (except for policy.cpp:IsStandard anyway), so looks good. naumenkogs: ACK e80259f Tree-SHA512: 6760a2c77e24e9e1f79a336ca925f9bbca3a827ce02003c71d7f214b82ed3dea13fa7d9f87df9b9445cd58dff8b44a15571d821c876f22f8e5a372a014c9976b --- doc/release-notes-16525.md | 9 +++++++++ src/core_write.cpp | 4 +++- src/rpc/rawtransaction.cpp | 8 ++++---- test/functional/rpc_rawtransaction.py | 3 ++- 4 files changed, 18 insertions(+), 6 deletions(-) create mode 100644 doc/release-notes-16525.md diff --git a/doc/release-notes-16525.md b/doc/release-notes-16525.md new file mode 100644 index 00000000000000..220cb78de422ee --- /dev/null +++ b/doc/release-notes-16525.md @@ -0,0 +1,9 @@ +RPC changes +----------- + +Exposed transaction version numbers are now treated as unsigned 32-bit integers +instead of signed 32-bit integers. This matches their treatment in consensus +logic. Versions greater than 2 continue to be non-standard (matching previous +behavior of smaller than 1 or greater than 2 being non-standard). Note that +this includes the joinpsbt command, which combines partially-signed +transactions by selecting the highest version number. diff --git a/src/core_write.cpp b/src/core_write.cpp index 8bb7e57d7bc894..560f5cfea90ece 100644 --- a/src/core_write.cpp +++ b/src/core_write.cpp @@ -190,7 +190,9 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, { uint256 txid = tx.GetHash(); entry.pushKV("txid", txid.GetHex()); - entry.pushKV("version", tx.nVersion); + // Transaction version is actually unsigned in consensus checks, just signed in memory, + // so cast to unsigned before giving it to the user. + entry.pushKV("version", static_cast(static_cast(tx.nVersion))); entry.pushKV("type", tx.nType); entry.pushKV("size", (int)::GetSerializeSize(tx, PROTOCOL_VERSION)); entry.pushKV("locktime", (int64_t)tx.nLockTime); diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index bcda6799c515db..13a950af93cd9e 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1604,7 +1604,7 @@ UniValue joinpsbts(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_PARAMETER, "At least two PSBTs are required to join PSBTs."); } - int32_t best_version = 1; + uint32_t best_version = 1; uint32_t best_locktime = 0xffffffff; for (unsigned int i = 0; i < txs.size(); ++i) { PartiallySignedTransaction psbtx; @@ -1614,8 +1614,8 @@ UniValue joinpsbts(const JSONRPCRequest& request) } psbtxs.push_back(psbtx); // Choose the highest version number - if (psbtx.tx->nVersion > best_version) { - best_version = psbtx.tx->nVersion; + if (static_cast(psbtx.tx->nVersion) > best_version) { + best_version = static_cast(psbtx.tx->nVersion); } // Choose the lowest lock time if (psbtx.tx->nLockTime < best_locktime) { @@ -1626,7 +1626,7 @@ UniValue joinpsbts(const JSONRPCRequest& request) // Create a blank psbt where everything will be added PartiallySignedTransaction merged_psbt; merged_psbt.tx = CMutableTransaction(); - merged_psbt.tx->nVersion = best_version; + merged_psbt.tx->nVersion = static_cast(best_version); merged_psbt.tx->nLockTime = best_locktime; // Merge diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 84c40f857d2f7f..4efac7b0514e56 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -359,11 +359,12 @@ def run_test(self): # Note, this is different to bitcoin. Bitcoin has a 32 bit integer # representing the version, we have 16 bits of version and 16 bits of # type. + # As transaction version is unsigned, this should convert to its unsigned equivalent. tx = CTransaction() tx.nVersion = -0x8000 rawtx = ToHex(tx) decrawtx = self.nodes[0].decoderawtransaction(rawtx) - assert_equal(decrawtx['version'], -0x8000) + assert_equal(decrawtx['version'], 0x80000000) # Test the maximum transaction version number that fits in a signed 32-bit integer. tx = CTransaction()