Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix: #2389 Mempool walks, add try-mine CLI command #2392

Merged
merged 7 commits into from
Jan 29, 2021
Merged

Conversation

kantai
Copy link
Contributor

@kantai kantai commented Jan 29, 2021

This PR addresses the most immediate mempool issue #2389, by modifying the walk logic to "try again" if there are mempool entries received at a particular height but not in the same fork.

This PR also adds a try-mine CLI command, as well as a diagnostic envar MEMPOOL_BAD_BEHAVIOR which executes the mempool walk in the manner that causes #2389.

This leaves unaddressed two potential issues:

  1. Transactions received into the mempool at block A, but block A is "forked away". Those transactions will not appear in the miner's walk. However, such transactions will be viable for "replace-across-forks", such that if the transaction is rebroadcasted (not re-signed or fee upgraded), the miner will replace the existing tx in the mempool. (Issue: Mempool transactions accepted_at different forks are not considered #2394)
  2. Mempool transactions are evaluated in "reverse" order: transactions received later are attempted first. This can impact chained transactions, because higher nonces will be tried first and fail, before the lower nonces are tried. In degenerate cases, this can lead to chained transactions only being included one-per-block. (Issue: Mempool transactions should be evaluated in nonce-order, not walk-order #2393)

I think #2389 is much worse than those two issues at the moment, so I'm not sure if we should try to address those issues at the same time, or instead split it into multiple releases.

@kantai kantai marked this pull request as draft January 29, 2021 16:14
@kantai kantai requested review from jcnelson and lgalabru January 29, 2021 18:13
@kantai kantai marked this pull request as ready for review January 29, 2021 18:17
@kantai kantai linked an issue Jan 29, 2021 that may be closed by this pull request
@kantai kantai added the mining label Jan 29, 2021
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for tackling this! Just one minor comment.

Copy link
Contributor

@lgalabru lgalabru left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you @kantai!

@kantai kantai merged commit ee06519 into develop Jan 29, 2021
@pavitthrap pavitthrap mentioned this pull request Mar 17, 2021
4 tasks
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 24, 2024
@wileyj wileyj deleted the fix/mempool-walks branch March 11, 2025 21:38
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Miner implementation does not correctly walk mempool
4 participants