ryanofsky
commented at 9:35 pm on April 1, 2019:
contributor
Move wallet startup code closer to a simple model where the wallet attaches to the chain with a single chain.handleNotifications() call, and just starts passively receiving blocks and mempool notifications from the last update point, instead having to actively rescan blocks and request a mempool snapshot, and deal with the tip changing, and deal with early or stale notifications.
Also, stop locking the cs_wallet mutex and registering for validationinterface notifications before the rescan. This was new behavior since 6a72f26968cf931c985d8d4797b6264274cabd06#16426 and is not ideal because it stops other wallets and rpcs and the gui from receiving new notifications until after the scan completes.
This change is a half-step towards implementing multiwallet scans (https://github.com/bitcoin/bitcoin/issues/11756), since it provides needed locator and birthday timestamp information to the Chain interface, and it rationalizes locking and event ordering in the startup code. The second half of implementing parallel rescans requires moving the ScanForWalletTransactions implementation (which this PR does not touch) from the wallet to the node.
DrahtBot
commented at 10:01 pm on April 1, 2019:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#23581 (Move BlockManager to node/blockstorage by MarcoFalke)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#21603 (log: Mitigate disk filling attacks by rate limiting LogPrintf by dergoegge)
#20205 (wallet: Properly support a wallet id by achow101)
#18554 (wallet: ensure wallet files are not reused across chains by rodentrabies)
#16673 (Relog configuration args on debug.log rotation by LarryRuane)
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.
promag
commented at 10:09 pm on April 1, 2019:
member
How will snapshot_fn work with IPC?
ryanofsky
commented at 0:37 am on April 2, 2019:
contributor
Added annotations. Effect is kind of limited since this is an override and these mutexes aren’t accessible where this is called in the wallet, but it still seems better to have these annotations than not.
Longer term, after more changes like this and #15632, which reduce Chain::Lock usage, wallet code should stop holding these locks altogether.
laanwj added the label
Mempool
on Apr 2, 2019
promag
commented at 12:24 pm on April 2, 2019:
member
in
src/interfaces/chain.cpp:369
in
10f92ce5c4outdated
369+ // Declare packaged task to run in notification thread, and to first
370+ // send the snapshot before enabling subsequent notifications.
371+ std::vector<CTransactionRef> snapshot;
372+ std::packaged_task<std::unique_ptr<Handler>()> task([&] {
373+ if (snapshot_fn) snapshot_fn(std::move(snapshot));
374+ return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
Mempool can change after the snapshot but before registering the interface and so those changes will be missed.
This isn’t true, and the comment below, “Hold locks while scheduling the task so notifications about added and removed transactions after the snapshot arrive after the snapshot” is specifically referring to this case.
Let me know if I should make it more clear, but as long as the locks are held, transactions can’t be added / removed by other threads, and as long as CallFunctionInValidationInterfaceQueue is called before transactions are added/removed, the handler will be registered before the queued notifications signal it.
But the queue can be non empty when CallFunctionInValidationInterfaceQueue is called?
I think you are referring to a different problem now (problem of old notifications being received when they shouldn’t be, which is different from the problem of new notifications not being received when they should be). The problem of old notifications being received when they shouldn’t be is what happens without this PR in the current code:
The problem is fixed by this PR by not constructing NotificationsHandlerImpl until after snapshot_fn is called. The comment there “to first send the snapshot before enabling subsequent notifications” is about this.
AddToProcessQueue tricked me, what really goes to that queue are notifications.
ryanofsky force-pushed
on Apr 2, 2019
in
src/interfaces/chain.cpp:378
in
5f593ab2e1outdated
375+ return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
376+ });
377+
378+ // Snapshot mempool transactions, then schedule the task to execute.
379+ // Hold locks while scheduling the task so notifications about added and
380+ // removed transactions after the snapshot arrive after the snapshot.
Yes task() is what sends the snapshot, so that’s what this is referring to. Replaced “transactions after the snapshot arrive after the snapshot” with “transactions after taking the snapshot arrive after the snapshot callback in task()” to reference it and be more specific.
ryanofsky force-pushed
on Apr 2, 2019
promag
commented at 8:07 pm on April 2, 2019:
member
Should we assert cs_main is held in the relevant validation interface notifications?
ryanofsky
commented at 8:24 pm on April 2, 2019:
contributor
Should we assert cs_main is held in the relevant validation interface notifications?
cs_main can be acquired inside notification callbacks, if necessary, but it is not held when these notifications happen. This PR and #15632 disentangle notifications and locking, so locks are never held when notifications are received (right now they are inconsistently held in some cases but not others).
Removing locking from notifications is a step toward eventually removing Chain::Lock completely, and having the wallet just asynchronously process notifications without being able to hold on to cs_main.
promag
commented at 1:35 pm on April 3, 2019:
member
Sorry for not being clear. I mean that cs_main is locked when GetMainSignals().TransactionAddedToMempool() is called, and it is important that doesn’t change, by adding:
DrahtBot added the label
Needs rebase
on Apr 9, 2019
ryanofsky force-pushed
on Apr 10, 2019
DrahtBot removed the label
Needs rebase
on Apr 10, 2019
ryanofsky force-pushed
on Apr 11, 2019
jnewbery
commented at 9:00 pm on April 11, 2019:
contributor
Concept ACK
Mempool transactions are sent in a single IPC call instead of in a loop calling an IPC method repeatedly.
Do we know what this does to memory usage if there’s a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?
ryanofsky
commented at 9:49 pm on April 11, 2019:
contributor
Do we know what this does to memory usage if there’s a very large mempool (either at startup when starting one or multiple wallets, or when loading a wallet dynamically during runtime)?
With this PR by itself, a new temporary vector with one pointer per mempool transaction is created when a wallet is loaded. It’s something to consider, but probably not likely to cause problems.
With this PR combined with #10102 and running bitcoin-node with a wallet loading in a different process, this will use significantly more memory, because serializing the vector serializes all the transactions at once in memory before sending them. This might be about as expensive as a getrawmempool call. If this is a problem, the snapshot_fn callback could be tweaked to send the transactions in batches. I guess I wouldn’t want to try to optimize this prematurely, though.
Since I did write this PR “Improves performance over IPC” in the description, the increased memory usage might factor into that. But to be sure, the motivation for this PR is mainly to make notifications show up cleanly and in order on the client side.
in
src/wallet/wallet.cpp:4227
in
0ebc74e763outdated
@jnewbery points out that it is too early to handle mempool transactions here at this point, because the rescan hasn’t happened yet. Processing a mempool transaction before the rescan could result in the IsFromMe check in AddToWalletIfInvolvingMe failing to add the transaction to the walelt when it depends on a block that hasn’t been scanned yet.
So the mempool loop here needs to be moved below and will require some changes to locking, since handleNotifications in this PR can’t be called with cs_main (it would deadlock).
@jnewbery points out that it is too early to handle mempool transactions here at this point
This is resolved in the current version of the PR which integrates rescans and finishes recsanning before creating the mempool snapshot
in
src/wallet/wallet.cpp:4218
in
0ebc74e763outdated
4214@@ -4213,6 +4215,17 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
4215 // Try to top up keypool. No-op if the wallet is locked.
4216 walletInstance->TopUpKeyPool();
42174218+ // Register with the validation interface. Skip requesting mempool transactions if wallet is empty.
I think moving the sync-from-mempool function to above the initial rescan breaks the (undocumented) expectation that the wallet is notified of transactions in dependency order. In fact, this was slightly broken by #15652, but this PR makes it worse.
Prior to dynamically-loaded wallets, the wallet would always receive a transaction after it had received all of that transaction’s ancestors. That wallet relies on that expectation:
CWallet::IsFromMe() will only match on a transaction if its parent is already in the wallet.
nTimeSmart should be monotonically increasing for transaction descendency. If transactions arrive in the wrong order then this is no longer true, and they’ll be shown out-of-order in the GUI.
Since #10740, a wallet could receive mempool transactions out-of-order (since a newly loaded wallet would not be notified of transactions in the mempool, but could be notified of their descendant transactions when they arrive). #15652 improved the situtation by notifying the wallet of all mempool transactions, but didn’t sort them first, so a wallet could still be notified of the mempool txs in the wrong order. This PR potentially makes things worse by notifying the wallet of mempool txs before the blocks which could contain the mempool txs’ ancestors.
I think to fix this, we need to:
move the sync-from-mempool to after the rescan call, while cs_main is still held
sort the mempool txs by using mempool.infoAll() before notifying the wallet.
We should also document that the wallet expects to see transactions in order!
I also wonder if there could brokenness in the case where a reorg happens during a rescanblockchain or importmulti RPC call (these rescans are different because cs_main isn’t held the whole time).
move the sync-from-mempool to after the rescan call, while cs_main is still held
sort the mempool txs by using mempool.infoAll() before notifying the wallet.
We should also document that the wallet expects to see transactions in order!
Moving sync is done. Rescanning happens before syncing from mempool in the current version of this PR. Sorting mempool isn’t done yet. So this will still be as broken as it has been since #15652, but not more broken. There’s already a lot going on here, so I think I would want to address this in a different PR. It seems pretty easy to fix independently of this PR, but writing a test case could be a challenge
ryanofsky renamed this:
Drop Chain::requestMempoolTransactions method
WIP: Drop Chain::requestMempoolTransactions method
on Apr 17, 2019
DrahtBot added the label
Needs rebase
on Apr 17, 2019
maflcko
commented at 1:56 pm on August 16, 2019:
member
@ryanofsky Are you still working on this? If no, then please close it.
ryanofsky
commented at 3:35 pm on August 19, 2019:
contributor
@ryanofsky Are you still working on this? If no, then please close it.
Thanks for the reminder. I looked into this again and I think the WIP tag still applies. I had been indecisive about involving rescan in this this change after John pointed out it was necessary, since I was unsure if it was better to incrementally change the rescan code (first synchronize rescan with notifications, then consolidate rescans across wallets https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa), or to just do both things in a single PR.
But now I think it should be possible to synchronize rescan with notifications in a way that doesn’t change existing code too much and should also make ariard’s proposed change easier.
ryanofsky force-pushed
on Jan 24, 2020
ryanofsky force-pushed
on Jan 27, 2020
ryanofsky renamed this:
WIP: Drop Chain::requestMempoolTransactions method
Drop Chain::requestMempoolTransactions method
on Jan 27, 2020
ryanofsky force-pushed
on Jan 28, 2020
DrahtBot removed the label
Needs rebase
on Jan 29, 2020
DrahtBot added the label
Needs rebase
on Jan 30, 2020
ryanofsky force-pushed
on Mar 31, 2020
ryanofsky force-pushed
on Mar 31, 2020
ryanofsky force-pushed
on Mar 31, 2020
DrahtBot removed the label
Needs rebase
on Mar 31, 2020
DrahtBot added the label
Needs rebase
on Apr 6, 2020
ryanofsky force-pushed
on Apr 15, 2020
ryanofsky
commented at 4:30 pm on April 15, 2020:
contributor
Rebased 3eb122b4a10024d86561492db19a881d420e34c5 -> b7716d0272125f4b4d2bce179d86d896f67869bd (pr/pool.9 -> pr/pool.10, compare) after merge of #17954 and conflict with #18192
DrahtBot removed the label
Needs rebase
on Apr 15, 2020
DrahtBot added the label
Needs rebase
on Apr 19, 2020
ryanofsky referenced this in commit
f9cea7231d
on Apr 21, 2020
ryanofsky force-pushed
on Apr 21, 2020
ryanofsky
commented at 8:53 pm on April 21, 2020:
contributor
Rebased b7716d0272125f4b4d2bce179d86d896f67869bd -> bb7e272bd968adff75bed5038680f23b665880c5 (pr/pool.10 -> pr/pool.11, compare) due to minor conflicts with #15761 and #18601
DrahtBot removed the label
Needs rebase
on Apr 21, 2020
ryanofsky referenced this in commit
2142bf14fc
on Apr 27, 2020
ryanofsky referenced this in commit
1dcbe64463
on Apr 27, 2020
ryanofsky referenced this in commit
66a4d4b2ec
on Apr 28, 2020
DrahtBot added the label
Needs rebase
on Apr 29, 2020
ryanofsky force-pushed
on Apr 29, 2020
ryanofsky
commented at 5:45 pm on April 29, 2020:
contributor
Rebased bb7e272bd968adff75bed5038680f23b665880c5 -> 3c50af4c76889261d19262086542b44220c74457 (pr/pool.11 -> pr/pool.12, compare) due to conflict with #18759
ryanofsky referenced this in commit
7918c1b019
on Apr 29, 2020
DrahtBot removed the label
Needs rebase
on Apr 29, 2020
maflcko referenced this in commit
0f204dd3f2
on Apr 29, 2020
ryanofsky force-pushed
on Apr 30, 2020
ryanofsky
commented at 7:22 am on April 30, 2020:
contributor
Rebased 3c50af4c76889261d19262086542b44220c74457 -> 044418037a6ead2e9c38f404228ea8536369b683 (pr/pool.12 -> pr/pool.13, compare) after #18727 to deal with failures in new CreateWalletFromFile test. Rewrote test to handle new startup sequence.
ariard referenced this in commit
86ec6f7ce6
on Apr 30, 2020
pierreN referenced this in commit
3eaf73ad8b
on May 1, 2020
DrahtBot added the label
Needs rebase
on May 1, 2020
ryanofsky force-pushed
on May 1, 2020
ryanofsky
commented at 3:20 pm on May 1, 2020:
contributor
Rebased 044418037a6ead2e9c38f404228ea8536369b683 -> 26e4ee32273ab232fa3898f86289131a6379b69b (pr/pool.13 -> pr/pool.14, compare) due to conflict with #16426
DrahtBot removed the label
Needs rebase
on May 1, 2020
sidhujag referenced this in commit
d7c1f85877
on May 2, 2020
DrahtBot added the label
Needs rebase
on May 4, 2020
ryanofsky force-pushed
on May 4, 2020
ryanofsky
commented at 6:58 pm on May 4, 2020:
contributor
Rebased 26e4ee32273ab232fa3898f86289131a6379b69b -> 52d152f0bf2543fc6fbb80cdde7ae2249305662f (pr/pool.14 -> pr/pool.15, compare) due to conflict with #18699
DrahtBot removed the label
Needs rebase
on May 4, 2020
ryanofsky
commented at 4:59 am on May 5, 2020:
contributor
Travis error test_framework.authproxy.JSONRPCException: non-mandatory-script-verify-flag (Signature must be zero for failed CHECK(MULTI)SIG operation) (-26) in sendrawtransaction call in rpc_rawtransaction.py in bionic C++17 build https://travis-ci.org/github/bitcoin/bitcoin/jobs/683072285#L5042 seems unrelated. It’s reported in #18803
in
src/wallet/wallet.cpp:2818
in
52d152f0bfoutdated
4027+ best_block_locator.emplace();
4028+ WalletBatch(*wallet->database).ReadBestBlock(*best_block_locator);
4029+ }
40304031+ // No need to read and scan block if block was created before
4032+ // our wallet birthday (as adjusted for block time variability)
Block time variability is unclear, you mean the absence of order guarantee between nTime and nTimeFirstKey?
This is not a new comment (just moved) but it is just referring to TIMESTAMP_WINDOW
in
src/interfaces/chain.h:237
in
52d152f0bfoutdated
234+ using MempoolFn = std::function<void(std::vector<CTransactionRef>)>;
235+
236+ //! Register handler for notifications. Before returning and before sending
237+ //! the first notification, call ScanFn with a scan range of blocks after a
238+ //! specified location and time, and then call MempoolFn with a snapshot of
239+ //! transactions in the mempool before the first transactionAddedToMempool /
This part of comment may be made more intuitively instead of one chunk, like “Call MempoolFn with a snapshot of transaction before firing transactions back to caller”.
This part of comment may be made more intuitively instead of one chunk, like “Call MempoolFn with a snapshot of transaction before firing transactions back to caller”.
That seems good. I shortened existing description to match it
in
src/interfaces/chain.h:257
in
52d152f0bfoutdated
248+ //! @param[in] mempool_fn callback invoked before notifications are sent
249+ //! with snapshot of mempool transactions
250+ //! @param[in] scan_locator location of last block previously scanned.
251+ //! scan_fn will be only be called for blocks after
252+ //! this point. Can be null to scan from genesis.
253+ //! @param[in] scan_time minimum block timestamp for beginning the scan
May we drop either or scan_locator or scan_time, like if wallet first key is older that locator use it as a starting point, if not use wallet last block nTime. Do we still really need wallet locator ?
May we drop either or scan_locator or scan_time, like if wallet first key is older that locator use it as a starting point, if not use wallet last block nTime. Do we still really need wallet locator ?
I think the idea is to allow scanning as little as necessary. Both arguments are optional so callers aren’t obligated to provide them. This rescan behavior be revisited in a followup PR, but in this PR I think it makes sense for wallet to continue scanning same blocks it was scanning previously.
in
src/interfaces/chain.h:270
in
52d152f0bfoutdated
253+ //! @param[in] scan_time minimum block timestamp for beginning the scan
254+ //! scan_fn will only be called for blocks starting
255+ //! from this timestamp
256+ //! @param[out] tip information about chain tip at the point where
257+ //! notifications will begin
258+ virtual std::unique_ptr<Handler> handleNotifications(std::shared_ptr<Notifications> notifications,
I think it would be better to rename handleNotifications. IMO it does 3 different tasks:
trigger rescan
sync with mempool
register notification
I’m fine with doing all of three in same functions rather than 3 chunks for each, but name could be something like syncClient ?
Also you may be interested by rescan but not mempool-sync and therefore it may be made optional.
I think it’s important to stick with the convention used throughout src/interfaces/ where functions that register for notifications and return Handler begin with handle. There are 20 of these functions currently, though only 2 in chain.h.
The main thing this function does, and only permanent thing it is intended to do is register a handler for notifications. scan_fn and mempool_fn are both optional arguments and are temporary. I want to rebase your https://github.com/bitcoin/bitcoin/compare/master...ariard:2019-08-rescan-index-refactor branch on top of this PR so the scan_fn callback will disappear and be replaced by normal blockConnected / blockDisconnected calls. And I want to update the transactionAddedToMempool function to take a list of transactions instead of a single transaction so the mempool_fn can go away.
in
src/interfaces/chain.h:260
in
52d152f0bfoutdated
251+ //! scan_fn will be only be called for blocks after
252+ //! this point. Can be null to scan from genesis.
253+ //! @param[in] scan_time minimum block timestamp for beginning the scan
254+ //! scan_fn will only be called for blocks starting
255+ //! from this timestamp
256+ //! @param[out] tip information about chain tip at the point where
This seems precise, but I may not be seeing what is is unclear. The important point is that there shouldn’t be any gap between the tip and the blockConnected or blockDisconnected notification
in
src/interfaces/chain.cpp:367
in
52d152f0bfoutdated
348+ AssertLockNotHeld(::mempool.cs);
349+ WAIT_LOCK(::cs_main, main_lock);
350+
351+ // Call scan_fn until it has scanned all blocks after specified
352+ // location and time. Looping is necessary because new blocks may
353+ // be connected during rescans.
Isn’t taking cs_main lock avoid block tip increase ?
Nope, cs_main isn’t held while rescanning so node will not lock up when loading a wallet
ariard
commented at 8:55 am on May 8, 2020:
member
Approach ACK, thanks for moving forward with wallet initialization/rescan refactor.
Is there a way to split commit ?
there is few-style changes around fRescan in src/wallet/rcpdump.cpp
maybe test modifications can be moved in their own commit ?
a wallet lock move from import* in src/rwallet/rpcdump.cpp to ReacceptWallet
I’m personally fine with reviewing PR in a single chunk, but maybe other reviewers are less familiar with this part of the codebase.
DrahtBot added the label
Needs rebase
on May 11, 2020
ryanofsky force-pushed
on May 12, 2020
ryanofsky
commented at 4:49 am on May 12, 2020:
contributor
Updated 52d152f0bf2543fc6fbb80cdde7ae2249305662f -> 66588cfcfeb7d89f242439a97db85be44c3a2dae (pr/pool.15 -> pr/pool.16, compare) splitting commits and updating some names and comments
Rebased 66588cfcfeb7d89f242439a97db85be44c3a2dae -> 78a42ce0251426051ff789e64a9bfae3200d1f83 (pr/pool.16 -> pr/pool.17, compare) due to conflict with #18216
DrahtBot removed the label
Needs rebase
on May 12, 2020
ryanofsky force-pushed
on May 12, 2020
ryanofsky
commented at 12:37 pm on May 12, 2020:
contributor
DrahtBot added the label
Needs rebase
on May 28, 2020
ryanofsky renamed this:
Drop Chain::requestMempoolTransactions method
Wallet passive startup
on Jun 1, 2020
ryanofsky force-pushed
on Jun 1, 2020
DrahtBot removed the label
Needs rebase
on Jun 1, 2020
DrahtBot added the label
Needs rebase
on Jun 2, 2020
ariard
commented at 0:42 am on June 4, 2020:
member
Actually rebasing #17484, I think to avoid breaking anti-fee snipping it would be better to setup IBD state at mempool/block notification sync. Ideally calling updateBlockTip at any ending of block rescan. But likely I need to wait for ScanForWalletTransactions being moved inside the node, if I want to drop isInitialBlockDownload
Is this PR ready for review ?
vahidjalaliii
commented at 1:54 am on June 4, 2020:
none
PR is complete and ready for review. Only caveat is that the unit test is triggering thread sanitizer errors. The errors look like they could be real, but I think they are errors in the test itself, not the code, and I’ll look into them.
meshcollider added the label
Wallet
on Jul 11, 2020
ryanofsky force-pushed
on Jul 11, 2020
ryanofsky force-pushed
on Jul 13, 2020
ryanofsky force-pushed
on Jul 14, 2020
DrahtBot added the label
Needs rebase
on Aug 5, 2020
ryanofsky force-pushed
on Aug 7, 2020
DrahtBot removed the label
Needs rebase
on Aug 7, 2020
DrahtBot added the label
Needs rebase
on Aug 15, 2020
luke-jr referenced this in commit
74de8d57b2
on Aug 15, 2020
ryanofsky force-pushed
on Aug 17, 2020
DrahtBot removed the label
Needs rebase
on Aug 17, 2020
DrahtBot added the label
Needs rebase
on Aug 31, 2020
ryanofsky force-pushed
on Sep 2, 2020
DrahtBot removed the label
Needs rebase
on Sep 2, 2020
DrahtBot added the label
Needs rebase
on Sep 5, 2020
ryanofsky force-pushed
on Sep 25, 2020
DrahtBot removed the label
Needs rebase
on Sep 25, 2020
DrahtBot added the label
Needs rebase
on Nov 11, 2020
janus referenced this in commit
c0add1ba79
on Nov 15, 2020
Fabcien referenced this in commit
a8ce4f8ed8
on Feb 9, 2021
ryanofsky force-pushed
on Apr 11, 2021
DrahtBot removed the label
Needs rebase
on Apr 12, 2021
rebroad
commented at 8:17 pm on April 18, 2021:
contributor
@ryanofsky I’ve not looked at this yet, but does this cause the GUI to become available sooner (e.g. the rescan can run after the splash screen rather than during)? I’d test this if so, or if I understand what benefits it provides - e.g. I’m not sure why we’d want parallel multiwallet scans - unless it means it would scan them all faster in total, for example.
ryanofsky
commented at 12:49 pm on April 21, 2021:
contributor
Thanks for looking at this.
@ryanofsky I’ve not looked at this yet, but does this cause the GUI to become available sooner (e.g. the rescan can run after the splash screen rather than during)?
Yes, this separates opening a wallet from attaching to the node, so should it be possible to interact with the wallet before the rescan finishes if GUI is changed to remove the rescan dialog or make it nonmodal.
Also if there are multiple wallets, rescanning once instead of multiple times should make loading faster in general.
I’d test this if so, or if I understand what benefits it provides - e.g. I’m not sure why we’d want parallel multiwallet scans - unless it means it would scan them all faster in total, for example.
Updated the description to drop the word parallel. It’s not really a question of parallel or serial, it’s a question of scanning blocks one time for all wallets, or scanning blocks multiple times, once for each wallet.
DrahtBot added the label
Needs rebase
on May 10, 2021
laanwj referenced this in commit
d4c409cf09
on May 19, 2021
ryanofsky force-pushed
on May 19, 2021
ryanofsky
commented at 6:16 pm on May 19, 2021:
contributor
DrahtBot removed the label
Needs rebase
on May 19, 2021
DrahtBot added the label
Needs rebase
on Jun 12, 2021
ryanofsky force-pushed
on Jun 14, 2021
DrahtBot removed the label
Needs rebase
on Jun 14, 2021
ryanofsky force-pushed
on Jun 14, 2021
ryanofsky force-pushed
on Jun 15, 2021
DrahtBot added the label
Needs rebase
on Aug 19, 2021
ryanofsky force-pushed
on Aug 19, 2021
DrahtBot removed the label
Needs rebase
on Aug 19, 2021
DrahtBot added the label
Needs rebase
on Sep 3, 2021
ryanofsky force-pushed
on Sep 3, 2021
DrahtBot removed the label
Needs rebase
on Sep 3, 2021
DrahtBot added the label
Needs rebase
on Sep 30, 2021
ryanofsky force-pushed
on Oct 5, 2021
DrahtBot removed the label
Needs rebase
on Oct 5, 2021
DrahtBot added the label
Needs rebase
on Oct 13, 2021
ryanofsky force-pushed
on Oct 13, 2021
DrahtBot removed the label
Needs rebase
on Oct 13, 2021
in
test/sanitizer_suppressions/tsan:40
in
24dfff610eoutdated
35+# Location is global 'std::__1::cout' of size 160 at 0x7f492785e270 (libc++.so.1+0x0000000c0290)
36+# https://travis-ci.org/github/bitcoin/bitcoin/jobs/695004018
37+#
38+# Uses of std::cout are guaranteed thread safe by the c++ standard
39+# https://stackoverflow.com/questions/50322790/thread-safety-and-piping-to-streams
40+race:std::__1::ios_base::width
maflcko
commented at 10:09 am on October 27, 2021:
DrahtBot added the label
Needs rebase
on Nov 25, 2021
lib: Add FoundBlock locator support
Allow retrieving locator from Chain findBlock methods, used in upcoming commit
by the wallet to get the locator for the chain tip on startup.
a1c9b87413
util: Release logger lock while calling log callbacks
Prevents log callbacks from deadlocking if they call code containing log
statements. In most cases it will not make sense to call logging code from a
log hook, because it could trigger and endless chain of log messages. But if a
hook is conditioned on the content of messages, like the DebugLogHelper hook,
it can make sense and be useful.
The new functionality is used in the CreateWalletFromFile test in an upcoming
commit to be able to create transactions and blocks at specific points during
test execution and check for race conditions.
f6bd2b1e2b
refactor: Make CWallet:::AttachChain return scan status
Make AttachChain just responsible for syncing to the chain, moving error
handling and chain assignment to the caller before more syncing
functionality is moved in the next commit.
Also, start using the AttachChain method in tests instead of trying
attach in a more partial way using Chain::handleNotifications.
37977ba4ec
Wallet passive startup
Move wallet startup code closer to a simple model where the wallet attaches to
the chain with a single chain.handleNotifications() call, and just starts
passively receiving blocks and mempool notifications from the last update point,
instead having to actively rescan blocks and request a mempool snapshot, and
deal with the tip changing, and deal with early or stale notifications.
Also, stop locking the cs_wallet mutex and registering for validationinterface
notifications before the rescan. This was new behavior since
6a72f26968cf931c985d8d4797b6264274cabd06
https://github.com/bitcoin/bitcoin/pull/16426 and is not ideal because it stops
other wallets and rpcs and the gui from receiving new notifications until after the
scan completes.
This change is a half-step towards implementing multiwallet parallel scans
(https://github.com/bitcoin/bitcoin/issues/11756), since it provides needed
locator and birthday timestamp information to the Chain interface, and it
rationalizes locking and event ordering in the startup code. The second half of
implementing parallel rescans requires moving the ScanForWalletTransactions
implementation (which this PR does not touch) from the wallet to the node.
d6b8f93fb7
refactor: Move cs_wallet to ReacceptWalletTransactions
This is just a code simplification and revert of
0440481c6bf5683eff669c789bdf6a306d99adc5 from
https://github.com/bitcoin/bitcoin/pull/15652. No behavior is changing.
2b898a0456
ryanofsky force-pushed
on Nov 29, 2021
DrahtBot removed the label
Needs rebase
on Nov 29, 2021
DrahtBot
commented at 8:40 am on December 3, 2021:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
DrahtBot added the label
Needs rebase
on Dec 3, 2021
uvhw referenced this in commit
47d44ccc3e
on Feb 14, 2022
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-07-01 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
