Revamping block validation interface interaction with wallet #2150

furszy · 2021-01-21T13:22:25Z

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:
bitcoin#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)

furszy · 2021-01-25T12:43:32Z

Rebased on master due #2118 and #2126 pre-requisites merge.
Ready for review and finally kill #1726 :).

random-zebra

Looking pretty good. Concept/code ACK. Will do some testing.

Seems that the backport missed 73296f5, so the wallet flush now is scheduled every 500 seconds, instead of half second (previously the ThreadFlushWalletDB had MilliSleep(500)).

random-zebra · 2021-01-28T16:24:58Z

src/wallet/wallet.cpp

-    if (!CWallet::fFlushThreadRunning.exchange(true)) {
-        threadGroup.create_thread(ThreadFlushWalletDB);
+    if (!CWallet::fFlushScheduled.exchange(true)) {
+        scheduler.scheduleEvery(MaybeFlushWalletDB, 500);


Without switching CScheduler to use milliseconds, this flushes the wallet every 500 seconds.

src/test/mempool_tests.cpp

src/validation.cpp

furszy · 2021-01-28T23:22:49Z

yeah zebra, the reason why 73296f5 isn't here is because i did it in #1973 some time ago and was expecting to merge that one before #1726 long road but.. got pretty obnubilated finishing all of this hehe. Going to cherry-pick it here, good catch 👍 .

Add notification signals to make it possible to subscribe to mempool changes: - NotifyEntryAdded(CTransactionRef)> - NotifyEntryRemoved(CTransactionRef, MemPoolRemovalReason)> Also add a mempool removal reason enumeration, which is passed to the removed notification based on why the transaction was removed from the mempool.

Analogue to ConnectTrace that tracks transactions that have been removed from the mempool due to conflicts and then passes them through SyncTransaction at the end of its scope.

This makes ConnectTip responsible for the ConnectTrace instead of splitting the logic between ActivateBestChainStep and ConnectTip

Adapted version of btc@d3167ba9bbefc2e5b7062f81c481547f21c5e44b

furszy · 2021-01-28T23:55:09Z

Feedback tackled, added 4bdf68a solving the wallet flushing time and the nullptr and extra argument nits.

random-zebra

tested ACK 4bdf68a

src/scheduler.h

random-zebra

utACK 98d770f

Fuzzbawls

utACK 98d770f

…ead (without cs_main) 3d11027 wallet:CreateCoinStake, solving IsSpent() missing cs_main lock. (furszy) 046386b IsNoteSaplingChange: Add missing cs_wallet lock. (furszy) ded2e8e feature_dbcrash.py using blockmaxsize instead of blockmaxweight that we currently don't support. (furszy) 00cc6ec dumpwallet: Add missing BlockUntilSyncedToCurrentChain (furszy) bfd9a15 test: sapling_fillblock.py sync mempool every 200 transactions instead of only at the end. (furszy) 53497f0 Validation: DisconnectTip doesn't need to force a flush to disk. (furszy) f8cd371 [Miner] Sync wallet state before try to solve proof of stake. (furszy) 3ace13b qa: Fix some tests to work on native windows (furszy) 65cf7e1 don't attempt mempool entry for wallet transactions on startup if already in mempool (instagibbs) 756d0fa Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift) 1423dba [bugfix] save feeDelta instead of priorityDelta in DumpMempool (Alex Morcos) d97ace9 [Test] notes_double_spend: sync wallet before check balances. (furszy) 1ed753f Fix wallet_tests.cpp, missing fInMempool flag set. (furszy) 815667d unit test framework: missing scheduler service loop start added. (furszy) de3c7ae fix wallet_upgrade.py test, wasn't counting the coinbase script. (furszy) e6770c8 fixing invalid wallet_dump.py, generated PoW blocks use a P2PKH coinbase script that now is properly marked as used inside the wallet. (furszy) 4ed7024 fix invalid numbers in wallet_labels.py (furszy) b9249c5 Miner: generate RPC, fix coinbase script key not marked as used (furszy) 296c956 wallet: guard null m_last_block_processed (furszy) 0dfebf4 sapling_rpc_wallet_tests: remove unneeded cs_main and cs_wallet locks. (furszy) c3a281c fix mempool_persist.py dump issue, missing sync with validation interface. (furszy) 67c754a qa: Sync with validationinterface queue in sync_mempools (MarcoFalke) 596056c [validation] Do not check for double spent zerocoins. (furszy) 0c4642c Add helper to wait for validation interface queue to catch up (Matt Corallo) cc91d44 Block ActivateBestChain to empty validationinterface queue (Matt Corallo) 0c68e2f Add an interface to get the queue depth out of CValidationInterface (Matt Corallo) 31c7974 Decouple block processing cs_main lock from the rest of inv get data requests (furszy) da7c0f7 Refactor ProcessGetData avoiding to lock cs_main for its entire time. (furszy) 10efbe5 net_processing: making PushTierTwoGetDataRequest return a bool in case of pushing the message. (furszy) 51dea23 net_processing move-only: decouple tier two get data request into its own function. (furszy) 1c9fe10 RPC: listunspent remove redundant wallet check (furszy) 4d927b0 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 5f521fd Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) 7d05997 Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) c7ab490 Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 31a8790 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) f6df6e4 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (furszy) 24a3ce4 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 40ed4c4 Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 268be9c Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) 1fa0d70 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Concluding with the validation <--> wallet asynchronous signal processing work started in #2082, #2118, #2150, #2192, #2195. Effectively moving every validation interface callback to a background thread without locking `cs_main` for its entire process (each handler can now request `cs_main` lock only when/if they need it). This has a direct performance improvement on the synchronization time (which i haven't measured yet because there is one/two more PRs over the wallet and GUI areas, probably large as well, on top of this one and #2201 that should boost the sync time a lot more). Containing the following changes: * Adaptations coming from bitcoin#10286. * Adaptations coming from bitcoin#11824 (this one is different for us, take the base idea when you review it). Essentially solves a severe memory leak introduced previously in 10286 and improves `cs_main` lock acquisitions as well. * net_processing: decouple and refactor tier two inv data request processing. * bitcoin#12206 ACKs for top commit: random-zebra: Great job! 🍻 ACK 3d11027 Fuzzbawls: ACK 3d11027 Tree-SHA512: 60a25604fb8a3ad0553ccb074aed99c1b3c6f8a765b40c1b43f25412373cbd2a9e4f0f413d45cf694bd62e48512c936099ffb7a0d23a1b97576cb33283ca05ac

ca3edc5 GUI: Update worker type if task already exist. (furszy) a1390c3 wallet balances, cache total delegated balance and calculate it only once. (furszy) 02eb781 GUI: settings information, do not update block num and block hash if the screen is not visible. (furszy) cbc5021 Masternode-sync read only function to get the "IsBlockchainSynced" state. (furszy) ef77fdf GUI: topbar, removing not used block source. (furszy) e9c160a wallet: single loop to calculate the currently required balances. (furszy) 32538ae wallet: Disallow abandon of conflicted txes (furszy) 62cd35f GUI: MN model, remove now unneeded cs_main locks. (furszy) 5658844 wallet: removing unnecessary `mempool.cs` lock from ReacceptWalletTransactions() (furszy) 63f3fa7 GUI: add missing qt metatype declarations. (furszy) bc76186 wallet model: update delay increased to 5 seconds. (furszy) 3fb3f34 GUI dashboard: do not update the staking filter if it's not needed. (furszy) 7cb8276 init: move "Done loading" message and rpc warm up finished after the wallet post initialization process (furszy) 8762e81 Refactoring with QString::toNSString (Hennadii Stepanov) 69b3330 GUI: txmodel, do not lock cs_wallet if no wtx status update is needed. (furszy) b4ab286 GUI: dashboard, charts update delay time increased for IBD. (furszy) 0f197ca Wallet interface: do not load not used cold staking balances. (furszy) dee4224 Wallet:GetLockedCoins() loop only over the locked coins, not over the entire wallet txes map. (furszy) 0a61f7f GUI: cold staking, do not refresh delegations if the screen is not visible. (furszy) c2d66d0 GUI: cold staking screen, do not initialize coin control dialog at startup. (furszy) 44d9cbe GUI: dashboard screen, remove stakes filter source model if chart is not being presented. (furszy) 6e49a11 GUI: send screen, initialize coinControlDialog view only when it's being called. (furszy) 379e5f2 GUI: send screen, move refresh amounts calculation to a background worker thread. (furszy) 76bc08b GUI: balance polling update moved to a background worker thread. (furszy) b73500a wallet:BlockConnected do not lock cs_wallet for its entire process. (furszy) 208a292 wallet: remove last cs_main locks from every signal handler function :) . (furszy) 9720579 SSPKM: remove redundant ReadBlockFromDisk from IncrementNoteWitnesses. (furszy) 4ccec74 wallet: Add IsSpent() cs_wallet lock assertion. (furszy) e0a0d2d sapling_wallet_tests: locking order refactor, solving inconsistent orders for cs_main and cs_wallet. (furszy) c69e7e8 Adapt sapling_wallet_listreceived.py to the new wtx confirmation structure. (furszy) c4952a2 Wallet::CreateWalletFromFile guard direct chainActive access (furszy) f889dcb test : updating wallet's last block processed manually. (furszy) 45c9471 wallet: split CheckTXAvailability in two. (furszy) 5109c8d Wallet: remove cs_main from IsSaplingSpent() and GetFilteredNotes() (furszy) a7f6ab1 Wallet: remove cs_main requirement from RelayWalletTransaction and FundTransaction. (furszy) 1e7ffc2 Wallet: remove cs_main requirement from CreateCoinStake (furszy) 2e7cdb2 Wallet: added max value out filter for AvailableCoins. (furszy) b9220f4 Wallet: Do not add P2CS utxo to autocombine flow and discard them later. (furszy) 59ed47d Wallet: remove cs_main lock from AutoCombineDust plus a redundant maturity check. (furszy) fcc4c83 Move AutoCombineDust functionality to the wallet background thread (furszy) fcb20c2 GUI: remove cs_main lock dependency from CWalletModel::isSpent, CWalletModel::isLockedCoin, CWalletModel::lockCoin, CWalletModel::unlockCoin (furszy) 65fbad1 GUI: remove cs_main lock dependency from transaction model update. (furszy) 0b61857 GUI: fix coinstake tx ordering. (furszy) 33588fe GUI: Remove cs_main lock and chain dependency from transaction update status (furszy) 3dede64 GUI: Remove cs_main lock from balance polling timer (furszy) 5e06330 Removed IsFinalTx() cs_main lock requirement. (furszy) 1bd97ca wallet::MarkConflicted remove blockIndex and there by cs_main lock dependency. (furszy) 239d6a2 wallet: remove the now unneeded cs_main locks (furszy) 1386ab7 Use CWallet::m_last_block_processed_height in GetDepthInMainChain (furszy) 9adeb61 Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (furszy) 8aa2d31 Add block_height field in struct Confirmation (furszy) 4405ac0 Replace CWalletTx::SetConf by Confirmation initialization list (furszy) 6320efb Update wallet last processed index in every unit test that needs it (furszy) 4957051 wallet: cache block hash, height and time inside the wallet. (furszy) 33f4788 wallet: refactor GetDepthInMainChain to not return the block index. (furszy) e7c8ca6 wallet: remove unused IsInMainChain method (furszy) 9e51a48 Add a test wallet_reorgsrestore (Antoine Riard) 370c200 wallet: simplifying pindexRescan set + added an AssertLockHeld on FindForkInGlobalIndex (furszy) 46f0e30 Fixing reindex problem, use mapBlockIndex.find() and not mapBlockIndex[] (furszy) da78039 [Wallet] Adapting TransactionAddedToMempool and BlockDisconnected to the new wtx confirmation status (furszy) ff04fa6 Modify wallet tx status if has been reorged out (furszy) f7baeaf Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard) 8d8928e Encapsulate tx status in a Confirmation struct (Antoine Riard) Pull request description: This is the conclusion of a deep deep rabbit hole: #1726, #2150, #2082, #2118, #2150, #2179, #2191, #2192, #2195, #2201, #2203.. Effectively removing every `cs_main` lock from the wallet and GUI processing threads. Completely decoupling the wallet state and the visual interface update procedures from the main message and validation handler thread. Meaning that the messages & validation handler thread will be largely more active, accepting and verifying way more data in less time, not being affected by several other threads accessing to the main critical section. And, at the same time, the wallet and GUI worker threads will be able to perform and process their tasks concurrently, without waiting for the `cs_main` mutex acquisition. Improving the overall software performance, the GUI responsiveness and decreasing the synchronization time. To make this possible, the wallet is maintaining in memory a view of the chain and updating it only via the validation interface signals. Using the view to perform all of the chain related calculations. ACKs for top commit: Fuzzbawls: All good now, ACK ca3edc5 random-zebra: ACK ca3edc5 and merging... Tree-SHA512: 6d4268730941822942b0df0aab200683a1fabaf6801618cf34955b43b29bc2beb694567635f731a724abf7b73cec3edfe506e0428de6710c0fcf603613f5614a

