test: cover getrawtransaction on a stale block via txindex #36083

pull arejula27 wants to merge 1 commits into bitcoin:master from arejula27:txindex-functional-reorg-coverage changing 1 files +10 −0
  1. arejula27 commented at 6:03 PM on August 25, 2026: none

    <!-- *** Please remove the following help text before submitting: *** Pull requests may be closed immediately if they: - do not have a rationale and clear improvement - do not adhere to doc/AI_POLICY.md GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

    This checks the txindex as the only possible source for a transaction in a stale block. The response of getrawtransaction is checked to report that block as stale: confirmations 0, no time, no blocktime.

    It uses two transactions that cannot come back to the mempool once the block is disconnected: the block's coinbase, and a non-standard one, which is asserted to be out of the mempool. I didn't find an existing test that reaches the index this way.

  2. test: cover getrawtransaction on a stale block via txindex 5a2daedf07
  3. DrahtBot added the label Tests on Aug 25, 2026
  4. DrahtBot commented at 6:03 PM on August 25, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    Concept ACK l0rinc

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  5. maflcko commented at 7:35 PM on August 25, 2026: member

    I didn't find an existing test that reaches the index this way.

    The burden is on the author to prove new coverage. What mutation is this killing?

  6. arejula27 commented at 8:06 PM on August 25, 2026: none

    PR #35531 added the txindex_reorg_keeps_stale_entries unit test. It mines a transaction, invalidates its block and checks FindTx still returns it in the now stale block (then it do more more tests around like reorgs). However it calls FindTx directly, so it never goes through GetTransaction or TxToJSON.

    What this PR adds is the getrawtransaction output for a transaction the index serves from a stale block: the block hash, confirmations 0, and no time or blocktime. This test would guarantee that getrawtransaction keeps serving transactions from stale blocks through the index, and that the output for them cannot change silently.

    I also considered adding the case of a regular transaction in a stale and valid block (valid must win), but at the end i decided to not add it as that path is already in the unit test i mentioned before, and the RPC response for a regular transaction in a valid block is already covered here

  7. arejula27 commented at 11:07 PM on August 25, 2026: none

    TL;DR:

    If you modify the if else case at rawtransaction.cpp#L75 only the test added in the PR fails You can mutate the code like this to make it fail :

    const CBlockIndex* pindex = active_chainstate.m_blockman.LookupBlockIndex(hashBlock);
        if (pindex) {
            entry.pushKV("confirmations", 1 + active_chainstate.m_chain.Height() - pindex->nHeight);
            entry.pushKV("time", pindex->GetBlockTime());
            entry.pushKV("blocktime", pindex->GetBlockTime());
        }

    or keeping confirmations as it is and only moving time and blocktime:

            if (pindex) {
            entry.pushKV("time", pindex->GetBlockTime());
            entry.pushKV("blocktime", pindex->GetBlockTime());
            if (active_chainstate.m_chain.Contains(*pindex)) {
                entry.pushKV("confirmations", 1 + active_chainstate.m_chain.Height() - pindex->nHeight);
            }
            else
                entry.pushKV("confirmations", 0);
        }

    I ran every functional test that calls getrawtransaction and the full unit test suite against both patches and against master using my PR and master tests. rpc_rawtransaction.py is the only one failing, at assert 'time' not in gottx, and it only fails with this PR applied.

  8. DrahtBot added the label CI failed on Aug 26, 2026
  9. DrahtBot removed the label CI failed on Aug 26, 2026
  10. arejula27 commented at 4:17 PM on August 26, 2026: none

    I'm thinking checking the same on two transactions might be redundant (coinbase and non-standard), maybe coinbase is better as it will not brake in the future if the version is increased, but would like a second opinion

  11. l0rinc commented at 8:32 PM on August 26, 2026: contributor

    Concept ACK The coinbase seems sufficient to prove the index-only path since it cannot return to the mempool, the non-standard tx appears to exercise the same path.

  12. arejula27 commented at 10:49 PM on August 26, 2026: none

    I will delete the non standard tomorrow, seems useless (redundant)

  13. arejula27 force-pushed on Aug 29, 2026
  14. arejula27 commented at 5:16 PM on August 29, 2026: none

    Removed the duplicated scenario (non stadarrd), only conbiase is checked

  15. in test/functional/rpc_rawtransaction.py:174 in 5a2daedf07 
     170 | @@ -171,6 +171,16 @@ def getrawtransaction_tests(self):
 171 |              self.nodes[n].invalidateblock(block1)
 172 |              gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
 173 |              assert_equal(gottx['in_active_chain'], False)
 174 | +            if n == 0:
    l0rinc commented at 6:58 PM on August 29, 2026:

    n == 0 and n == 2 are acting as placeholders for whether txindex is enabled, which I find super-confusing. Could we first add a behavior-neutral refactoring commit that iterates over the node and an explicit txindex_enabled value (which would make the new test in the second commit easier to interpret)?

    <details><summary>name raw transaction test nodes</summary>

    diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index 78e12139fc..a30fb270f2 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -142,37 +142,37 @@ class RawTransactionsTest(BitcoinTestFramework):
         # Make a tx by sending, then generate 2 blocks; block1 has the tx in it
         tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid']
         block1, block2 = self.generate(self.nodes[2], 2)
-        for n in [0, 2]:
-            self.log.info(f"Test getrawtransaction {'with' if n == 0 else 'without'} -txindex, with blockhash")
+        for node, txindex_enabled in [(self.nodes[0], True), (self.nodes[2], False)]:
+            self.log.info(f"Test getrawtransaction {'with' if txindex_enabled else 'without'} -txindex, with blockhash")
             # We should be able to get the raw transaction by providing the correct block
-            gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
+            gottx = node.getrawtransaction(txid=tx, verbose=True, blockhash=block1)
             assert_equal(gottx['txid'], tx)
             assert_equal(gottx['in_active_chain'], True)
-            if n == 0:
+            if txindex_enabled:
                 self.log.info("Test getrawtransaction with -txindex, without blockhash: 'in_active_chain' should be absent")
                 for v in [1,2]:
-                    gottx = self.nodes[n].getrawtransaction(txid=tx, verbosity=v)
+                    gottx = node.getrawtransaction(txid=tx, verbosity=v)
                     assert_equal(gottx['txid'], tx)
                     assert 'in_active_chain' not in gottx
             else:
                 self.log.info("Test getrawtransaction without -txindex, without blockhash: expect the call to raise")
-                assert_raises_rpc_error(-5, err_msg, self.nodes[n].getrawtransaction, txid=tx, verbose=True)
+                assert_raises_rpc_error(-5, err_msg, node.getrawtransaction, txid=tx, verbose=True)
             # We should not get the tx if we provide an unrelated block
-            assert_raises_rpc_error(-5, "No such transaction found", self.nodes[n].getrawtransaction, txid=tx, blockhash=block2)
+            assert_raises_rpc_error(-5, "No such transaction found", node.getrawtransaction, txid=tx, blockhash=block2)
             # An invalid block hash should raise the correct errors
-            assert_raises_rpc_error(-3, "JSON value of type bool is not of expected type string", self.nodes[n].getrawtransaction, txid=tx, blockhash=True)
-            assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 6, for 'foobar')", self.nodes[n].getrawtransaction, txid=tx, blockhash="foobar")
-            assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 8, for 'abcd1234')", self.nodes[n].getrawtransaction, txid=tx, blockhash="abcd1234")
+            assert_raises_rpc_error(-3, "JSON value of type bool is not of expected type string", node.getrawtransaction, txid=tx, blockhash=True)
+            assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 6, for 'foobar')", node.getrawtransaction, txid=tx, blockhash="foobar")
+            assert_raises_rpc_error(-8, "parameter 3 must be of length 64 (not 8, for 'abcd1234')", node.getrawtransaction, txid=tx, blockhash="abcd1234")
             foo = "ZZZ0000000000000000000000000000000000000000000000000000000000000"
-            assert_raises_rpc_error(-8, f"parameter 3 must be hexadecimal string (not '{foo}')", self.nodes[n].getrawtransaction, txid=tx, blockhash=foo)
+            assert_raises_rpc_error(-8, f"parameter 3 must be hexadecimal string (not '{foo}')", node.getrawtransaction, txid=tx, blockhash=foo)
             bar = "0000000000000000000000000000000000000000000000000000000000000000"
-            assert_raises_rpc_error(-5, "Block hash not found", self.nodes[n].getrawtransaction, txid=tx, blockhash=bar)
+            assert_raises_rpc_error(-5, "Block hash not found", node.getrawtransaction, txid=tx, blockhash=bar)
             # Undo the blocks and verify that "in_active_chain" is false.
-            self.nodes[n].invalidateblock(block1)
-            gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
+            node.invalidateblock(block1)
+            gottx = node.getrawtransaction(txid=tx, verbose=True, blockhash=block1)
             assert_equal(gottx['in_active_chain'], False)
