Always clean up manual transaction prioritization #6464
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
concept ACK - will review & test in depth |
Member
|
Prioritisations should never be removed until a transaction is mined. It's intentionally not on CTxMemPoolEntry because we may not have (or be willing to accept) the transaction until after prioritising it. Although it does make sense to remove the entry if a conflicting transaction is confirmed. |
Member
Contributor
Author
Contributor
Author
|
So I think in that case it's just a simple one line change to clear prioritizations when a conflicting transaction is mined. |
Member
|
Seems reasonable to me. |
Contributor
Author
|
@luke-jr, could you take a look and make sure that it's now doing the right thing? |
Contributor
|
Untested ACK. |
Member
|
Looks reasonable, utACK |
laanwj
added a commit
that referenced
this pull request
Oct 27, 2015
2d8c49d Clean up tx prioritization when conflict mined (Casey Rodarmor)
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
Nov 18, 2015
Github-Pull: bitcoin#6464 Rebased-From: 2d8c49d
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
Dec 8, 2015
Github-Pull: bitcoin#6464 Rebased-From: 2d8c49d
furszy
referenced
this pull request
in PIVX-Project/PIVX
Jan 30, 2021
…llet 98d770f CScheduler boost->std::function, use millisecs for times, not secs (Matt Corallo) 67e068c Remove now unneeded ChainTip signal (furszy) bcdd3e9 Move ChainTip sapling update witnesses and nullifiers to BlockConnected/BlockDisconnected. (furszy) b799070 Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo) d77244c Remove dead-code tracking of requests for blocks we generated (Matt Corallo) 10ccbbf Hold cs_wallet for whole block [dis]connection processing (Matt Corallo) 1a45036 fix compiler warning member functions not marked as 'override' (furszy) d3867a7 An adaptation of Corallo's btc@461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 (Matt Corallo) f5ac648 [Refactor] Move Sapling ChainTip signal notification loop to use the latest connectTrace class structure (furszy) 8704d9d Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo) 6dcb6b0 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo) 50d3e0e Handle conflicted transactions directly in ConnectTrace (furszy) 27fb897 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo) 60329da Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo) e7c2789 Include missing #include in zmqnotificationinterface.h (Matt Corallo) 1b396b8 Move `NotifyEntryRemoved` to use the general interfaces::MakeHandler (furszy) 4cb5820 Better document usage of SyncTransaction (Alex Morcos) 21be4e2 Introduce MemPoolConflictRemovalTracker (Alex Morcos) 7326acb mempool: add notification for added/removed entries (Wladimir J. van der Laan) a8605d2 Clean up tx prioritization when conflict mined (Casey Rodarmor) e7db9ff remove internal tracking of mempool conflicts for reporting to wallet (Alex Morcos) 76c72c6 remove external usage of mempool conflict tracking (Alex Morcos) ef429af tests: Stop node before removing the notification file (furszy) 15a21c2 tests: write the notification to different files to avoid race condition (Chun Kuan Lee) 466e97a [Wallet] Simplify InMempool (furszy) 85e18f0 Rename FlushWalletDB -> CompactWalletDB, add function description (Matt Corallo) 00f36ea Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (Matt Corallo) Pull request description: Revamped the validation interface interaction with the wallet, encapsulating and improving the mempool and block signaling and each of the wallet signals handler. Restructured the Sapling witnesses and nullifiers update under the new signals. Solved several bugs that found on the way as well (Check each commit description). This PR concludes with #1726 long road :). Pushing our repository around 2 years ahead in btc time in the mempool and validation interface areas (without including RBF). The new validation -> wallet interaction architecture will enable further, and much more user facing important, improvements for the syncing process, overall software responsiveness and multithreading locking issues. Adapted backports: dashpay#6464 --> Always clean up manual transaction prioritization (mempool) bitcoin#9240 --> Remove txConflicted (mempool) bitcoin#9371 --> Notify on removal (mempool) bitcoin#9605 --> Use CScheduler for wallet flushing, remove ThreadFlushWalletDB (walletdb) bitcoin#9725 --> CValidationInterface Cleanups (wallet, validation and validation interface) ACKs for top commit: random-zebra: utACK 98d770f Fuzzbawls: utACK 98d770f Tree-SHA512: 84c86567c2bff36b859b2ae73c558956a70dff0fffb4f73208708d92165b851bf42d35246410238c66c7a4b77e5bf51ec93885234a75fa48901fd182d2f70a28
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #4818
An alternative fix is to add the transaction prioritization information to CTxMemPoolEntry itself, instead of storing it separately. This would add 128 bits to every transaction in the mempool, which seems undesirable, given that the majority of transactions won't have a manual transaction prioritization delta.
@morcos