backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 -- deglobalisation of mempool #5169

knst · 2023-01-22T02:46:33Z

This pull request is part of deglobalisation of mempool.
Merged backports:

This backports does not work correctly without #5174 and #5175

How Has This Been Tested?

Run unit/functional test and introduce no breaking changes.

Checklist:

I have performed a self-review of my own code
I have assigned this pull request to a milestone

PastaPastaPasta · 2023-01-30T00:43:12Z

IMO, the first two non-backport commits should be in another PR

knst · 2023-01-30T09:56:31Z

IMO, the first two non-backport commits should be in another PR

Please, review #5175 and #5174

#5175) ## Issue being fixed or feature implemented Previously noticed, that GetTransaction is called deep inside governance objects and it is true indeed. But so far it is used `nullptr` as a mempol object instead any real mempool in GetTransaction and no usage of a global mempool or any other mempool, this locks are useless. This changes are important for mempool globalization. ## What was done? Removed LOCK for ::mempool.cs in governance's code ## How Has This Been Tested? Run unit/functional tests. Also done deglobalization of mempool to validate that governance indeed doesn't use global mempool implicit in #5169 ## Breaking Changes No breaking changes ## Checklist: - [x] I have performed a self-review of my own code - [x] I have assigned this pull request to a milestone

github-actions · 2023-01-31T17:29:26Z

This pull request has conflicts, please rebase.

src/rpc/mining.cpp

src/txmempool.cpp

src/validationinterface.h

UdjinM6

utACK

github-actions · 2023-02-03T22:27:03Z

This pull request has conflicts, please rebase.

knst · 2023-02-04T16:05:00Z

@knst knst force-pushed the bc-bp-mempool branch from 85741eb to 81931f9

Resolved conflicts with net_processing's refactorins in develop branch

src/coinjoin/client.cpp

src/coinjoin/client.h

github-actions · 2023-02-06T19:59:48Z

This pull request has conflicts, please rebase.

knst · 2023-02-06T20:03:34Z

@knst knst force-pushed the bc-bp-mempool branch from 280a8a3 to a0ce905

Resolved conflict with #5178 + bumped 1st commit

src/wallet/salvage.cpp

UdjinM6 · 2023-02-06T20:29:29Z

pls squash d77e96d into 6e878be (19556)

UdjinM6

re-utACK

github-actions · 2023-02-11T02:27:35Z

This pull request has conflicts, please rebase.

UdjinM6

re-utACK

kwvg

utACK

PastaPastaPasta

utACK for merging via merge commit

1b46a48 [cleanup] Remove unused CReserveKey (John Newbery) 9819ad6 [rpc] simplify generate RPC (John Newbery) Pull request description: Removes dead code from after the generate method was removed ACKs for commit 1b46a4: MarcoFalke: utACK 1b46a48 meshcollider: utACK bitcoin@1b46a48 scravy: utACK 1b46a48 Empact: utACK bitcoin@1b46a48 Tree-SHA512: d1fab1bf76ac3036b85cf33be89868bc016f912575545ecaa16f958397b0ec4f1ce4de8fe254d4f21aabeea9c83a8928530cc520de26af0d1a8bdb4ca0f2cb77

… depend on global args fad691c rpc: Make verifychain default values static, not depend on global args (MarcoFalke) Pull request description: This fixes several issues: * The documentation is not compile-time static and depends on run-time arguments, making it impossible to host it on a static resource like a website or pdf. See also a similar change in the wallet rpc code: bitcoin#18499 * The same call (relying on default values) will run different code on different machines, depending on the command line args that were used to start the server. This might lead to hard-to-debug-remote issues. This is a small behaviour change, and I will add release notes. ACKs for top commit: theStack: ACK bitcoin@fad691c promag: Code review ACK fad691c. Tree-SHA512: 1c7a253ff0ec13a973b10d3777b71c70954ded5805b65a3ab06317327014de4cd0601d71d30c6ce89a581722c150cb5567acc1bd3e0c789cb51bab6ef0dcfc4a

fa9ee52 doc: Add doxygen comment to IsRBFOptIn (MarcoFalke) faef4fc Remove mempool global from interfaces (MarcoFalke) fa83168 refactor: Add IsRBFOptInEmptyMempool (MarcoFalke) Pull request description: The chain interface has an `m_node` member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See bitcoin#19556 ACKs for top commit: jnewbery: utACK fa9ee52 darosior: ACK fa9ee52 hebasto: re-ACK fa9ee52, since my [previous](bitcoin#19848 (review)) review: Tree-SHA512: 11b4c1446f0860a743fdaa67f95c52bf0262d0a4f888be0eaf07ee497448965d32be414111bf016bd568f2989cde923430e3a3889e224057b73c499f06de7199

