mining: add getTransactions(ByWitnessID) IPC methods #34020

Sjors commented at 7:13 PM on December 5, 2025: member

For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.

The spec doesn't specify how this is achieved, but one method is to call the getrawtransaction RPC on each transaction id listed in DeclareMiningJob (or a subset if the pool software maintains a cache). Using RPC is inefficient, made worse by the need to make multiple calls. It also doesn't support queuing by witness id (yet, see #34013).

This PR introduces two new IPC methods:

getTransactionsById(): takes a list of Txid's
getTransactionsByWitnessID(): : takes a list of Wtxid's

Both return a list of serialised transactions. An empty element is returned for transactions that were not found.

Unlike the RPC counterpart, the IPC methods do not take advantage of -txindex. This could be done in a followup. For Wtxid that would involve adding a -witnesstxindex.

I thought about having a single (or overloaded) getTransactions() that works with both Txid and Wtxid, but I prefer that clients are intentional about which one they want.

A unit and functional test cover the new functionality.

Sv2 probably only needs getTransactionsByWitnessID(), but it's easy enough to just add both.

To rest with Rust use:

https://github.com/2140-dev/bitcoin-capnp-types/pull/11

[^0]: there's two reasons the pool requests these transactions: to approve the template and to broadcast the block if a solution is found (the miner will also broadcast via their template provider). See also https://github.com/stratum-mining/sv2-spec/issues/170

DrahtBot added the label Mining on Dec 5, 2025

DrahtBot commented at 7:13 PM on December 5, 2025: 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/34020.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sedited, ViniciusCestarii
Concept ACK	enirox001

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35084 (ipc: Support for windows support by ryanofsky)
#34860 (mining: always pad scriptSig at low heights, drop include_dummy_extranonce by Sjors)
#34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#32387 (ipc: add windows support by ryanofsky)
#29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
#19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
#19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
#10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

submitSolution(ctx, 0, 0, 0, b"") in test/functional/interface_ipc_mining.py

2026-04-27 13:10:21

DrahtBot added the label CI failed on Dec 5, 2025

DrahtBot commented at 8:32 PM on December 5, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/19973417058/job/57283535788 LLM reason (✨ experimental): Lint failure: a subtree directory was touched without performing a proper subtree merge.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

ryanofsky commented at 1:01 AM on December 8, 2025: contributor

figure out Treat std::shared_ptr nullptr as empty Data bitcoin-core/libmultiprocess#235 (this PR is draft pending that, subtree linter failure is expected)

This turned out to be an interesting problem, because in general it's not safe to treat empty Data fields the same as unset values. For example with std::optional<std::string> or std::string* we would want to interpret empty Data fields as empty strings not null values. Even for serializable types it's not really safe to treat empty Data the same as null since empty Data could be a valid serialization for some objects. For CTransaction objects though it is safe.

Another annoying thing is that if the goal is to be able to serialize vector<CTransactionRef> as List(Data) and allow the vector to contain null values, the most obvious way to do it would be to map null CTransactionRef values to null data values. But annoyingly although the Cap'n Proto wire format can represent null Data values in a list and has APIs to set null Data values and has APIs to distinguish between null and empty Data values in non-list contexts, it doesn't currently provide any way to distinguish between null and empty Data values when the Data value is in a List.

So your workaround of interpreting empty Data values as null CTransactionRef is probably the best approach for now. I put together an implementation that should be more robust in b14778374b0694f7255f774cb657c7049cb4256e (tag). It limits the workaround to CTransactionRef data fields only and avoids making any assumptions in type-pointer.h about the type of pointers. It also includes a unit test that makes sure null values can be passed correctly.

Sjors force-pushed on Dec 8, 2025

Sjors commented at 8:44 AM on December 8, 2025: member

@ryanofsky thanks, I swapped in your commit (subtree linter is still expected to fail).

So your workaround of interpreting empty Data values as null CTransactionRef is probably the best approach for now.

One reason I have some confidence in this approach is that the Python functional tests are able to read the result. Although it interprets the empty fields as b'' rather than None, that shouldn't be an issue for client developers. This is captured by your note:

//! [*] The Cap'n Proto wire format actually does distinguish between null and
//! empty Data values inside Lists, and the C++ API does allow distinguishing
//! between null and empty Data values in other contexts, just not the List
//! context, so this limitation could be removed in the future.

If this is fixed one day and we correctly encode null on the wire, it would technically be a breaking API change, but trivial to deal with for clients. E.g. the functional test would check for both b'' and None.

b14778374b0694f7255f774cb657c7049cb4256e looks good to me, can you make a libmultiprocess PR, and another PR here that bumps the subtree to use it and add the tests? I'll rebase on that.

DrahtBot added the label Needs rebase on Dec 16, 2025

Sjors commented at 12:06 PM on December 19, 2025: member

Needs rebase after #34003, but I'll do that after there's a multiprocess PR.

in src/ipc/capnp/mining.capnp:22 in 0ce5aba23c

  18 | @@ -19,6 +19,8 @@ interface Mining $Proxy.wrap("interfaces::Mining") {
  19 |      waitTipChanged @3 (context :Proxy.Context, currentTip: Data, timeout: Float64) -> (result: Common.BlockRef);
  20 |      createNewBlock @4 (options: BlockCreateOptions) -> (result: BlockTemplate);
  21 |      checkBlock @5 (block: Data, options: BlockCheckOptions) -> (reason: Text, debug: Text, result: Bool);
  22 | +    getTransactions @6 (context :Proxy.Context, txids: List(Data)) -> (result: List(Data));

plebhash commented at 1:43 PM on February 13, 2026:

    getTransactionsByTxID [@6](/bitcoin-bitcoin/contributor/6/) (context :Proxy.Context, txids: List(Data)) -> (result: List(Data));

Sjors referenced this in commit fb2622ff5e on Feb 20, 2026

Sjors referenced this in commit e8bcca3be6 on Feb 20, 2026

Sjors referenced this in commit 423a789eb4 on Feb 20, 2026

Sjors force-pushed on Feb 20, 2026

Sjors commented at 10:37 AM on February 20, 2026: member

I opened https://github.com/bitcoin-core/libmultiprocess/pull/242 and this PR now pulls that in.

Also rebased and renamed getTransactions to getTransactionsByTxID.

Oh oops, #34568 got merged right before I pushed this. Will rebase again.

Sjors force-pushed on Feb 20, 2026

DrahtBot removed the label Needs rebase on Feb 20, 2026

Sjors force-pushed on Feb 20, 2026

Sjors commented at 11:46 AM on February 20, 2026: member

p2p_opportunistic_1p1c.py failure seems spurious: https://github.com/bitcoin/bitcoin/actions/runs/22222027229/job/64279933038?pr=34020

Sjors referenced this in commit 413f915979 on Feb 21, 2026

Sjors force-pushed on Feb 21, 2026

Sjors commented at 10:59 AM on February 23, 2026: member

Test failure is expected: the subtree update pulled in https://github.com/bitcoin-core/libmultiprocess/pull/240 without the corresponding changes from #34422, so I'll rebase once that lands.

DrahtBot added the label Needs rebase on Feb 24, 2026

Sjors referenced this in commit 2d3cc775e6 on Feb 27, 2026

Sjors referenced this in commit 97d877053b on Mar 4, 2026

Sjors force-pushed on Mar 5, 2026

Sjors commented at 12:39 AM on March 5, 2026: member

Rebased after #34422 and using the latest https://github.com/bitcoin-core/libmultiprocess/pull/242. I added a separate test for run_transaction_lookup_test instead of modifying the existing tests in test/functional/interface_ipc_mining.py.

DrahtBot removed the label CI failed on Mar 5, 2026

DrahtBot removed the label Needs rebase on Mar 5, 2026

Sjors commented at 3:22 PM on March 5, 2026: member

I opened https://github.com/2140-dev/bitcoin-capnp-types/pull/11 for easier Rust testing.

Sjors referenced this in commit aa804580ee on Mar 5, 2026

Sjors referenced this in commit df8ed54aa5 on Mar 5, 2026

Sjors referenced this in commit d7d22ec00f on Mar 5, 2026

Sjors referenced this in commit ebe2996baa on Mar 6, 2026

ryanofsky referenced this in commit 8a5e3ae6ed on Mar 9, 2026

Sjors referenced this in commit 9a8d8614a0 on Mar 10, 2026

Sjors referenced this in commit 99ff226379 on Mar 11, 2026

Sjors force-pushed on Mar 11, 2026

Sjors commented at 2:51 PM on March 11, 2026: member

https://github.com/bitcoin-core/libmultiprocess/pull/242 landed, so this PR now simply updates the libmultiprocess subtree. We'll probably have another regular subtree update for 31.x bug fixes (see #34804), so I'll wait for that, rebase, drop the subtree update here, and then mark ready for review.

Sjors referenced this in commit 2cd3cd1dc3 on Mar 11, 2026

in src/node/interfaces.cpp:1014 in 12dac5f4e7 outdated

1007 | @@ -1008,6 +1008,33 @@ class MinerImpl : public Mining
1008 |          return state.IsValid();
1009 |      }
1010 |  
1011 | +    std::vector<CTransactionRef> getTransactionsByTxID(const std::vector<Txid>& txids) override
1012 | +    {
1013 | +        if (!m_node.mempool) return {};
1014 | +        LOCK(m_node.mempool->cs);

ViniciusCestarii commented at 6:35 PM on March 19, 2026:

In getTransactionsByTxID and getTransactionsByWitnessID, is the outer LOCK(m_node.mempool->cs) necessary? Both GetTransaction overloads call mempool->get() internally which already acquires the lock itself.

In the case of getTransactionsByTxID, this also means cs_mempool is held while GetTransaction falls through to g_txindex->FindTx() for txids not found in the mempool, causing unnecessary contention during disk I/O.

What was the reasoning for taking the lock at this level rather than leaving it to GetTransaction?

Sjors commented at 1:14 PM on March 20, 2026:

My understanding is that taking and releasing a lock comes with overhead, so it's better to do it once and get the whole loop over with. However I haven't benchmarked this.

You're right though that it doesn't make sense for the indexer fallback, since that's looking at blocks.

So I'm inclined to drop the outer lock. At least for getTransactionsByTxID(), since there's no witness index yet for getTransactionsByWitnessID().

in src/node/interfaces.cpp:1013 in 12dac5f4e7 outdated

1007 | @@ -1008,6 +1008,33 @@ class MinerImpl : public Mining
1008 |          return state.IsValid();
1009 |      }
1010 |  
1011 | +    std::vector<CTransactionRef> getTransactionsByTxID(const std::vector<Txid>& txids) override
1012 | +    {
1013 | +        if (!m_node.mempool) return {};

ViniciusCestarii commented at 6:42 PM on March 19, 2026:

In getTransactionsByTxID and getTransactionsByWitnessID, the docstring in mining.h states "one entry per requested txid... otherwise nullptr" but doesn't mention the case where m_node.mempool is null, which returns an empty vector instead.

If someone iterates over the results assuming results.size() == txids.size(), they'd get unexpected behavior without any indication from the interface that this case exists.

Sjors commented at 1:18 PM on March 20, 2026:

This should never happen, except perhaps during a brief moment in node startup. But I should either document that or investigate if it's safe to Assert() instead.

sedited commented at 1:30 PM on March 20, 2026: contributor

I'm not sure about adding this to the mining interface. Is this function going to be duplicated by the chain, gui, or wallet interface in the future? How are we going to handle similar overlapping scope creep in the future?

Sjors commented at 1:45 PM on March 20, 2026: member

Is this function going to be duplicated by the chain, gui, or wallet interface in the future?

No, I would expect (non mining) clients who need it to just use the mining interface.

There is a broader question here: do we want to proceed with the #10102 approach of separating the GUI, wallet and node along existing interface lines? Or do we continue to build new interfaces around use cases such as mining, indexers and independent wallet implementations? Or both.

However, as long as the Mining interface is marked experimental, we can always move these new methods elsewhere to completely change how they work in order to avoid duplication.

Sjors force-pushed on Mar 31, 2026

Sjors commented at 6:07 PM on March 31, 2026: member

Rebased after the subtree update in #34804, which includes the change from https://github.com/bitcoin-core/libmultiprocess/pull/242 that this PR needed.

Also rebased past #34727 which add a submitSolution() test. That test proved very useful because it reveals that I forgot to modify the implementation, which 3064197bb576bfebed12f54b8b79fea6c9d8643c ipc: Serialize null CTransactionRef as empty Data made reachable for a nullptr coinbase. I expanded the test to cover both an empty coinbase and an (actually) unparseable one (0x00).

I changed the locking behavior as discussed here: #34020 (review)

I have a branch that addresses #34020 (review), replacing the existing early return with a client error and expanding it to check mempool->GetLoadTried(). That way, an empty result means: it's not in the mempool. Rather than: maybe it's not in the mempool, or maybe we're still loading mempool.dat, come back later. However I got a bit carried away writing a test for it, so that's going to be a different PR: #34970.

Although we could just add such an error without test coverage, I prefer stick to the early return for now.

Sjors marked this as ready for review on Mar 31, 2026

in src/node/interfaces.cpp:1018 in cf0b42755f

1011 | @@ -1012,6 +1012,20 @@ class MinerImpl : public Mining
1012 |          return state.IsValid();
1013 |      }
1014 |  
1015 | +    std::vector<CTransactionRef> getTransactionsByTxID(const std::vector<Txid>& txids) override
1016 | +    {
1017 | +        if (!m_node.mempool) return {};
1018 | +        LOCK(m_node.mempool->cs);

sedited commented at 11:47 AM on April 26, 2026:

Can you lock after the memory allocations (here and in the wtxid implementation)? It also feels weird that we are locking here and using GetTransaction. Might GetTransaction be the wrong tool for the query anyway? Seems weird that we would hit the txindex through this function, but not the witness one. I realize you mention this in the PR description, but I am having a hard time rationalizing supporting index queries as part of a mining interface. How about calling mempool.get(...) directly?

It also seems like a footgun to call GetTransaction from a loop that itself locks the mempool. GetTransaction can lock cs_main, which could invert the usual locking order.

Sjors commented at 9:21 AM on April 27, 2026:

Can you lock after the memory allocations (here and in the wtxid implementation)? It also feels weird that we are locking here and using GetTransaction

This was also brought up here: #34020 (review)

I thought I already dropped the outer lock in ...ByTxID(, but I hadn't. I've now dropped it for both methods.

I am having a hard time rationalizing supporting index queries as part of a mining interface.

Addressed that in the main thread, below.

How about calling mempool.get(...) directly?

I'm trying to avoid low level methods in interface.cpp. With the outer lock gone, this should be fine now?

sedited commented at 9:41 AM on April 27, 2026:

I think I would prefer a mempool member function that locks and returns the required transactions in one go. Isn't there also a race condition without the lock being held while iterating? You now have the potential of returning a transaction and its replacement, which seems confusing?

If there ever is the need for querying the txindex through this, we can simply add that later, no? Adding flexibility that nobody is currently asking for while paying for a potential locking contention penalty seems not ideal. We'll have other interfaces for non-mining clients eventually.

Sjors commented at 10:33 AM on April 27, 2026:

You now have the potential of returning a transaction and its replacement, which seems confusing?

Confusing yes, though it's just one of many ways a block can be invalid. Getting a consistent mempool image is not the goal of this method. Rather, the goal is to be able to construct a block based on (witness) transaction ids.

The mempool is one source of data, we could also have it iterate the extra pool, orphanage, etc.

Sjors commented at 1:11 PM on April 27, 2026:

As of 313aaa0eae4446533b3668360ae0a353cde63312 I ended up dropping -txindex support and taking the lock outside the loop again.

in src/test/mempool_tests.cpp:10 in ddb7aca3c6

   6 | @@ -7,12 +7,16 @@
   7 |  #include <test/util/txmempool.h>
   8 |  #include <txmempool.h>
   9 |  #include <util/time.h>
  10 | +#include <node/transaction.h>

sedited commented at 12:03 PM on April 26, 2026:

Nit (clang-format): This in include is not in sorted order.

Sjors commented at 9:21 AM on April 27, 2026:

Fixed

Sjors force-pushed on Apr 27, 2026

Sjors commented at 9:21 AM on April 27, 2026: member

@sedited wrote (https://github.com/bitcoin/bitcoin/pull/34020#discussion_r3143425195):

I am having a hard time rationalizing supporting index queries as part of a mining interface.

Afaik it's not necessary for mining clients.

Non-mining clients may find it useful, as a drop-in replacement for the getrawtransaction RPC. That gets into the discussion of #34981. An alternative would be to have a second getTransactionsById() method on a different interface that does use the index, but that just seems confusing.

An argument against including it, is that it slows down misses. Though if that's really an issue, it can be addressed with a boolean use_index argument.

Sjors commented at 10:34 AM on April 27, 2026: member

I wrote:

Afaik it's not necessary for mining clients.

Maybe there is, though it's more nice-to-have than essential.

Searching the index may be slower than searching the mempool, but it's still a lot faster than having to request the transaction from an upstream miner. This should only happen if the miner tip is behind the pool tip, or on a fork, and there are ways to detect that earlier.

Update: but then again, the sv2 spec was modified in https://github.com/stratum-mining/sv2-spec/pull/171 to switch from txid to wtxid, so they don't benefit from the index in any case. So I'm inclined to drop it.

sedited approved

sedited commented at 12:21 PM on April 27, 2026: contributor

ACK 6e4359d0726e39b50cc81cda4769ed25905b4aee

mempool: add lookup by witness hash

Add a simple test for both the Txid and Wtxid variants of CTxMemPool::get().

493ff82bad

ipc: Serialize null CTransactionRef as empty Data

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

0a87e34f69

test: restart node after IPC option override test

Add cleanup so it doesn't need to be the last test.

aa08e3a0a2

mining: add getTransactionsByTxID() IPC method 8a1078da62

mining: add getTransactionsByWitnessID() IPC method 313aaa0eae

Sjors force-pushed on Apr 27, 2026

Sjors commented at 1:10 PM on April 27, 2026: member

I dropped -txindex support and switched to more direct mempool.get() since the GetTransaction wrappers became a bit overkill. I modified mempool_tests.cpp to keep the new coverage without those wrappers.

Sjors referenced this in commit 1e203498e9 on Apr 27, 2026

sedited approved

sedited commented at 1:23 PM on April 27, 2026: contributor

Re-ACK 313aaa0eae4446533b3668360ae0a353cde63312

enirox001 commented at 2:45 PM on April 27, 2026: contributor

Concept ACK

I took a close look through this and the design makes sense to me.

The PR adds direct mempool transaction lookup to the mining IPC interface with either txid or wtxid , and the null-CTransactionRef serialization change is what makes it possible to preserve positional results across the IPC boundary when a lookup misses.

The mempool-side changes are straightforward as well, and the tests cover the main behavior I’d want to see here such as txid/wtxid lookup, misses, and transactions no longer being returned once mined

ViniciusCestarii commented at 3:09 PM on April 27, 2026: contributor

tACK 313aaa0

Ran tests and tested via a standalone IPC client (separate process, Cap'n Proto over Unix socket) calling both getTransactionsByTxID and getTransactionsByWitnessID against a mainnet node.

Verified that each method:

returns the transaction for a known-present ID
returns a nullptr entry for a not present ID

DrahtBot requested review from enirox001 on Apr 27, 2026