It was removed in PIVX-Project#2150

furszy self-assigned this Jan 21, 2021

furszy mentioned this pull request Jan 21, 2021

Up to CTransactionRef point back ports to do list. #1726

Closed

21 tasks

furszy force-pushed the 2020_up_to_9725 branch from 2f85421 to 9d1f48e Compare January 23, 2021 10:21

furszy added Validation Wallet Mempool labels Jan 23, 2021

TheBlueMatt and others added 5 commits January 25, 2021 08:54

Use CScheduler for wallet flushing, remove ThreadFlushWalletDB

00f36ea

Rename FlushWalletDB -> CompactWalletDB, add function description

85e18f0

[Wallet] Simplify InMempool

466e97a

tests: write the notification to different files to avoid race condition

15a21c2

tests: Stop node before removing the notification file

ef429af

furszy force-pushed the 2020_up_to_9725 branch from 9d1f48e to b02d4e5 Compare January 25, 2021 12:13

random-zebra reviewed Jan 28, 2021

View reviewed changes

random-zebra added the Upstream label Jan 28, 2021

random-zebra added this to the 5.1.0 milestone Jan 28, 2021

morcos and others added 12 commits January 28, 2021 20:52

remove external usage of mempool conflict tracking

76c72c6

remove internal tracking of mempool conflicts for reporting to wallet

