kernel: expose transaction version #36283

nervana21 commented at 1:54 AM on September 17, 2026: contributor

This PR is a follow-up to #36194 and #36207. It completes the existing set of transaction accessors allowing clients to read the transaction version without slicing serialized bytes.

Tests cover a range of versions.

DrahtBot added the label Validation on Sep 17, 2026

DrahtBot commented at 1:55 AM on September 17, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36283.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	yuvicc, stickies-v, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

[src/test/kernel/test_kernel.cpp] BOOST_CHECK_THROW(Transaction{broken_tx_data}, std::runtime_error); -> consider BOOST_CHECK_EXCEPTION(..., std::runtime_error, ...) with a predicate that checks the expected failure message, instead of only asserting the exception type.

nervana21 commented at 1:55 AM on September 17, 2026: contributor

cc: @lucasdbr05 @KY-U

in src/test/kernel/test_kernel.cpp:483 in 4397e8647a

 475 | @@ -475,6 +476,15 @@ BOOST_AUTO_TEST_CASE(btck_transaction_tests)
 476 |      check_equal(script_pubkey_roundtrip.ToBytes(), script_pubkey.ToBytes());
 477 |  }
 478 |  
 479 | +BOOST_AUTO_TEST_CASE(btck_transaction_version_tests)
 480 | +{
 481 | +    auto tx_data{hex_string_to_byte_vec("02000000013f7cebd65c27431a90bba7f796914fe8cc2ddfc3f2cbd6f7e5f2fc854534da95000000006b483045022100de1ac3bcdfb0332207c4a91f3832bd2c2915840165f876ab47c5f8996b971c3602201c6c053d750fadde599e6f5c4e1963df0f01fc0d97815e8157e3d59fe09ca30d012103699b464d1d8bc9e47d4fb1cdaa89a1c5783d68363c4dbc4b524ed3d857148617feffffff02836d3c01000000001976a914fc25d6d5c94003bf5b0c7b640a248e2c637fcfb088ac7ada8202000000001976a914fbed3d9b11183209a57999d54d59f67c019e756c88ac6acb0700")};
 482 | +    for (const uint32_t version : {0u, 1u, 2u, 3u, 0xffffffffu}) {
 483 | +        WriteLE32(tx_data.data(), version);

stickies-v commented at 9:40 AM on September 17, 2026:

We try to keep dependencies for test_kernel on internal headers minimal, so in that light including crypto/common.h just to write a byte seems excessive. Perhaps concatenating strings or something like tx_data[i] = std::byte(version >> (8 * i)) could be an alternative?

Also, transaction version is a scalar member like locktime etc. I think this can just be folded into btck_transaction_tests.

stickies-v commented at 9:40 AM on September 17, 2026: contributor

Code LGTM 4397e8647a27d40fbc26d29b17bbedded5692009 but test should be improved a bit

yuvicc commented at 12:02 PM on September 17, 2026: contributor

Concept ACK

KY-U commented at 3:01 PM on September 17, 2026: contributor

GetVersion() is provided by TransactionApi to both Transaction and TransactionView, but the current test only exercises the owning type. Would it be usefull to also check it on the existing transaction view, so both wrapper instantiations are covered?

<details> <summary>diff testing both wrappers, moving the test to btck_transaction_tests and without crypto/common.h as suggested by [@stickies-v](/bitcoin-bitcoin/contributor/stickies-v/)</summary>

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 08c7c1e5f3..1df9b9fbd1 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -2,7 +2,6 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <crypto/common.h>
 #include <kernel/bitcoinkernel.h>
 #include <kernel/bitcoinkernel_wrapper.h>
 #include <util/byte_units.h>
@@ -410,6 +409,19 @@ BOOST_AUTO_TEST_CASE(btck_transaction_tests)
     BOOST_CHECK_EQUAL(tx.CountOutputs(), 2);
     BOOST_CHECK_EQUAL(tx.CountInputs(), 1);
     BOOST_CHECK_EQUAL(tx.GetLocktime(), 510826);
+
+    auto versioned_tx_data{tx_data};
+    for (const uint32_t version : {0u, 1u, 2u, 3u, 0xffffffffu}) {
+        for (size_t i = 0; i < 4; ++i) {
+            versioned_tx_data[i] = std::byte((version >> (8 * i)) & 0xff);
+        }
+
+        Transaction versioned_tx{versioned_tx_data};
+        TransactionView versioned_tx_view{versioned_tx.get()};
+
+        BOOST_CHECK_EQUAL(versioned_tx.GetVersion(), version);
+        BOOST_CHECK_EQUAL(versioned_tx_view.GetVersion(), version);
+    }
     auto broken_tx_data{std::span<std::byte>{tx_data.begin(), tx_data.begin() + 10}};
     BOOST_CHECK_THROW(Transaction{broken_tx_data}, std::runtime_error);
     auto input{tx.GetInput(0)};
@@ -476,14 +488,6 @@ BOOST_AUTO_TEST_CASE(btck_transaction_tests)
     check_equal(script_pubkey_roundtrip.ToBytes(), script_pubkey.ToBytes());
 }
 
-BOOST_AUTO_TEST_CASE(btck_transaction_version_tests)
-{
-    auto tx_data{hex_string_to_byte_vec("02000000013f7cebd65c27431a90bba7f796914fe8cc2ddfc3f2cbd6f7e5f2fc854534da95000000006b483045022100de1ac3bcdfb0332207c4a91f3832bd2c2915840165f876ab47c5f8996b971c3602201c6c053d750fadde599e6f5c4e1963df0f01fc0d97815e8157e3d59fe09ca30d012103699b464d1d8bc9e47d4fb1cdaa89a1c5783d68363c4dbc4b524ed3d857148617feffffff02836d3c01000000001976a914fc25d6d5c94003bf5b0c7b640a248e2c637fcfb088ac7ada8202000000001976a914fbed3d9b11183209a57999d54d59f67c019e756c88ac6acb0700")};
-    for (const uint32_t version : {0u, 1u, 2u, 3u, 0xffffffffu}) {
-        WriteLE32(tx_data.data(), version);
-        BOOST_CHECK_EQUAL(Transaction{tx_data}.GetVersion(), version);
-    }
-}
 
 BOOST_AUTO_TEST_CASE(btck_transaction_id_tests)
 {

</details>

I've added an extra & 0xff on byte conversion to make it explicit that only the least significant byte of the shifted value is used, though this is purely stylistic.

nervana21 force-pushed on Sep 17, 2026

nervana21 commented at 4:28 PM on September 17, 2026: contributor

Thanks for the reviews. I've updated the code to address feedback on the testing logic.

in src/test/kernel/test_kernel.cpp:425 in a40947d1c5

 420 | +        TransactionView versioned_tx_view{versioned_tx.get()};
 421 | +
 422 | +        BOOST_CHECK_EQUAL(versioned_tx.GetVersion(), version);
 423 | +        BOOST_CHECK_EQUAL(versioned_tx_view.GetVersion(), version);
 424 | +    }
 425 | +

stickies-v commented at 7:10 AM on September 18, 2026:

nit: I'm not sure testing 0, 1, 2, and 3 adds much value. 0, 2, and max seems plenty. We also generally don't test parity between Handle and View for every accessor, so far. I'm not sure it adds much value?

Suggested simplification:

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index e20203b810..1dd816fe28 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -15,6 +15,7 @@
 #include <test/kernel/block_data.h>
 #include <test/util/common.h>
 
+#include <algorithm>
 #include <charconv>
 #include <concepts>
 #include <cstdint>
@@ -410,17 +411,11 @@ BOOST_AUTO_TEST_CASE(btck_transaction_tests)
     BOOST_CHECK_EQUAL(tx.CountInputs(), 1);
     BOOST_CHECK_EQUAL(tx.GetLocktime(), 510826);
 
-    auto versioned_tx_data{tx_data};
-    for (const uint32_t version : {0u, 1u, 2u, 3u, 0xffffffffu}) {
-        for (size_t i{0}; i < 4; ++i) {
-            versioned_tx_data[i] = std::byte(version >> (8 * i));
-        }
-
-        Transaction versioned_tx{versioned_tx_data};
-        TransactionView versioned_tx_view{versioned_tx.get()};
-
-        BOOST_CHECK_EQUAL(versioned_tx.GetVersion(), version);
-        BOOST_CHECK_EQUAL(versioned_tx_view.GetVersion(), version);
+    BOOST_CHECK_EQUAL(tx.GetVersion(), 2);
+    for (const auto& [version_hex, version] : {std::pair{"00000000", 0u}, {"ffffffff", 0xffffffffu}}) {
+        auto versioned_tx_data{tx_data};
+        std::ranges::copy(hex_string_to_byte_vec(version_hex), versioned_tx_data.begin());
+        BOOST_CHECK_EQUAL(Transaction{versioned_tx_data}.GetVersion(), version);
     }
 
     auto broken_tx_data{std::span<std::byte>{tx_data.begin(), tx_data.begin() + 10}};

</details>

stickies-v approved

stickies-v commented at 7:11 AM on September 18, 2026: contributor

ACK a40947d1c50193d3e09d17b39c79e78cec46f252

kernel: expose transaction version

Expose CTransaction::version, allowing clients to read the transaction
version without slicing serialized bytes.

Test the accessor against a range of transaction versions.

83434f7683

nervana21 force-pushed on Sep 18, 2026

nervana21 commented at 5:51 PM on September 18, 2026: contributor

Updates:

Simplified the test based on @stickies-v's comment

yuvicc commented at 5:05 AM on September 19, 2026: contributor

lgtm! ACK 83434f7683e45afc1ebbb7690a6ae288b2602b03

DrahtBot requested review from stickies-v on Sep 19, 2026

stickies-v commented at 11:15 AM on September 21, 2026: contributor

ACK 83434f7683e45afc1ebbb7690a6ae288b2602b03

sedited approved

sedited commented at 11:48 AM on September 21, 2026: contributor

ACK 83434f7683e45afc1ebbb7690a6ae288b2602b03

sedited merged this on Sep 21, 2026

sedited closed this on Sep 21, 2026

nervana21 deleted the branch on Sep 21, 2026