Fire TransactionRemovedFromMempool callbacks from mempool #14384

l2a5b1 · 2018-10-03T20:43:23Z

This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.

It also resolves the txmempool -> validation -> validationinterface -> txmempool circular dependency.

Ideally, validationinterface is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving txmempool specific code out of validationinterface to txmempool where it belongs.

DrahtBot · 2018-10-03T22:11:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16688 (log: Add validation interface logging by jkczyz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

src/txmempool.cpp

src/validationinterface.h

src/validationinterface.cpp

practicalswift · 2018-10-04T09:37:15Z

Concept ACK

Let's get rid of these ugly circular dependencies! :-)

jamesob · 2019-11-18T18:27:37Z

ACK 37fd92b

This is basically some helpful code-shuffling; the change removes unneeded code to absorb the TransactionRemovedFromMempool signal into the existing CValidationInterface callback registration code, and avoids including mempool-specific header files into the more general validationinterface.

jonatack · 2019-11-18T18:43:45Z

Concept ACK, kudos @l2a5b1 for staying with this.

Built 37fd92b rebased onto current master at 3052130, tests pass, bitcoind running.

Now reviewing the code.

jonatack

ACK 37fd92b

Nice cleanup that improves separation of concerns and enables removing MempoolEntryRemoved, RegisterWithMempoolSignals, and UnregisterWithMempoolSignals.

TheBlueMatt · 2019-11-18T20:31:57Z

This isn't strictly worse, but its still super awkward....CTxMemPool is (mostly) a "dumb datastructure", with all the adding/removing happening in validation.cpp. Currently its double awkward cause they're signals that go back to validation so that validation can fire appropriate events, but just firing the events inside a datastructure is also really awkward. Is there an equally-easy way to move the event-firing back into validation.cpp?

jnewbery · 2019-11-18T21:14:06Z

Is there an equally-easy way to move the event-firing back into validation.cpp?

I don't think there is. That would require that every site that validation calls a CTxMemPool method that could result in transactions being removed would need to be passed back a vector of transactions that it should fire the events for. A quick search shows at least the following sites would need to be updated:

validation's UpdateMempoolForReorg() calling into mempool.removeRecursive()
validation's LimitMempoolSize() calling into pool.Expire()
validation's LimitMempoolSize() calling into pool.TrimToSize()

There are probably more. I don't think those call sites have ever handled firing the events, and adding that functionality would be new data structures, function signatures and logic.

This seems to be a strict improvement to me. The events are being fired from exactly the same place as before, except that there's no weird indirection in the validation interface.

maflcko · 2019-11-19T19:30:10Z

Re-run ci

TheBlueMatt · 2019-11-21T02:46:39Z

Right, certainly don't interpret my comment as a NACK, then. Longer-term it would be nice, indeed, to move the event-firing into those callsites, cause the responsibility right now is very awkwardly split (hence why we're checking the reason in the first place - validation fires some similar things), but this isn't worse than master and if other PR's need this, do it!

laanwj · 2019-11-21T18:42:19Z

argh, needs trivial rebase for the linter test/lint/lint-circular-dependencies.sh

This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.

l2a5b1 · 2019-11-22T06:43:11Z

argh

@laanwj lol, no worries, rebased e20c72f

jnewbery · 2019-11-22T14:59:48Z

ACK e20c72f

e20c72f Fire TransactionRemovedFromMempool from mempool (251) Pull request description: This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency. Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs. ACKs for top commit: jnewbery: ACK e20c72f Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3

ariard · 2019-11-22T16:16:28Z

ACK e20c72f, only rebased.

jonatack · 2019-11-22T17:13:07Z

Posthumous ACK e20c72f

…m mempool e20c72f Fire TransactionRemovedFromMempool from mempool (251) Pull request description: This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency. Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs. ACKs for top commit: jnewbery: ACK e20c72f Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3

The reason parameter was removed in bitcoin#14384 as part of a clean-up but the reason would be useful to log according to comments on the PR.

Summary: This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. This is a backport of Core [[bitcoin/bitcoin#14384 | PR14384]] policyestimator_tests was modified to use TestingSetup because it uses the `CTxMemPool::TrimToSize` function which now may trigger the signal, but the signals handlers are not setup properly to do so using `BasicTestingSetup`. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6451

…m mempool e20c72f Fire TransactionRemovedFromMempool from mempool (251) Pull request description: This pull request fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. It also resolves the `txmempool -> validation -> validationinterface -> txmempool` circular dependency. Ideally, `validationinterface` is a dumb component that doesn't have any knowledge of the sub-systems it sends its notifications to. The commit that aims to resolve this circular dependency by moving `txmempool` specific code out of `validationinterface` to `txmempool` where it belongs. ACKs for top commit: jnewbery: ACK e20c72f Tree-SHA512: 354c3ff1113b21a0b511d80d604edfe3846dddae3355e43d1387f68906e54bf5dc01e7c029edc0b8e635b500b2ab97ee50362e2486eb4319f7347ee9a9e6cef3

…fication fix f2734f0 [Tests] Fix policyestimator and validation_block tests (random-zebra) 3448bc8 wallet: Minimal fix to restore conflicted transaction notifications (Antoine Riard) ba9d84d feature_notifications.py mimic upstream is_wallet_compiled() function to make future back ports easier. (furszy) 37c9944 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery) 5cc619f [validation] Remove pool member from ConnectTrace (John Newbery) 1e453b7 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery) e774bfd [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery) 389680a [validation interface] Remove vtxConflicted from BlockConnected (furszy) 19c9383 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery) 139882d Fire TransactionRemovedFromMempool from mempool This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code. (furszy) Pull request description: 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. ACKs for top commit: Fuzzbawls: ACK f2734f0 random-zebra: re-utACK f2734f0 and merging... Tree-SHA512: 07d437e0d5e5d9798b16d982b6f58585d1f0dcd82251c5ac06f47e44233433831243d451ce43068e33c6002a64b76fa4d165167a2cbaeeedc28cde2019853565

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch 2 times, most recently from c22e3aa to 249df58 Compare October 3, 2018 21:09

