Skip to content

Disallow unconfirmed coinbase #1976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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!

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

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
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants