Skip to content

fix(chain): track spends and conflicts on null prevout for non-coinbase txs - #2435

Closed
AlphaSparrow wants to merge 2 commits into
bitcoindevkit:masterfrom
AlphaSparrow:fix/null-prevout-conflicts
Closed

AlphaSparrow wants to merge 2 commits into
bitcoindevkit:masterfrom
AlphaSparrow:fix/null-prevout-conflicts

Conversation

@AlphaSparrow

Copy link
Copy Markdown

Description

Fixes #2286.

TxGraph::insert_tx previously skipped every null previous output (OutPoint::null()) unconditionally when indexing spends, under the assumption that any null prevout implied the transaction was a coinbase.

However, under Bitcoin consensus rules, a transaction is only a coinbase if it has exactly one input and that input has a null outpoint (tx.is_coinbase()). A malformed transaction with a null prevout and another input is not a coinbase, yet its null input was skipped. Two such transactions sharing only the null prevout were therefore not recorded in spends, missed by direct_conflicts, and both erroneously admitted to CanonicalView.

This PR:

  • Updates TxGraph::insert_tx to skip indexing spends only if tx.is_coinbase(). Non-coinbase transactions have all their inputs indexed, including null prevouts.
  • Updates TxGraph::direct_conflicts to ignore coinbases, as valid coinbases have no previous outputs and cannot conflict on inputs.
  • Adds regression tests in crates/chain/tests/test_null_outpoint.rs verifying conflict detection on null prevouts, eviction in CanonicalView, and isolation from valid coinbases.
  • Includes a CI commit resolving the recent MSRV dependency breakage (yoke-derive pin drop) and check-wasm runner pin (ubuntu-22.04).

Notes to the reviewers

Valid coinbases remain strictly isolated from conflict tracking. Non-coinbase transactions (even malformed consensus-invalid ones) now have their spends indexed consistently so direct_conflicts and CanonicalView correctly detect conflicts.

Changelog notice

Fixed

Checklists

All Submissions:

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

…se txs

TxGraph::insert_tx previously skipped every null previous output unconditionally when indexing spends, under the assumption that any null prevout implied the transaction was a coinbase.

However, under Bitcoin consensus rules, a transaction is only a coinbase if it has exactly one input and that input has a null outpoint (tx.is_coinbase()). A malformed transaction with a null prevout and another input is not a coinbase, yet its null input was skipped. Two such transactions sharing only the null prevout were therefore not recorded in spends, missed by direct_conflicts, and both erroneously admitted to CanonicalView.

This change:
- Updates TxGraph::insert_tx to skip indexing spends only if tx.is_coinbase(). Non-coinbase transactions have all their inputs indexed, including null prevouts.
- Updates TxGraph::direct_conflicts to ignore coinbases, as valid coinbases have no previous outputs and cannot conflict on inputs.
- Adds regression tests covering null prevout conflicts on non-coinbase txs, conflict pruning in CanonicalView, and isolation from valid coinbases.

Fixes bitcoindevkit#2286

Signed-off-by: Sparrow <theaayushsakariya@gmail.com>
…heck-wasm

Drop yoke-derive pin from ci/pin-msrv.sh now that yoke-derive 0.8.4 has been released with rust-version 1.82 and fixes compatibility with Rust 1.85.0.

Pin check-wasm runner to ubuntu-22.04 to prevent apt-key missing error and package restart hangs on newer Ubuntu releases.

Signed-off-by: Sparrow <theaayushsakariya@gmail.com>
@AlphaSparrow
AlphaSparrow requested a review from a team as a code owner October 23, 2026 12:15
@github-project-automation github-project-automation Bot moved this to Triage in BDK Chain Oct 23, 2026
@evanlinjin

Copy link
Copy Markdown
Member

@AlphaSparrow can you explain how a valid non-coinbase tx will have a null prevout?

@AlphaSparrow

Copy link
Copy Markdown
Author

@evanlinjin yeah you're right, it can't, as per the bitcoin consensus rules (bad-txns-prevout-null), any non-coinbase txn with a null prevout is invalid and would never be accepted by a node, let alone make it into the block/mempool.

I picked up #2286 to fix the internal conflict tracking inside TxGraph without properly taking into account the bitcoin consensus rules on the actual network. I noticed calculate_fee checks tx.is_coinbase(), whereas insert_tx was using prevout.is_null(), so I thought aligning them and resolving the edge case in #2286 made sense from a pure data structure standpoint, but i overlooked that the consensus already prevents these transactions from ever reaching a wallet in practice.

Given it's redundant in the real world, I'm completely fine closing this PR and #2286 .
(Happy to see the MSRV fix in the 2nd commit already helping CI fixes in #2438!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

TxGraph doesn't index null-prevout spends of non-coinbase txs, so their conflicts go undetected

2 participants