mining: preserve lookup result count without a mempool #36132

1. 
   l0rinc commented at 4:11 PM on August 31, 2026: contributor

   Problem: The mining interface returns one nullable transaction for every requested txid or wtxid so callers can match results by position. When its NodeContext has no mempool (not sure that's possible, but there's a condition for it which violates the contract), both lookup methods return an empty vector for every nonempty request and discard that positional mapping, see: https://github.com/bitcoin/bitcoin/blob/7b6f9ba7bad13b0c4169259000f7802854cdda0d/src/interfaces/mining.h#L202-L203

   Fix: Return a vector sized to the request when no mempool is available. Default-initialized null entries match the existing representation for identifiers absent from an available mempool, so callers receive one response position per request in either state.

2. DrahtBot added the label Mining on Aug 31, 2026
3. 
   DrahtBot commented at 4:11 PM on August 31, 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/36132.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

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

   Type	Reviewers
   ACK	Sjors
   Concept ACK	jeanpablojp, enirox001

   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-->

4. sedited requested review from Sjors on Aug 31, 2026
5. 
   jeanpablojp commented at 4:57 PM on September 2, 2026: contributor

   Concept ACK

   Built and ran the unit tests, and reverting just interfaces.cpp makes the new test fail.

6. test: characterize mining lookups without mempool f1e9197fbe
7. mining: preserve transaction lookup result count

   The mining interface returns one nullable transaction for every requested txid or wtxid so callers can match results by position.
   When a `NodeContext` has no mempool, both lookup methods return an empty vector and discard that positional mapping.
   
   Return a vector sized to the request in this state. Its default-initialized entries represent missing transactions, matching
   the existing result for identifiers absent from an available mempool.

   534aee84bf
8. in src/test/miner_tests.cpp:99 in ac2c6c4ef1

     95 | @@ -96,6 +96,22 @@ BOOST_FIXTURE_TEST_SUITE(miner_tests, MinerTestingSetup)
     96 |  
     97 |  static CFeeRate blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE);
     98 |  
     99 | +BOOST_AUTO_TEST_CASE(transaction_lookups_without_mempool)
   

   ---

   ---

   

   jeanpablojp commented at 4:57 PM on September 2, 2026:

   nit: the case builds its own empty NodeContext and stands on its own, which is nice, but the suite's MinerTestingSetup still gets built around it with nothing using it. chainstatemanager_args handles that by overriding the fixture on the case itself.

   BOOST_FIXTURE_TEST_CASE(transaction_lookups_without_mempool, BasicTestingSetup)
   

   ---

   

   l0rinc commented at 11:33 PM on September 2, 2026:

   Nice, I switched to BasicTestingSetup, rebased, thanks.

9. l0rinc force-pushed on Sep 2, 2026
10. 
    Sjors commented at 10:35 AM on September 4, 2026: member

    utACK 534aee84bfe813224cf1bbb5622c93137e8c713c

    An alternative is to throw if there's no mempool. That has the advantage of clients not being "misled" that we don't have a transaction. However, there's also a short period of time where the node has a mempool and it's still being populated from disk. In this alternative approach, that should throw too. That way the contract is: we give you the transaction if we have it, and otherwise we throw. But I'm fine with the simpler approach here.

11. DrahtBot requested review from jeanpablojp on Sep 4, 2026
12. 
    enirox001 commented at 2:28 PM on September 8, 2026: contributor

    Concept ACK

    I would understand the rationale behind this change, having the node return a list of nullptrs is better than having it return nothing, especially if the documentation already promises to at least return an entry per requested txid or wtxid

    For my understanding, is there a current production path where these lookups run without the mempool, given that IPC makeMining() waits for the chainstate to load? Or is this primarily keeping the interface contract consistent for internal callers and tests?

Contributors

l0rincDrahtBotjeanpablojpSjorsenirox001

Review Requested
Sjorsjeanpablojp