[wallet] Conflicted transactions external listeners notification fix#2241
Merged
random-zebra merged 10 commits intoApr 14, 2021
Merged
Conversation
|
Can be rebased now. |
furszy
force-pushed
the
2020_conflicted_tx_notification_fix
branch
from
a118a95 to
f2ae007
Compare
Author
|
Took me a bit to rebase but done, so many changes included in master since i created this one. |
|
There are two minor issues in the tests:
|
Author
|
commit cherry-picked 👍 |
random-zebra
previously approved these changes
Apr 9, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
ACK 467888f0176e1548bce4f875a1a327affeaf8efd
…nsactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. Adaptation of btc@e20c72f9f076681def325b5b5fa53bccda2b0eab
The only CValidationInterface client that cares about transactions that are removed from the mempool because of CONFLICT is the wallet. Start using the TransactionRemovedFromMempool method to notify about conflicted transactions instead of using the vtxConflicted vector in BlockConnected.
The wallet now uses TransactionRemovedFromMempool to be notified about conflicted wallet, and no other clients use vtxConflicted. Adaptation of btc@cdb893443cc16edf974f099b8485e04b3db1b1d7
Since we don't add a vtxConflicted vector to BlockConnected the conflictedTxs member of PerBlockConnectTrace is no longer used.
ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved callback to be notified of transactions removed for conflict. Since PerBlockConnectTrace no longer tracks conflicted transactions, ConnectTrace no longer requires these notifications
It's no longer used for anything.
NotifyEntryAdded never had any subscribers so can be removed. Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are now no subscribers. The CValidationInterface TransactionAddedToMempool and TransactionRemovedFromMempool methods can now provide this functionality. There's no need for a special notifications framework for the mempool.
… to make future back ports easier.
furszy
force-pushed
the
2020_conflicted_tx_notification_fix
branch
from
467888f to
f2734f0
Compare
Author
|
Rebased on master, conflicts solved. Ready to go. |
Fuzzbawls
approved these changes
Apr 14, 2021
random-zebra
approved these changes
Apr 14, 2021
random-zebra
left a comment
random-zebra
left a comment
There was a problem hiding this comment.
re-utACK f2734f0 and merging...
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
PR built on top of #2209, #2235 and #2240 (don't get scared by the amount of commits, most of them are coming from 2209. Will disappear as soon as that one gets merged).
Based on the brief talk originated in #2209 (comment) .
Solving the conflicted transactions external listeners notification. Adapting the following PRs: bitcoin#14384, bitcoin#17477 and bitcoin#18982. Without functional test support, for the time being, for the lack of RBF functionality.