backport: Call wallet notify callbacks in scheduler thread (without cs_main) (bitcoin #10286) #1168
Conversation
This file contains 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
This is currently unused, but will by used by wallet to cache when transactions are in the mempool, obviating the need for calls to mempool from CWalletTx::InMempool()
This is both good practice (we want to move all such callbacks into a background thread eventually) and prevents a lock inversion when we go to use this in wallet (mempool.cs->cs_wallet and cs_wallet->mempool.cs would otherwise both be used).
This blocks until the wallet has synced up to the current height.
This prevents the wallet-RPCs-return-stale-info issue from being re-introduced when new-block callbacks no longer happen in the block-connection cs_main lock conflict: We have disabled bumpfee
This avoid calling out to mempool state during coin selection, balance calculation, etc. In the next commit we ensure all wallet callbacks from CValidationInterface happen in the same queue, serialized with each other. This helps to avoid re-introducing one of the issues described in #9584 [1] by further disconnecting wallet from current chain/mempool state. Thanks to @morcos for the suggestion to do this. Note that there are several race conditions introduced here: * If a user calls sendrawtransaction from RPC, adding a transaction which is "trusted" (ie from them) and pays them change, it may not be immediately used by coin selection until the notification callbacks finish running. No such race is introduced in normal transaction-sending RPCs as this case is explicitly handled. * Until Block{Connected,Disconnected} and TransactionAddedToMempool calls also run in the CSceduler background thread, there is a race where TransactionAddedToMempool might be called after a Block{Connected,Disconnected} call happens. * Wallet will write a new best chain from the SetBestChain callback prior to having processed the transaction from that block. [1] "you could go to select coins, need to use 0-conf change, but such 0-conf change may have been included in a block who's callbacks have not yet been processed - resulting in thinking they are not in mempool and, thus, not selectable." Conflicts: Already backported beef7ec4be725beea870a2da510d2817487601ec
This runs Block{Connected,Disconnected}, SetBestChain, Inventory,
and TransactionAddedToMempool on the background scheduler thread.
Of those, only BlockConnected is used outside of Wallet/ZMQ, and
is used only for orphan transaction removal in net_processing,
something which does not need to be synchronous with anything
else.
This partially reverts #9583, re-enabling some of the gains from
#7946. This does not, however, re-enable the gains achieved by
repeatedly releasing cs_main between each transaction processed.
Conflicts:
Already backported beef7ec4be725beea870a2da510d2817487601ec
hans-schmidt
requested changes
Jan 21, 2022
There was a problem hiding this comment.
Please correct the following error:
In src/validationinterface.h this PR incorrectly deleted Line 42 "~CValidationInterface() = default;"
-that destructor was added into Ravencoin by Mark Ney 2019-11-11 to remove build warnings
-that desctructor is also in the current version of bitcoin (added into bitcoin 2018-03-14 in commit 2b3ea39d)
The only other two changes relative to the bitcoin PR which I saw are correct:
1) src/validationinterface.cpp is different:
The last few line changes are ommitted in RVN because RVN doesn't have a "void CMainSignals::Inventory" function
2) src/wallet/rpcwallet.cpp is different:
The last 3-lines added in the bitcoin PR are ommitted on the RVN PR because bumpfee has been deprecated on the RVN Wallet
Note that UpdatedBlockTip is also used in net_processing to announce new blocks to peers. As this may need additional review, this change is included in its own commit.
fdoving
force-pushed
the
fdov-btc10286
branch
from
7612ba6 to
aee4d33
Compare
|
Changes pushed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport of bitcoin/bitcoin#10286