e7db9ff

Clean up tx prioritization when conflict mined

a8605d2

Introduce MemPoolConflictRemovalTracker

21be4e2

Analogue to ConnectTrace that tracks transactions that have been removed from the mempool due to conflicts and then passes them through SyncTransaction at the end of its scope.

Better document usage of SyncTransaction

4cb5820

Move NotifyEntryRemoved to use the general interfaces::MakeHandler

1b396b8

Include missing #include in zmqnotificationinterface.h

e7c2789

Add pblock to connectTrace at the end of ConnectTip, not start

60329da

This makes ConnectTip responsible for the ConnectTrace instead of splitting the logic between ActivateBestChainStep and ConnectTip

Make ConnectTrace::blocksConnected private, hide behind accessors

27fb897

Handle conflicted transactions directly in ConnectTrace

50d3e0e

Adapted version of btc@d3167ba9bbefc2e5b7062f81c481547f21c5e44b

Keep conflictedTxs in ConnectTrace per-block

6dcb6b0

Remove now unneeded ChainTip signal

67e068c

furszy force-pushed the 2020_up_to_9725 branch from b02d4e5 to 4bdf68a Compare January 28, 2021 23:53

random-zebra previously approved these changes Jan 29, 2021

View reviewed changes

src/scheduler.h Outdated Show resolved Hide resolved

CScheduler boost->std::function, use millisecs for times, not secs

98d770f

furszy dismissed random-zebra’s stale review via 98d770f January 29, 2021 15:10

furszy force-pushed the 2020_up_to_9725 branch from 4bdf68a to 98d770f Compare January 29, 2021 15:10

random-zebra approved these changes Jan 29, 2021

View reviewed changes

furszy requested a review from Fuzzbawls January 30, 2021 12:00

Fuzzbawls approved these changes Jan 30, 2021

View reviewed changes

furszy merged commit 6143f80 into PIVX-Project:master Jan 30, 2021

furszy mentioned this pull request Feb 10, 2021

Call wallet/validation notify callbacks in scheduler thread (without cs_main) #2203

Merged

furszy mentioned this pull request Feb 17, 2021

Killing wallet and GUI cs_main locks #2209

Merged

random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 22, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

cfa6b10

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 23, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

c395644

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 23, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

befbdbd

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 4, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

bd2a357

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 5, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

35d0c05

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 7, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

7c63f2f

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 11, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

6d4d4e9

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 12, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

fc4eead

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 13, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

553a582

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 15, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

4477944

It was removed in PIVX-Project#2150

random-zebra added a commit to random-zebra/PIVX that referenced this pull request May 17, 2021

[Doc] Remove ThreadFlushWalletDB from developer notes

fb64bbc

It was removed in PIVX-Project#2150

furszy mentioned this pull request Jul 13, 2021

[WIP][GUI] Include MN Rewards in the dashboard chart. #1362

Closed

furszy deleted the 2020_up_to_9725 branch November 29, 2022 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamping block validation interface interaction with wallet #2150

Revamping block validation interface interaction with wallet #2150

furszy commented Jan 21, 2021 •

edited

Loading

furszy commented Jan 25, 2021

random-zebra left a comment

random-zebra Jan 28, 2021

furszy commented Jan 28, 2021

furszy commented Jan 28, 2021

random-zebra left a comment

random-zebra left a comment

Fuzzbawls left a comment

Revamping block validation interface interaction with wallet #2150

Revamping block validation interface interaction with wallet #2150

Conversation

furszy commented Jan 21, 2021 • edited Loading

furszy commented Jan 25, 2021

random-zebra left a comment

Choose a reason for hiding this comment

random-zebra Jan 28, 2021

Choose a reason for hiding this comment

furszy commented Jan 28, 2021

furszy commented Jan 28, 2021

random-zebra left a comment

Choose a reason for hiding this comment

random-zebra left a comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

furszy commented Jan 21, 2021 •

edited

Loading