Skip to content

Conversation

@evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Jun 13, 2025

Description

The logic in CanonicalIter includes transactions anchored to blocks outside the best chain, since they may still appear in the mempool.

However, coinbase transactions can never be unconfirmed—a case the previous logic failed to exclude.

Notes to the reviewers

Sorry for my previous oversight on this.

Changelog notice

Fixed:
- During canonicalization, exclude coinbase transactions when considering txs that are anchored in stale blocks.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing

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

@evanlinjin evanlinjin self-assigned this Jun 13, 2025
@evanlinjin evanlinjin added the bug Something isn't working label Jun 13, 2025
Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need a rebase and this LGTM!

@evanlinjin evanlinjin force-pushed the fix/disallow-unconfirmed-coinbase branch from 5660720 to 6df8d14 Compare June 14, 2025 03:19
Copy link
Contributor

@LagginTimes LagginTimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6df8d14

The logic in `CanonicalIter` will consider txs that are anchored to
blocks not in the best chain since they still can appear in the mempool.

However, coinbase txs can never be unconfirmed - which the old logic
failed to exclude.
@evanlinjin evanlinjin force-pushed the fix/disallow-unconfirmed-coinbase branch from 6df8d14 to c84035e Compare July 10, 2025 06:39
Copy link
Member Author

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self-ACK c84035e

@evanlinjin evanlinjin merged commit d9c2b95 into bitcoindevkit:master Jul 10, 2025
19 checks passed
@oleonardolima oleonardolima mentioned this pull request Jul 31, 2025
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants