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, 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 Current IPC clients cannot reach this state: makeMining() waits for chainstate loading after mempool creation, and shutdown disconnects IPC clients before resetting the mempool.

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

   External sites

   • Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36132
   • Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36132.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36132#author-ralph

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

13. 
    Sjors commented at 12:35 PM on September 10, 2026: member

    @enirox001 I also suspect it can't happen.

14. 
    optout21 commented at 1:44 PM on September 10, 2026: contributor

    In this special case the contract ("one entry per requested ID") is not satisfied.

    Is there any report of this causing a problem with users?

    Even if the contract is satisfied (as done in this PR), the callers should still verify the size of the returned array before accessing elements, it's not safe to rely on the contract and access elements without length checks. By this logic, wouldn't be it easier to just change the contract, and specify that "a returned vector that has less elements than IDs requested should be treated as error"? This would not mean any extra work for the callers. The code could stay as it is.

15. 
    Sjors commented at 3:10 PM on September 10, 2026: member

    "a returned vector that has less elements than IDs requested should be treated as error"

    There's three states:

    1. mempool not ready (zero length result)
    2. mempool ready but still reading from disk (normal length result, but all empty)
    3. mempool fully read from disk (normal length result, empty results mean actually missing txs)

    The difference between (1) and (2) is not interesting to the client, the previous code requires clients to distinguish them, this PR hides the distinction.

    The difference between (2) and (3) is interesting, but we're not revealing it. Doing so would be a more involved change (probably not worth doing).

16. 
    l0rinc commented at 11:39 AM on October 4, 2026: contributor

    is there a current production path where these lookups run without the mempool

    Is there any report of this causing a problem with users?

    Current IPC callers can't hit this. I changed it because it contradicts the documented contract, rather than in response to a user report. I've updated the PR description to clarify that.

Contributors

l0rincDrahtBotjeanpablojpSjorsenirox001optout21

Review Requested
Sjorsjeanpablojp