…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

…EntryRemoved signals e57980b [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery) 2dd561f [validation] Remove pool member from ConnectTrace (John Newbery) 969b65f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery) 5613f98 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery) cdb8934 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery) 1168394 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery) Pull request description: These boost signals were added in bitcoin#9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback. Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions. Note that bitcoin#9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in bitcoin#16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR. ACKs for top commit: jonatack: Re-ACK e57980b ryanofsky: Code review ACK e57980b, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69

fa0572d Pass mempool reference to chainstate constructor (MarcoFalke) Pull request description: Next step toward bitcoin#19556 Instead of relying on the mempool global, each chainstate is given a reference to a mempool to keep up to date with the tip (block connections, disconnections, reorgs, ...) ACKs for top commit: promag: Code review ACK fa0572d. darosior: ACK fa0572d hebasto: ACK fa0572d, reviewed and tested on Linux Mint 20 (x86_64). Tree-SHA512: 12184d33ae5797438d03efd012a07ba3e4ffa0d817c7a0877743f3d7a7656fe279280c751554fc035ccd0058166153b6c6c308a98b2d6b13998922617ad95c4c

fafb381 Remove mempool global (MarcoFalke) fa0359c Remove mempool global from p2p (MarcoFalke) eeee110 Remove mempool global from init (MarcoFalke) Pull request description: This refactor unlocks some nice potential features, such as, but not limited to: * Removing the fee estimates global (would avoid slightly fragile workarounds such as bitcoin#18766) * Making the mempool optional for a "blocksonly" operation mode Even absent those features, the new code without the global should be easier to maintain, read and write tests for. ACKs for top commit: jnewbery: utACK fafb381 hebasto: ACK fafb381, I have reviewed the code and it looks OK, I agree it can be merged. darosior: ACK fafb381 Tree-SHA512: a2e696dc377e2e81eaf9c389e6d13dde4a48d81f3538df88f4da502d3012dd61078495140ab5a5854f360a06249fe0e1f6a094c4e006d8b5cc2552a946becf26

…le cases 020f051 refactor: CTxMemPool::IsUnbroadcastTx() requires CTxMemPool::cs lock (Hennadii Stepanov) 7c4bd03 refactor: CTxMemPool::GetTotalTxSize() requires CTxMemPool::cs lock (Hennadii Stepanov) fa5fcb0 refactor: CTxMemPool::ClearPrioritisation() requires CTxMemPool::cs lock (Hennadii Stepanov) 7140b31 refactor: CTxMemPool::ApplyDelta() requires CTxMemPool::cs lock (Hennadii Stepanov) 66e47e5 refactor: CTxMemPool::UpdateChild() requires CTxMemPool::cs lock (Hennadii Stepanov) 9398077 refactor: CTxMemPool::UpdateParent() requires CTxMemPool::cs lock (Hennadii Stepanov) Pull request description: This is another step to transit `CTxMemPool::cs` from `RecursiveMutex` to `Mutex`. Split out from bitcoin#19306. Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change. Refactoring `const uint256` to `const uint256&` was [requested](bitcoin#19647 (comment)) by **promag**. Please note that now, since bitcoin#19668 has been merged, it is safe to apply `AssertLockHeld()` macros as they do not swallow compile time Thread Safety Analysis warnings. ACKs for top commit: promag: Core review ACK 020f051. jnewbery: Code review ACK 020f051 vasild: ACK 020f051 Tree-SHA512: a31e389142d5a19b25fef0aaf1072a337278564528b5cc9209df88ae548a31440e1b8dd9bae0169fd7aa59ea06e22fe5e0413955386512b83ef1f3e7d941e890

knst mentioned this pull request Jan 22, 2023

refactor: improved initialization of members of LLMQContext and related changes #5150

Merged

3 tasks

knst changed the title ~~backport: deglobalisation of mempool~~ backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 [deglobalisation of mempool] Jan 22, 2023

knst force-pushed the bc-bp-mempool branch from 4947232 to 0cddb7f Compare January 22, 2023 12:53

knst changed the title ~~backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 [deglobalisation of mempool]~~ backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 -- deglobalisation of mempool Jan 22, 2023

knst marked this pull request as ready for review January 22, 2023 15:51

knst requested review from PastaPastaPasta and UdjinM6 January 22, 2023 15:51

knst mentioned this pull request Jan 30, 2023

fix: governance should not lock mempool mutex even call GetTransaction #5175

Merged

2 tasks

kwvg mentioned this pull request Jan 31, 2023

backport: merge bitcoin#20222, #18766, #14033, #19486, #19607, #20291, partial #20187, #20228 (deglobalize addrman and feeEstimator) #5163

Merged

github-actions bot added the needs rebase label Jan 31, 2023

knst force-pushed the bc-bp-mempool branch from 0cddb7f to 8747e75 Compare January 31, 2023 17:37

github-actions bot removed the needs rebase label Jan 31, 2023

UdjinM6 requested changes Jan 31, 2023

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

src/txmempool.cpp Outdated Show resolved Hide resolved

src/validationinterface.h Outdated Show resolved Hide resolved

knst force-pushed the bc-bp-mempool branch from 8747e75 to 85741eb Compare February 1, 2023 06:56

UdjinM6 previously approved these changes Feb 1, 2023

View reviewed changes

UdjinM6 added this to the 19 milestone Feb 1, 2023

github-actions bot added the needs rebase label Feb 3, 2023

knst dismissed UdjinM6’s stale review via 81931f9 February 4, 2023 16:02

knst force-pushed the bc-bp-mempool branch from 85741eb to 81931f9 Compare February 4, 2023 16:02

github-actions bot removed the needs rebase label Feb 4, 2023

kwvg reviewed Feb 4, 2023

View reviewed changes

src/coinjoin/client.cpp Outdated Show resolved Hide resolved

kwvg reviewed Feb 4, 2023

View reviewed changes

src/coinjoin/client.cpp Outdated Show resolved Hide resolved

kwvg reviewed Feb 4, 2023

View reviewed changes

src/coinjoin/client.h Outdated Show resolved Hide resolved

knst force-pushed the bc-bp-mempool branch from 81931f9 to 280a8a3 Compare February 5, 2023 11:29

github-actions bot added the needs rebase label Feb 6, 2023

knst dismissed UdjinM6’s stale review via a0ce905 February 6, 2023 20:02

knst force-pushed the bc-bp-mempool branch from 280a8a3 to a0ce905 Compare February 6, 2023 20:02

github-actions bot removed the needs rebase label Feb 6, 2023

UdjinM6 requested changes Feb 6, 2023

View reviewed changes

src/wallet/salvage.cpp Outdated Show resolved Hide resolved

knst requested a review from UdjinM6 February 6, 2023 20:27

knst force-pushed the bc-bp-mempool branch from d77e96d to 8f73893 Compare February 6, 2023 20:31

UdjinM6 previously approved these changes Feb 6, 2023

View reviewed changes

github-actions bot added the needs rebase label Feb 11, 2023

knst dismissed UdjinM6’s stale review via 19b533a February 12, 2023 07:47

knst force-pushed the bc-bp-mempool branch from 8f73893 to 19b533a Compare February 12, 2023 07:47

github-actions bot removed the needs rebase label Feb 12, 2023

UdjinM6 approved these changes Feb 14, 2023

View reviewed changes

kwvg approved these changes Feb 14, 2023

View reviewed changes

PastaPastaPasta approved these changes Feb 15, 2023

View reviewed changes

meshcollider and others added 9 commits February 15, 2023 00:07

fix: re-run a script from backport bitcoin#17407

88633ca

PastaPastaPasta force-pushed the bc-bp-mempool branch from 19b533a to 7dcdc95 Compare February 15, 2023 06:07

PastaPastaPasta merged commit 50f16bc into dashpay:develop Feb 15, 2023

backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 -- deglobalisation of mempool #5169

backport: merge bitcoin#15748, #18541, #19848, #17407 [redo], #14384, #17477, #19826, #19556, #19854 -- deglobalisation of mempool #5169

Uh oh!

Conversation

knst commented Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How Has This Been Tested?

Checklist:

Uh oh!

PastaPastaPasta commented Jan 30, 2023

Uh oh!

knst commented Jan 30, 2023

Uh oh!

github-actions bot commented Jan 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 3, 2023

Uh oh!

knst commented Feb 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 6, 2023

Uh oh!

knst commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

UdjinM6 commented Feb 6, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 11, 2023

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

knst commented Jan 22, 2023 •

edited

Loading

knst commented Feb 6, 2023 •

edited

Loading