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:

getTransactions(): 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 or 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.

TODO:

figure out https://github.com/bitcoin-core/libmultiprocess/issues/235 (this PR is draft pending that, subtree linter failure is expected)

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

mempool: add lookup by witness hash

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

2ae8ab5c08

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:

#34003 (test: interface_ipc.py minor fixes and cleanup by ryanofsky)
#33965 (mining: fix -blockreservedweight shadows IPC option by Sjors)
#33936 (mining: pass missing context to createNewBlock() and checkBlock() by Sjors)
#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 typos and grammar issues:

represeent -> represent [spelling error in “represeent null pointers” makes the sentence harder to read]

2025-12-08

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.

ipc: Serialize null CTransactionRef as empty Data 2ac21ab471

mining: add getTransactions() IPC method a76888193b

mining: add getTransactionsByWitnessID() IPC method 0ce5aba23c

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

DrahtBot commented at 3:26 pm on December 16, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

mining: add getTransactions(ByWitnessID) IPC methods #34020

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)