mining: add getTransactions(ByWitnessID) IPC methods

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

The Txid variant, just like its RPC counterpart, takes advantage of -txindex if enabled. The Wtxid variant can’t do that unless we add a -witnesstxindex, but at least sv2 has no use for that.

The RPC additionally allows providing a block_hash hint. I haven’t implemented that here because I don’t have a need for it. It can always be added later.

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

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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34860 (mining: always pad scriptSig at low heights, drop include_dummy_extranonce by Sjors)
#34644 (mining: add submitBlock to IPC Mining interface by w0xlt)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#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):

await template.submitSolution(ctx, 0, 0, 0, b"") in test/functional/interface_ipc_mining.py
await template.submitSolution(ctx, 0, 0, 0, b"\x00") in test/functional/interface_ipc_mining.py

2026-03-31 18:06:50

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.

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.

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:

0//! [*] The Cap'n Proto wire format actually does distinguish between null and
1//! empty Data values inside Lists, and the C++ API does allow distinguishing
2//! between null and empty Data values in other contexts, just not the List
3//! 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:

0    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 0: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.

mempool: add lookup by witness hash

Add a simple test for both the Txid and Wtxid variants of GetTransaction().

ddb7aca3c6

ipc: Serialize null CTransactionRef as empty Data

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

9b6c6fcb98

test: restart node after IPC option override test

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

a92f575f04

mining: add getTransactionsByTxID() IPC method cf0b42755f

mining: add getTransactionsByWitnessID() IPC method a626172516

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

mining: add getTransactions(ByWitnessID) IPC methods #34020

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)