From 970de70bdd3542e75b73c79b06f143168c361494 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 1 Aug 2019 17:54:48 -0400 Subject: [PATCH 1/2] Dump transaction version as an unsigned integer in RPC/TxToUniv 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 --- doc/release-notes-16525.md | 7 +++++++ src/core_write.cpp | 4 +++- test/functional/rpc_rawtransaction.py | 3 ++- 3 files changed, 12 insertions(+), 2 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 000000000..f042c875f --- /dev/null +++ b/doc/release-notes-16525.md @@ -0,0 +1,7 @@ +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). diff --git a/src/core_write.cpp b/src/core_write.cpp index 4d64446d7..c7049eb16 100644 --- a/src/core_write.cpp +++ b/src/core_write.cpp @@ -179,7 +179,9 @@ void TxToUniv(const CTransaction& tx, const uint256& hashBlock, UniValue& entry, { entry.pushKV("txid", tx.GetHash().GetHex()); entry.pushKV("hash", tx.GetWitnessHash().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("size", (int)::GetSerializeSize(tx, PROTOCOL_VERSION)); entry.pushKV("vsize", (GetTransactionWeight(tx) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR); entry.pushKV("weight", GetTransactionWeight(tx)); diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py index 433867527..c2b80bc3d 100755 --- a/test/functional/rpc_rawtransaction.py +++ b/test/functional/rpc_rawtransaction.py @@ -417,11 +417,12 @@ class RawTransactionsTest(BitcoinTestFramework): #################################### # Test the minimum transaction version number that fits in a signed 32-bit integer. + # As transaction version is unsigned, this should convert to its unsigned equivalent. tx = CTransaction() tx.nVersion = -0x80000000 rawtx = ToHex(tx) decrawtx = self.nodes[0].decoderawtransaction(rawtx) - assert_equal(decrawtx['version'], -0x80000000) + assert_equal(decrawtx['version'], 0x80000000) # Test the maximum transaction version number that fits in a signed 32-bit integer. tx = CTransaction() From e80259f1976545e4f1ab6a420644be0c32261773 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 3 Sep 2019 10:53:45 -0400 Subject: [PATCH 2/2] Additionally treat Tx.nVersion as unsigned in joinpsbts This gets its own release note callout, though doesn't appear to violate the BIP as the BIP appears to be underspecified. We probably want to update BIP 174 to mention how version numbers are combined. --- doc/release-notes-16525.md | 4 +++- src/rpc/rawtransaction.cpp | 8 ++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/doc/release-notes-16525.md b/doc/release-notes-16525.md index f042c875f..220cb78de 100644 --- a/doc/release-notes-16525.md +++ b/doc/release-notes-16525.md @@ -4,4 +4,6 @@ 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). +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/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 0ab504de0..0abc2d26b 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -1566,7 +1566,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; @@ -1576,8 +1576,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) { @@ -1588,7 +1588,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