Skip to content
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

fix(chain): filter coinbase tx not in best chain #1202

Merged

Conversation

LagginTimes
Copy link
Contributor

@LagginTimes LagginTimes commented Nov 9, 2023

Description

Fixes #1144.
Coinbase transactions cannot exist in the mempool and be unconfirmed. TxGraph::try_get_chain_position should always return None for coinbase transactions not anchored in best chain.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo 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

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

The code looks good to me.

In #1144, @evanlinjin says:

A proper test has not been written for this scenario. The test would mine blocks which has the coinbase send to a tracked/owned spk. A reorg should not result in an unconfirmed balance.

Are we ok with the test that this PR includes, or better to also add the one evan is suggesting?

crates/chain/tests/test_tx_graph_conflicts.rs Outdated Show resolved Hide resolved
@notmandatory notmandatory added the bug Something isn't working label Nov 10, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Nov 10, 2023
Coinbase transactions cannot exist in the mempool and be unconfirmed.
`TxGraph::try_get_chain_position` should always return `None` for coinbase
transactions not anchored in best chain.
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 991cb77

I verified new test fails without this fix. If an additional test is needed and not easy to do with current framework can we make a new issue and put in the beta milestone?

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

ACK 991cb77

@danielabrozzoni danielabrozzoni merged commit f382fa9 into bitcoindevkit:master Nov 14, 2023
11 checks passed
@notmandatory notmandatory mentioned this pull request Jan 3, 2024
12 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.

Coinbase transaction that is not anchored in the best chain should always be filtered out
3 participants