This was referenced Oct 3, 2018

Document intentional and unintentional unsigned integer overflows (wraparounds) using annotations #14224

Closed

Utxoscriptindex #14035

Closed

fanquake added the Refactoring label Oct 3, 2018

This was referenced Oct 3, 2018

refactor: Replace boost::bind with std::bind #13743

Merged

Refactor: separate wallet from node #10973

Merged

Multiprocess bitcoin #10102

Draft

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from 249df58 to a60a187 Compare October 4, 2018 07:25

practicalswift reviewed Oct 4, 2018

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Oct 4, 2018

View reviewed changes

src/validationinterface.h Outdated Show resolved Hide resolved

practicalswift reviewed Oct 4, 2018

View reviewed changes

src/validationinterface.cpp Outdated Show resolved Hide resolved

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch 6 times, most recently from 48d4bb2 to 1c2b458 Compare October 4, 2018 20:23

DrahtBot mentioned this pull request Oct 9, 2018

Refactor: Start to separate wallet from node #14437

Merged

DrahtBot mentioned this pull request Oct 23, 2018

Move util files to directory #14555

Merged

DrahtBot added the Needs rebase label Nov 5, 2018

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from 1c2b458 to e729c6f Compare November 5, 2018 17:11

DrahtBot removed the Needs rebase label Nov 5, 2018

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from e729c6f to 1ac729f Compare November 5, 2018 21:36

DrahtBot added the Needs rebase label Nov 9, 2018

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from 1ac729f to e72cd85 Compare November 13, 2018 16:17

DrahtBot removed the Needs rebase label Nov 13, 2018

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from e72cd85 to b58bbc9 Compare November 13, 2018 18:28

jonatack reviewed Nov 18, 2019

View reviewed changes

maflcko closed this Nov 19, 2019

maflcko reopened this Nov 19, 2019

DrahtBot added the Needs rebase label Nov 21, 2019

Fire TransactionRemovedFromMempool from mempool

e20c72f

This commit fires TransactionRemovedFromMempool callbacks from the mempool and cleans up a bunch of code.

l2a5b1 force-pushed the patch/validationinterface-resolve-circular-dependencies branch from 37fd92b to e20c72f Compare November 21, 2019 20:30

DrahtBot removed the Needs rebase label Nov 21, 2019

laanwj merged commit e20c72f into bitcoin:master Nov 22, 2019

jkczyz mentioned this pull request Nov 22, 2019

log: Add validation interface logging #16688

Merged

jkczyz added a commit to jkczyz/bitcoin that referenced this pull request Nov 22, 2019

Log MemPoolRemovalReason in validation interface

34d6486

The reason parameter was removed in bitcoin#14384 as part of a clean-up but the reason would be useful to log according to comments on the PR.

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

This was referenced Mar 9, 2021

Killing wallet and GUI cs_main locks PIVX-Project/PIVX#2209

Merged

[wallet] Conflicted transactions external listeners notification fix PIVX-Project/PIVX#2241

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Fire TransactionRemovedFromMempool callbacks from mempool #14384

Fire TransactionRemovedFromMempool callbacks from mempool #14384

Uh oh!

Conversation

l2a5b1 commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Oct 4, 2018

Uh oh!

jamesob commented Nov 18, 2019

Uh oh!

jonatack commented Nov 18, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 18, 2019

Uh oh!

jnewbery commented Nov 18, 2019

Uh oh!

maflcko commented Nov 19, 2019

Uh oh!

TheBlueMatt commented Nov 21, 2019

Uh oh!

laanwj commented Nov 21, 2019

Uh oh!

l2a5b1 commented Nov 22, 2019

Uh oh!

jnewbery commented Nov 22, 2019

Uh oh!

ariard commented Nov 22, 2019

Uh oh!

jonatack commented Nov 22, 2019

Uh oh!

Uh oh!

l2a5b1 commented Oct 3, 2018 •

edited

Loading

DrahtBot commented Oct 3, 2018 •

edited

Loading