-            self.nodes[n].reconsiderblock(block1)
-            assert_equal(self.nodes[n].getbestblockhash(), block2)
+            node.reconsiderblock(block1)
+            assert_equal(node.getbestblockhash(), block2)
 
         self.log.info("Test getrawtransaction on genesis block coinbase returns an error")
         block = self.nodes[0].getblock(self.nodes[0].getblockhash(0))

    </details>

  16. in test/functional/rpc_rawtransaction.py:183 in 5a2daedf07 
     178 | +                stale_coinbase = self.nodes[n].getblock(block1)['tx'][0]
 179 | +                gottx = self.nodes[n].getrawtransaction(txid=stale_coinbase, verbose=True)
 180 | +                assert_equal(gottx['blockhash'], block1)
 181 | +                assert_equal(gottx['confirmations'], 0)
 182 | +                assert 'time' not in gottx
 183 | +                assert 'blocktime' not in gottx
    l0rinc commented at 7:00 PM on August 29, 2026:

    Instead of 4 separate lines deconstructing the internals, we could filter out the keys we want and compare that (since conceptually it's a single unit, we're comparing the state after)

                    assert_equal({key: gottx[key] for key in gottx.keys() & {'blockhash', 'confirmations', 'time', 'blocktime'}}, {'blockhash': block1, 'confirmations': 0})
  17. in test/functional/rpc_rawtransaction.py:177 in 5a2daedf07 
     170 | @@ -171,6 +171,16 @@ def getrawtransaction_tests(self):
 171 |              self.nodes[n].invalidateblock(block1)
 172 |              gottx = self.nodes[n].getrawtransaction(txid=tx, verbose=True, blockhash=block1)
 173 |              assert_equal(gottx['in_active_chain'], False)
 174 | +            if n == 0:
 175 | +                self.log.info("Test getrawtransaction with -txindex on a stale block, without blockhash")
 176 | +                # The stale block's coinbase cannot return to the mempool nor be mined
 177 | +                # again, so the index is the only source left for it.
    l0rinc commented at 7:01 PM on August 29, 2026:

    The message and code comment serve the same purpose, could be squashed:

                    self.log.info(f"Test getrawtransaction {'with' if txindex_enabled else 'without'} -txindex on a stale block, without blockhash")

    But now that we have an explicit txindex_enabled, could we exercise the stale coinbase lookup in both loop iterations:

    • with txindex, it should be found and report the stale block;
    • without txindex, the same lookup should raise the expected error.

    This would directly prove that txindex is the source, and the end result is even simpler (on top of the previous loop refactor):

    <details><summary>cover getrawtransaction on a stale block via txindex</summary>

    diff --git a/test/functional/rpc_rawtransaction.py b/test/functional/rpc_rawtransaction.py
index a30fb270f2..04b0a0b201 100755
--- a/test/functional/rpc_rawtransaction.py
+++ b/test/functional/rpc_rawtransaction.py
@@ -142,6 +142,7 @@ class RawTransactionsTest(BitcoinTestFramework):
         # Make a tx by sending, then generate 2 blocks; block1 has the tx in it
         tx = self.wallet.send_self_transfer(from_node=self.nodes[2])['txid']
         block1, block2 = self.generate(self.nodes[2], 2)
+        stale_coinbase = self.nodes[0].getblock(block1)['tx'][0]
         for node, txindex_enabled in [(self.nodes[0], True), (self.nodes[2], False)]:
             self.log.info(f"Test getrawtransaction {'with' if txindex_enabled else 'without'} -txindex, with blockhash")
             # We should be able to get the raw transaction by providing the correct block
@@ -171,6 +172,12 @@ class RawTransactionsTest(BitcoinTestFramework):
             node.invalidateblock(block1)
             gottx = node.getrawtransaction(txid=tx, verbose=True, blockhash=block1)
             assert_equal(gottx['in_active_chain'], False)
+            self.log.info(f"Test getrawtransaction {'with' if txindex_enabled else 'without'} -txindex on a stale block, without blockhash")
+            if txindex_enabled:
+                gottx = node.getrawtransaction(txid=stale_coinbase, verbose=True)
+                assert_equal({key: gottx[key] for key in gottx.keys() & {'blockhash', 'confirmations', 'time', 'blocktime'}}, {'blockhash': block1, 'confirmations': 0})
+            else:
+                assert_raises_rpc_error(-5, err_msg, node.getrawtransaction, txid=stale_coinbase, verbose=True)
             node.reconsiderblock(block1)
             assert_equal(node.getbestblockhash(), block2)

    </details>

  18. l0rinc changes_requested
  19. l0rinc commented at 7:08 PM on August 29, 2026: contributor

    I think this is useful coverage, especially since we were changing txindex internals recently. Existing tests cover stale entries in FindTx directly and stale transactions when a block hash is provided, but this test proves the RPC can retrieve a stale block’s coinbase without a block hash, making txindex its only possible source. It also checks that the response reports the stale block with zero confirmations and omits active-chain timestamps.

    My high-level suggestions are to first make the existing loop’s node and txindex configuration explicit, then exercise the stale coinbase lookup in both cases. The node with txindex should find it, while the node without txindex should return the expected error. This keeps the test compact and makes the new coverage straightforward to verify. Note: the PR description needs updating.

Contributors
arejula27DrahtBotmaflckol0rinc
Labels
Tests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-08-31 17:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me