#36132 mining: preserve lookup result count without a mempool
https://github.com/bitcoin/bitcoin/pull/36132 · · +18/-2 in 2 files, 2 commits · labels: Mining
Goal
- Preserve positional indexing in transaction lookups via the mining interface when no mempool is present
- Prevent broken indexing expectations by returning null entries matching request size instead of empty lists
Updates `interfaces::Mining::getTransactionsByTxID` and `getTransactionsByWitnessID` to return a vector sized to the request with null entries when `NodeContext` lacks a mempool, rather than returning an empty vector. It also adds a unit test verifying this behavior.
Problem: The mining interface contract promises one nullable transaction per requested identifier to allow positional matching by callers. When no mempool is present, returning an empty vector breaks this positional indexing expectation.
Category: Mining (#12 of 13)
P4 · cleanup
- P4 because the missing mempool scenario cannot happen in production mining workflows
- MakeMining waits for mempool initialization, making this primarily an internal interface cleanup
P4 because the bug affects a scenario that reviewers note cannot actually occur in production mining workflows. As Sjors and enirox001 noted, `MakeMining` waits for chainstate and mempool initialization, making this primarily an internal interface consistency cleanup.
Membership: Modifies `interfaces::Mining` implementation in `src/node/interfaces.cpp` and adds a mining unit test.
Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0
Reviewability: Ready
- Ready for review: small and cleanly tested with design questions resolved in discussion
The patch is small, cleanly tested, and the alternative contract suggestion raised by a reviewer was answered in discussion.
Author status: active (addressed test fixture feedback and pushed an update)
Resolved concerns:
- Using `BasicTestingSetup` rather than inheriting unused `MinerTestingSetup` in the new unit test (jeanpablojp, fixed by author)
- Whether the interface should throw or redefine the contract on a missing mempool instead (optout21, Sjors)
Agreement: Strong
- Strong consensus on preserving interface contract consistency across reviewers
- Tested and supported because unifying unpopulated and missing mempool states aids callers (Sjors)
- Concept approval for interface consistency (jeanpablojp, enirox001)
- Alternative contract redefinition suggestion answered in discussion (optout21)
Strong consensus with an explicit utACK from Sjors and Concept ACKs, with contract alternatives resolved.
Reviewers agreed that returning null entries consistent with missing transactions is preferable to empty vectors or throwing, and the minor design question has been answered.
- Sjors utACKed with rationale explaining why unifying unpopulated and missing mempool states is preferable for callers
- jeanpablojp and enirox001 gave Concept ACKs supporting interface contract consistency
- optout21 asked if redefining the contract would be simpler; Sjors explained why hiding the mempool readiness distinction benefits clients
Review verdicts (DrahtBot): 1
- ACK: Sjors
- Concept ACK: jeanpablojp, enirox001
Files
16 lines under test/bench/ci.
- src/test/miner_tests.cpp +16/-0
- src/node/interfaces.cpp +2/-2
Card
This PR fixes `interfaces::Mining` lookup methods so that when no mempool is present, they return a vector matching the requested size filled with null pointers instead of an empty vector. This preserves the interface contract of 1:1 positional mapping between requested IDs and returned results. Reviewers note this scenario is largely theoretical in production because IPC and node startup initialize the mempool first, making this a minor consistency fix. Review state is positive with a utACK from Sjors, clean CI, and no open blockers.