#36132 mining: preserve lookup result count without a mempool

ReviewabilityReviewsAgreementSize
#36132 l0rinc mining: preserve lookup result count without a mempoolReady1Strong2 + 16 tests
  • Keep mining transaction lookups consistent with caller expectations when no mempool is loaded
  • Ensure callers can safely index results by position without special-casing an empty mempool
  • Ready to review
  • Substantive support for preserving caller indexing expectations without a mempool
  • Verified by running tests and checking that reverting the fix causes failures (jeanpablojp)
  • Prefers returning empty entries over throwing errors when no mempool is loaded (Sjors)
  • Agrees it preserves documented contracts better than returning an empty list (enirox001)
  • 2 lines added or modified outside tests
  • 16 lines added or modified in tests
  • 2 lines removed in total
  • 2 files, 2 commits
Mining#16 of 17
P4 · bug fix
  • P4 after comparing with the other PRs here (assessed alone as P3): Lowered to P4: reviewers confirm the no-mempool lookup path is unreachable in production, making it a contract-consistency tidy-up on par with #36451 rather than a real bug fix.
  • P3 because callers relying on positional indexing get consistent results without a mempool
  • The benefit is minor since current production paths do not encounter lookups without a mempool
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

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.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • reviewer: re-review following author's clarification on IPC callers and user reports 2026-10-04
  • reviewer: optout21 to review replies from Sjors and author 2026-10-04

Agreement

Strong

substantive support, no open objection (jeanpablojp, Sjors, enirox001)

Thread notes: Reclassified schema 1 objections as suggestions, added questions from enirox001 and optout21, and marked them answered following Sjors's explanation and the author's reply in c:5979544252. / Converted jeanpablojp's test fixture nit and optout21's contract redesign comment from objections to suggestions under the current rules, added enirox001's and optout21's questions, and recorded the answers from Sjors and l0rinc.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
jeanpablojp (contributor)suggestion, maintenanceuse BasicTestingSetup for the test case so MinerTestingSetup is not built unnecessarilyfixed · fix pushed · author switched to BasicTestingSetup and rebased in rc:3919651258answeredyes2026-09-02: 'the suite's `MinerTestingSetup` still gets built around it with nothing using it'
Status from: 2026-09-02: l0rinc: 'Nice, I switched to `BasicTestingSetup`, rebased, thanks.'
enirox001 (contributor)question, scopeIs there a current production path where lookups run without the mempool, or is this primarily keeping interface contracts consistent for internal callers and tests?answered · Sjors agreed it likely cannot happen, and author confirmed IPC callers cannot hit itansweredyes2026-09-08: 'is there a current production path where these lookups run without the mempool, given that IPC `makeMining()` waits for the chainstate to load?'
Status from: 2026-10-04: l0rinc: 'Current IPC callers can't hit this. I changed it because it contradicts the documented contract, rather than in response to a user report.'
optout21 (contributor)suggestion, approachredefine the contract to specify that returning fewer elements than requested is an error, leaving the code as isanswered · Sjors explained that hiding the distinction between mempool readiness states is cleaner for callersansweredyes2026-09-10: '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.'
Status from: 2026-09-10: Sjors: '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.'
optout21 (contributor)question, usefulnessIs there any report of this causing a problem for users?answered · author confirmed the change was made to fix contract contradiction rather than from a user reportansweredyes2026-09-10: 'Is there any report of this causing a problem with users?'
Status from: 2026-10-04: l0rinc: 'Current IPC callers can't hit this. I changed it because it contradicts the documented contract, rather than in response to a user report.'

Support:

  • jeanpablojp (contributor) (Concept ACK, 2026-09-02) [both: interfaces.cpp]: built and ran the unit tests, and verified that reverting interfaces.cpp makes the new test fail
  • Sjors (member) (ACK, 2026-09-04) [both]: prefers the simpler approach of returning nullptrs over throwing when there is no mempool, which would also require handling the mempool disk-load state
  • enirox001 (contributor) (Concept ACK, 2026-09-08) [read]: returning a list of nullptrs better maintains the documented contract of returning an entry per requested ID than returning nothing

Participants:

  • jeanpablojp (contributor, support, 2 statements 2026-09-02 to 2026-09-02): Concept ACK after testing; suggested using BasicTestingSetup for the unit test
  • Sjors (member, support, 3 statements 2026-09-04 to 2026-09-10): ACK; explained mempool readiness states and why hiding the distinction is useful
  • enirox001 (contributor, support, 1 statements 2026-09-08 to 2026-09-08): Concept ACK; asked whether production paths ever run without a mempool
  • optout21 (contributor, neutral, 1 statements 2026-09-10 to 2026-09-10): suggested redefining the contract instead of changing code, and asked if users had reported issues

Files

16 lines under test/bench/ci.

  • src/test/miner_tests.cpp +16/-0
  • src/node/interfaces.cpp +2/-2

Card

The short text the ranking pass compares across PRs.

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.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-05T01:17, confidence high, input hash 168974e26f8ea778
thread read through 2026-10-04 (9 statements), head 534aee84bf
labels: Mining

Categories

Mining

P4 · bug fix (assessed alone as P3; Lowered to P4: reviewers confirm the no-mempool lookup path is unreachable in production, making it a contract-consistency tidy-up on par with #36451 rather than a real bug fix.)

Fixes an interface contract violation in `interfaces::Mining` when no mempool is present. The interface contract promises one nullable transaction per requested identifier for positional matching, but returning an empty vector breaks caller indexing expectations.

Membership: Has the Mining label and modifies `interfaces::Mining` methods in `src/node/interfaces.cpp`.

Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0