Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281

jonasschnelli commented at 11:40 PM on September 7, 2017: contributor

Right now, we are holding cs_main/cs_wallet during the whole rescan process (which can take a couple of hours). This was probably only done because of laziness and it is an important show-stopper for #11200 (GUI rescan abort).

jonasschnelli added the label Refactoring on Sep 7, 2017

jonasschnelli commented at 11:40 PM on September 7, 2017: contributor

Best reviewed with ?w=1 (https://github.com/bitcoin/bitcoin/pull/11281/files?w=1)

laanwj added the label Wallet on Sep 7, 2017

practicalswift commented at 7:26 AM on September 8, 2017: contributor

@jonasschnelli Which variables are guarded by cs_main and cs_wallet in these specific cases?

I'll double-check against my lock annotations to make sure they are in sync.

promag commented at 1:35 PM on September 11, 2017: member

Awesome, will review.

in src/wallet/wallet.cpp:1537 in 4c1f082b99 outdated

1537 |      // to be scanned.
1538 | -    CBlockIndex* const startBlock = chainActive.FindEarliestAtLeast(startTime - TIMESTAMP_WINDOW);
1539 | -    LogPrintf("%s: Rescanning last %i blocks\n", __func__, startBlock ? chainActive.Height() - startBlock->nHeight + 1 : 0);
1540 | +    CBlockIndex* startBlock = nullptr;
1541 | +    {
1542 | +        LOCK2(cs_main, cs_wallet);

sipa commented at 12:01 AM on September 12, 2017:

What needs cs_wallet here?

in src/wallet/wallet.cpp:1575 in 4c1f082b99 outdated

1573 | -        double dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1574 | -        double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());
1575 | +        double dProgressStart = 0;
1576 | +        double dProgressTip = 0;
1577 | +        {
1578 | +            LOCK2(cs_main, cs_wallet);

sipa commented at 12:07 AM on September 12, 2017:

What needs cs_wallet here?

in src/wallet/wallet.cpp:1694 in 4c1f082b99 outdated

1587 | @@ -1582,13 +1588,17 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
1588 |  
1589 |              CBlock block;
1590 |              if (ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
1591 | +                LOCK2(cs_main, cs_wallet);

sipa commented at 12:11 AM on September 12, 2017:

What needs cs_main here?

promag commented at 11:53 PM on December 19, 2017:

chainActive.

in src/wallet/wallet.cpp:1704 in 4c1f082b99 outdated

1595 |              } else {
1596 |                  ret = pindex;
1597 |              }
1598 | -            pindex = chainActive.Next(pindex);
1599 | +            {
1600 | +                LOCK2(cs_main, cs_wallet);

sipa commented at 12:11 AM on September 12, 2017:

What needs cs_wallet here?

jonasschnelli commented at 8:41 PM on December 13, 2017:

Fixed

jonasschnelli force-pushed on Sep 16, 2017

jonasschnelli commented at 3:53 AM on September 16, 2017: contributor

Updated and reduced locks after @sipa's mentioning.

achow101 commented at 3:49 PM on September 18, 2017: member

utACK 1d367effa8e7a19303ae46a6b871084cbaaf5a0a

in src/wallet/wallet.cpp:1572 in 1d367effa8 outdated

1570 |          fScanningWallet = true;
1571 |  
1572 |          ShowProgress(_("Rescanning..."), 0); // show rescan progress in GUI as dialog or on splashscreen, if -rescan on startup
1573 | -        double dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1574 | -        double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());
1575 | +        double dProgressStart = 0;

promag commented at 9:46 PM on September 18, 2017:

Only chainActive.Tip() needs the lock? So keep old code and:

CBlockIndex* tip = nullptr;
{
    LOCK(cs_main);
    tip = chainActive.Tip();
}

double dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
double dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);

promag commented at 9:48 PM on September 18, 2017:

BTW, this patch allows the tip to be update so dProgressTip should be inside the loop?

in src/wallet/wallet.cpp:1712 in 1d367effa8 outdated

1596 |                  ret = pindex;
1597 |              }
1598 | -            pindex = chainActive.Next(pindex);
1599 | +            {
1600 | +                LOCK(cs_main);
1601 | +                pindex = chainActive.Next(pindex);

theuni commented at 10:00 PM on September 19, 2017:

What if there's a reorg between iterations?

Does anything weird happen if we (for ex): begin scanning, receive a new best block with our tx from p2p, then re-scan here?

Seems to me that we should take a copy of ChainActive, traverse, compare to progress to chainActive.Tip(), and repeat until they match. Something like (just a quick sketch, probably broken):

CBlockIndex* pindex = pindexStart;
CChain chain;
while (!fAbortRescan) {
    {
        LOCK(cs_main);
        if (pindex == chainActive.Tip()) {
            break;
        }
        chain.SetTip(chainActive.Tip());
    }
    if (pindex) {
        // start where we left off.
        const CBlockIndex* fork = chain.FindFork(pindex);
        if (fork != pindex) {
            // Need to undo the orphans.
        }
        pindex = chain.Next(fork);
    }
    while(!fAbortRescan && pindex) {
        /*
        do stuff
        ...
        pindex = chain.Next(pindex);
        */
    }
    pindex = chain.Tip();
}

That almost entirely avoids locking cs_main, while ensuring that we're never jumping chains.

It could also be done with just the pindex and GetAncestor(), to skip the memory overhead of the second chain copy.

jonasschnelli commented at 3:44 AM on September 22, 2017:

I'm not sure if this is really a problem. During rescans, IMO it won't hurt to scan an orphaned block nor does it hurt if you do not scan up to the tip in case you have connected new blocks during the time of re-scanning. This, because those new blocks must also have been scanned by the wallet logic (outside of ScanForWalletTransactions).

laanwj commented at 11:39 AM on November 16, 2017:

If a reorg happens during rescan, we want to continue rescanning using the current chain, not the outdated one.

This, because those new blocks must also have been scanned by the wallet logic (outside of ScanForWalletTransactions).

Indeed. ... I think it works out like this. Though @theuni does put a good point.

promag commented at 3:03 PM on November 27, 2017:

Maybe break the rescan as soon as possible if the above tip is not in the current chain? Then add here:

if (!chainActive.Contains(tip)) break;

jonasschnelli commented at 7:40 PM on November 28, 2017:

In case of a reorg, wouldn't chainActive.Next(pindex); return a nullptr leading to stop the scan, which should be totally fine. During the reorg, the blocks of the new chain must have been scanned by the wallet via ConnectBlock.

promag commented at 7:48 PM on November 28, 2017:

Yes that's true when pindex is not in the chain. But if any later block is not in the chain, why keep rescanning?

jonasschnelli commented at 8:07 PM on November 28, 2017:

It should be detected during the next chainActive.Next(pindex); which, in case we had a fork between the last chainActive.Next(pindex); we would only scan max 1 block... also very unlikely IMO.

promag commented at 12:17 AM on November 29, 2017:

Suppose the following:

A B C D E F G H I J - chain to rescan, tip = J
(A) B C D E F G H I J - scanning block A
A (B) C D E F G H I J - scanning block B
A (B) C D E F G H K L M - scanning block B, reorg after H

Should only rescan to H inclusive? Agree it's an edge case.

BTW, if the tip changes dProgressTip could be updated?

TheBlueMatt commented at 3:32 PM on December 4, 2017:

Yea, I think there is a real issue here. I (believe) it could be fixed by doing the AddToWalletIfInvolvingMe inside a cs_main (which is going to be at least mostly required since ReadBlockFromDisk requires cs_main) with a chainActive.Contains() check before doing the AddToWalletIfInvolvingMe. That should put us back to the original state of anything we add to the wallet is on the main chain and, thus, will either be Disconnected then Connected in the callbacks, only Connected, or not included in a callback at all.

jonasschnelli commented at 7:22 AM on December 6, 2017:

I still don't fully understand the concerns (please help me).

If there is a reorg, all blocks of the new chain will be scanned by the wallet via ConnectBlock (no need to rescan again, we only need to rescan the stuff that doesn't come in via Connect/Disconnect block).
If we scan blocks that are no longer in the main-chain, and we could find a transaction in those blocks that is relevant to our wallet, so be it. The wallet does display such transactions as unconfirmed... same as we would have a reorg outside of the rescan process.

TheBlueMatt commented at 6:50 PM on December 6, 2017:

Heh, sorry, I didnt fully specify my concern because I thought the above discussion did, but I dont think the above comments are correct. As you point out, I do not believe there is a race where you simply miss transactions. However, there is a race where you may end up marking a transaction as in the wrong block:

A -> B -> .... -> C -> ...

Both B and C have the transaction you're interested in.

You go to load block B from a rescan, but it has already been reorg'd and the notificatitons for disconnect(B) and connect(C) have already fired. Thus, you have the transaction marked as in block C, which is correct, but then you finish rescanning and mark the transaction as in block B, which now makes you think the transaction has been conflicted and has negative GetDepthInMainChain.

As I mentioned, this is most simply fixed by adding a "LOCK(cs_main); if (!chainActive.Contains(pindex)) break;" outside the AddToWalletIfInvolvingMe loop.

jonasschnelli commented at 10:45 PM on December 12, 2017:

Fixed

promag commented at 11:54 PM on September 19, 2017: member

Sorry for the bad reference ☝️ ...

jonasschnelli force-pushed on Sep 22, 2017

jonasschnelli commented at 3:49 AM on September 22, 2017: contributor

Fixed @promag's code folding/style issue

jonasschnelli force-pushed on Oct 14, 2017

jonasschnelli commented at 8:53 PM on November 15, 2017: contributor

Reviewers welcome (to make progress with GUI rescan abort #11200), best reviewed with ?w=1

laanwj commented at 11:35 AM on November 16, 2017: member

utACK 726fe69. There might be a slight overhead to locking for every block, likely only relevant for the first blocks of the chain which have few transactions. In any case this is preferable to the current situation.

(removed my utACK for now, this does not get reorgs right in case the order in which blocks are scanned matters, which I'm not sure about)

laanwj renamed this:
~~Avoid pemanent cs_main/cs_wallet lock during RescanFromTime~~
Avoid permanent cs_main/cs_wallet lock during RescanFromTime
on Nov 16, 2017

promag commented at 3:05 PM on November 27, 2017: member

LGTM, will play a bit.

promag commented at 3:12 PM on November 27, 2017: member

BTW, see question #11281 (review).

promag commented at 3:33 PM on November 27, 2017: member

This also allows concurrent rescans correct? It will mess up the progress dialog no?

jonasschnelli added this to the "Blockers" column in a project

laanwj commented at 8:51 AM on November 28, 2017: member

This also allows concurrent rescans correct? It will mess up the progress dialog no?

Concurrent rescans on multiple wallets would be nice, multiple concurrent rescans on one wallet would be dangerous or at least counter-productive (so we might want to track that?). Progress dialog issues can be fixed later.

jonasschnelli force-pushed on Nov 29, 2017

jonasschnelli commented at 8:08 PM on November 29, 2017: contributor

Rebased the PR.

Added checks to make sure only one rescan per time and per wallet can be executed (with appropriate error responses)
Extended the lock reduction to the new rescanblockchain call

jonasschnelli force-pushed on Nov 29, 2017

jonasschnelli commented at 8:12 PM on November 29, 2017: contributor

Best reviewed with w=1. Easy way to test multiple parallel rescans by adding a sleep within the inner rescan loop.

in src/wallet/wallet.cpp:1665 in fbca42a021 outdated

1662 | +            LOCK(cs_main);
1663 | +            tip = chainActive.Tip();
1664 | +        }
1665 |          double dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1666 | -        double dProgressTip = GuessVerificationProgress(chainParams.TxData(), chainActive.Tip());
1667 | +        double dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);

TheBlueMatt commented at 3:01 PM on December 4, 2017:

GuessVerificationProcess needs cs_main (probably best to lock cs_main in GuessVerificationProcess).

in src/wallet/wallet.cpp:1693 in fbca42a021 outdated

1673 | @@ -1669,6 +1674,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1674 |  
1675 |              CBlock block;
1676 |              if (ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
1677 | +                LOCK(cs_wallet);

TheBlueMatt commented at 3:11 PM on December 4, 2017:

(one line up) ReadBlockFromDisk in CBlockIndex* form requires cs_main.

in src/wallet/rpcdump.cpp:435 in d859779cd0 outdated

 431 | @@ -428,6 +432,10 @@ UniValue importpubkey(const JSONRPCRequest& request)
 432 |      if (fRescan && fPruneMode)
 433 |          throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");
 434 |  
 435 | +    if (fRescan && pwallet->IsScanning()) {

TheBlueMatt commented at 3:40 PM on December 4, 2017:

This is very race-y. You could check check this, have another thread call rescan, and then end up with two parallell rescans.

promag commented at 2:16 AM on December 5, 2017: member

This also allows concurrent rescans correct? It will mess up the progress dialog no?

Concurrent rescans on multiple wallets would be nice, multiple concurrent rescans on one wallet would be dangerous or at least counter-productive (so we might want to track that?). Progress dialog issues can be fixed later.

Please see #11826.

ryanofsky commented at 6:21 PM on December 6, 2017: member

Seems like this PR still has some races described above that need to be fixed.

Also, it is unclear to me what the "important show-stopper for #11200" mentioned in the PR description is. The comments in #11200 seems to indicate that there is some existing deadlock that this fixes and #11200 would worsen without this PR. But there's no explanation of how or when the deadlock happens. It's be good to have some clarification about this. (I also asked about it in #11200 (comment)).

jonasschnelli commented at 8:12 PM on December 6, 2017: contributor

@ryanofsky: #11200 won't currently work because the rescan locks cs_main over a longer period. The GUI also tries to lock cs_main via graphical updates on the transaction table as well as the balances.... == unresponsiveness (not actually a deadlock) == you won't be able to abort the rescan because the whole GUI is locked.

jonasschnelli commented at 6:50 AM on December 7, 2017: contributor

Added two commits.

Addressed the race issue found by @promag and @TheBlueMatt (d7f3836b74b790887793997562216885840df7a2)
Update the progress max if the tip has changed (as found by @promag, 9b59847ddca46d30404ca574cba32b6de214844f)

ryanofsky commented at 7:35 AM on December 7, 2017: member

Is this still WIP? I don't see changes or responses for 3 other races: IsScanning, GuessVerificationProcess, ReadBlockFromDisk

jonasschnelli commented at 7:55 AM on December 7, 2017: contributor

Added a commit that fixes the remaining missing locks mentioned by @TheBlueMatt, expect the racy IsScanning(). Would it make sense to read and set (protected by a internal lock) the scanning flag right after checking (something like checkAndEventuallySetScanning())? Other ideas?

TheBlueMatt commented at 3:43 PM on December 8, 2017: member

re: race: I certainly appreciate the RPC exception before importing in case of an existing scan. It might be nice to get some RAII-like lock on the ability to scan, ie like your suggestion but you create a class instance which gives you the right to rescan instead of having to make sure you set some boolean to false if you decide you don't want to rescan.

jonasschnelli commented at 9:09 PM on December 8, 2017: contributor

Added an RAII object to preserve wallet rescan races.

jonasschnelli force-pushed on Dec 8, 2017

TheBlueMatt commented at 6:28 PM on December 9, 2017: member

It appears test_bitcoin.exe hung. Not sure if PR-related or some other issue, but I'm not aware of any issues with such hangs on master atm?

jonasschnelli commented at 6:11 AM on December 10, 2017: contributor

@TheBlueMatt: where did it hung? Travis seems to be happy?

jonasschnelli force-pushed on Dec 11, 2017

jonasschnelli force-pushed on Dec 12, 2017

jonasschnelli commented at 7:26 PM on December 12, 2017: contributor

Rebased and cleaned up commit history.

TheBlueMatt commented at 8:17 PM on December 12, 2017: member

It may be nice to make a WalletRescanReserver a required argument to ScanForWalletTransactions/RescanFromTime to enforce correctness.

in src/wallet/wallet.cpp:1708 in 8f0e976df2 outdated

1702 | @@ -1703,6 +1703,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1703 |              {
1704 |                  LOCK(cs_main);
1705 |                  pindex = chainActive.Next(pindex);
1706 | +                if (pindex && !chainActive.Contains(pindex)) break;

TheBlueMatt commented at 8:18 PM on December 12, 2017:

This check needs to be inside a LOCK2(cs_main, cs_wallet) in the same block as AddToWalletIfInvolvingMe.

jonasschnelli commented at 8:41 PM on December 13, 2017:

Fixed.

jonasschnelli force-pushed on Dec 12, 2017

jonasschnelli commented at 11:15 PM on December 12, 2017: contributor

Followed @TheBlueMatt advice and made the WalletRescanReserver an argument of ScanForWalletTransactions and RescanFromTime to ensure on code level that the wallet rescan is reserved before scanning.

TheBlueMatt commented at 5:05 PM on December 13, 2017: member

See comment github is hiding at #11281 (review). Also, could you squash "Make sure we rescan only blocks in the main chain" into the first commit? Otherwise the first commit is "wrong", which is annoying in review.

jonasschnelli force-pushed on Dec 13, 2017

jonasschnelli commented at 9:08 PM on December 13, 2017: contributor

Added a commit ff54709 to allow to call ReadBlockFromDisk() without holding cs_main (push the lock down for CDiskBlockPos::GetBlockPos()).

in src/wallet/wallet.cpp:1703 in ff547097b3 outdated

1696 | @@ -1684,7 +1697,15 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1697 |              if (pindex == pindexStop) {
1698 |                  break;
1699 |              }
1700 | -            pindex = chainActive.Next(pindex);
1701 | +            {
1702 | +                LOCK(cs_main);
1703 | +                pindex = chainActive.Next(pindex);
1704 | +                if (pindex && !chainActive.Contains(pindex)) break;

TheBlueMatt commented at 10:47 PM on December 13, 2017:

This line is a noop - chainActive.Next() is never going to return something non-nullptr that isn't on the best chain.

jonasschnelli commented at 11:20 PM on December 13, 2017:

Oops. This is a rebase issue, ... removing this line now.

in src/wallet/wallet.cpp:1691 in ff547097b3 outdated

1690 |              }
1691 |  
1692 |              CBlock block;
1693 |              if (ReadBlockFromDisk(block, pindex, Params().GetConsensus())) {
1694 | +                LOCK2(cs_main, cs_wallet);
1695 | +                if (pindex && !chainActive.Contains(pindex)) break;

TheBlueMatt commented at 10:58 PM on December 13, 2017:

There is a weird case here that I think should be documented - something in the description of ScanForWalletTransactions should mention that, in order to ensure scan is "successful", pindex must have been/be on the main chain at some point after the addition of any keys which we want to rescan for.

jonasschnelli commented at 11:45 PM on December 13, 2017:

Yes. That case is a bit wired. We should probably return a custom state object instead a single CBlockIndex in the long run. Mentioned that behaviour now in the ScanForWalletTransactions comment.

in src/wallet/rpcwallet.cpp:3409 in ff547097b3 outdated

3390 | -        if (!pindexStart) {
3391 | -            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid start_height");
3392 | -        }
3393 | -    }
3394 | +    CBlockIndex *pChainTip = nullptr;
3395 | +    {

TheBlueMatt commented at 11:00 PM on December 13, 2017:

I think you meant to put a LOCK(cs_main) at the top of this scope?

jonasschnelli commented at 11:44 PM on December 13, 2017:

Oh. Indeed! Added. Fixed.

in src/wallet/wallet.cpp:1716 in ff547097b3 outdated

1702 | +                LOCK(cs_main);
1703 | +                pindex = chainActive.Next(pindex);
1704 | +                if (pindex && !chainActive.Contains(pindex)) break;
1705 | +                if (tip != chainActive.Tip()) {
1706 | +                    // in case the tip has changed, update progress max
1707 | +                    dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);

TheBlueMatt commented at 11:02 PM on December 13, 2017:

err, didnt you mean to set tip to chainActive.Tip() first?

jonasschnelli commented at 11:44 PM on December 13, 2017:

Argh. Thanks. Fixed.

jonasschnelli force-pushed on Dec 13, 2017

in src/wallet/wallet.cpp:1649 in 61315d1455 outdated

1644 | @@ -1644,37 +1645,52 @@ int64_t CWallet::RescanFromTime(int64_t startTime, bool update)
1645 |   *
1646 |   * If pindexStop is not a nullptr, the scan will stop at the block-index
1647 |   * defined by pindexStop
1648 | + *
1649 | + * An additional check if the returned CBlockIndex* is/was on the main chain

TheBlueMatt commented at 3:59 PM on December 14, 2017:

I dont think that fully captures the requirement here. Its actually rather loose, y ou just need to make sure pindexStop (and pindexStart) are on the main chain after to the addition of any new keys you want to detect transactions for. What is returned actually doesnt matter because if pindexStart and pindexStop were on the main chain at some point when all the neccessary keys were in the wallet, any missed blocks during rescan were also caught in the validationinterface callbacks.

jonasschnelli commented at 7:06 AM on December 15, 2017:

Updated the comment.

jonasschnelli force-pushed on Dec 15, 2017

in src/wallet/rpcwallet.cpp:3435 in eba5308066 outdated

3422 |      }
3423 |  
3424 |      // We can't rescan beyond non-pruned blocks, stop and throw an error
3425 |      if (fPruneMode) {
3426 | -        CBlockIndex *block = pindexStop ? pindexStop : chainActive.Tip();
3427 | +        CBlockIndex *block = pindexStop ? pindexStop : pChainTip;

TheBlueMatt commented at 7:37 PM on December 15, 2017:

The nStatus check two lines down still needs cs_main.

jonasschnelli commented at 10:02 PM on December 15, 2017:

Right! Fixed by adding a cs_main lock for this block (only occurs for pruned peers).

TheBlueMatt commented at 8:07 PM on December 15, 2017: member

utACK eba53080666402bba2efe4a430bde2b824edef9d except the one minor missing lock.

in src/wallet/rpcdump.cpp:161 in 22510c9a82 outdated

 184 |  
 185 | -        if (fRescan) {
 186 | -            pwallet->RescanFromTime(TIMESTAMP_MIN, true /* update */);
 187 |          }
 188 |      }
 189 | +    if (fRescan) {

sipa commented at 9:18 PM on December 15, 2017:

Releasing cs_wallet here means another RPC could occur in between which observes the key being present, but not the relevant transactions. Is that a concern?

TheBlueMatt commented at 9:23 PM on December 15, 2017:

I wouldnt think so directly. What may be of larger concern may be that you could get a bogus balance due to having found old transactions but not newer ones. Its probably still fine, but we should note it in the help doc for the rescanning functions. Alternatively we could block other wallet RPC calls until rescan finishes.

jonasschnelli commented at 10:00 PM on December 15, 2017:

Possible solutions: 1.) User must take care: report the rescan status in getwalletinfo() 2.) Block relevant RPC calls during an active rescan...

I think 1) is more flexible but my introduce some pitfalls...

promag commented at 11:51 PM on December 19, 2017:

@sipa that is already possible: importprivkey rescan=false followed by rescanblockchain.

sipa commented at 12:03 AM on December 20, 2017:

@promag If you sent rescan to false you're taking that risk yourself obviously. My point is that if you do request a rescan, there is a reasonable expectation to not see inconsistent results.

promag commented at 12:10 AM on December 20, 2017:

Yeah I know, there is also reasonable expectation to have the RPC responsive 😄

promag commented at 12:14 AM on December 20, 2017:

The same happens in importmulti, where the locks are held only when importing the keys.

jonasschnelli force-pushed on Dec 15, 2017

heri99 commented at 10:05 PM on December 15, 2017: none

Sory for that

On 16 Dec 2017 5:24 am, "Matt Corallo" notifications@github.com wrote:

@TheBlueMatt commented on this pull request.

In src/wallet/rpcdump.cpp https://github.com/bitcoin/bitcoin/pull/11281#discussion_r157302011:
     }
 }
if (fRescan) {

I wouldnt think so directly. What may be of larger concern may be that you could get a bogus balance due to having found old transactions but not newer ones. Its probably still fine, but we should note it in the help doc for the rescanning functions. Alternatively we could block other wallet RPC calls until rescan finishes.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11281#discussion_r157302011, or mute the thread https://github.com/notifications/unsubscribe-auth/AT0amrJB8ZTspbPKK86FyFNuKI74wYBeks5tAuN3gaJpZM4PQhYL .

in src/wallet/wallet.h:1232 in e56aa6ce6b outdated

1227 | +public:
1228 | +    CWalletRef m_wallet;
1229 | +    bool m_could_reserve;
1230 | +    explicit WalletRescanReserver(CWalletRef w): m_wallet(w), m_could_reserve(false) {}
1231 | +
1232 | +    bool reserve() {

promag commented at 2:36 AM on December 17, 2017:

Nit, { in new line.

in src/wallet/wallet.h:1230 in e56aa6ce6b outdated

1225 | +class WalletRescanReserver
1226 | +{
1227 | +public:
1228 | +    CWalletRef m_wallet;
1229 | +    bool m_could_reserve;
1230 | +    explicit WalletRescanReserver(CWalletRef w): m_wallet(w), m_could_reserve(false) {}

promag commented at 2:37 AM on December 17, 2017:

Nit, missing space before :.

in src/wallet/wallet.h:1274 in e56aa6ce6b outdated

1220 | @@ -1218,4 +1221,31 @@ bool CWallet::DummySignTx(CMutableTransaction &txNew, const ContainerType &coins
1221 |      return true;
1222 |  }
1223 |  
1224 | +/** RAII object to check and reserve a wallet rescan */
1225 | +class WalletRescanReserver
1226 | +{
1227 | +public:
1228 | +    CWalletRef m_wallet;

promag commented at 2:37 AM on December 17, 2017:

Make these private?

jonasschnelli commented at 6:47 AM on December 21, 2017:

Done.

in src/wallet/wallet.h:673 in e56aa6ce6b outdated

 655 | @@ -656,6 +656,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 656 |      static std::atomic<bool> fFlushScheduled;
 657 |      std::atomic<bool> fAbortRescan;
 658 |      std::atomic<bool> fScanningWallet;
 659 | +    std::mutex mutexScanning;

promag commented at 2:42 AM on December 17, 2017:

Why the mutex when fScanningWallet is std::atomic? Could use std::atomic<bool>::compare_exchange_strong?

jonasschnelli commented at 11:26 PM on December 19, 2017:

Yes. Maybe. But lets keep this for now.

laanwj added this to the milestone 0.16.0 on Dec 18, 2017

jonasschnelli force-pushed on Dec 19, 2017

jonasschnelli commented at 11:30 PM on December 19, 2017: contributor

Fixed @promag's nits

promag commented at 12:01 AM on December 20, 2017: member

Squash Make sure WalletRescanReserver has successfully reserved the rescan into Add RAII wallet rescan reserver?

TheBlueMatt commented at 5:11 AM on December 23, 2017: member

Needs rebase. Also I believe still needs (at least) doc fixes to address @sipa's comments at #11281 (review)

promag commented at 4:25 PM on January 2, 2018: member

Needs rebase.

jonasschnelli force-pushed on Jan 5, 2018

jonasschnelli commented at 8:46 PM on January 5, 2018: contributor

Rebased and added a commit that adds mentioning of the concern reported by @sipa (https://github.com/bitcoin/bitcoin/pull/11281#discussion_r157300953, other RPC calls may report keys as imported but related transactions are still not there because of the ongoing rescan)

in src/wallet/wallet.h:1293 in b2cc702095 outdated

1241 | +        m_could_reserve = true;
1242 | +        return true;
1243 | +    }
1244 | +
1245 | +    bool isReserved() const {
1246 | +        return (m_could_reserve && m_wallet->fScanningWallet);

promag commented at 12:06 PM on January 9, 2018:

return m_could_reserve is enough? WalletRescanReserver is the "owner.

jonasschnelli commented at 8:02 PM on January 15, 2018:

I think it's better for future changes to keep the check here... don't hurt but may protect from a future mistake.

ryanofsky commented at 9:48 PM on January 22, 2018:

In commit "Make sure WalletRescanReserver has successfully reserved".

Alternative could be to just return m_could_reserve and assert !m_could_reserve || fScanningWallet, so if there is a future mistake, it can be detected instead of papered over.

Also formatting here is inconsistent, opening brace for function goes on new line.

in src/wallet/wallet.h:1282 in b2cc702095 outdated

1231 | +public:
1232 | +    explicit WalletRescanReserver(CWalletRef w) : m_wallet(w), m_could_reserve(false) {}
1233 | +
1234 | +    bool reserve()
1235 | +    {
1236 | +        std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);

promag commented at 12:06 PM on January 9, 2018:

Add assert(!m_could_reserve);.

jonasschnelli commented at 8:02 PM on January 15, 2018:

Added

in src/wallet/wallet.cpp:1666 in b2cc702095 outdated

1659 |      CBlockIndex* pindex = pindexStart;
1660 |      CBlockIndex* ret = nullptr;
1661 |      {
1662 | -        LOCK2(cs_main, cs_wallet);
1663 |          fAbortRescan = false;
1664 |          fScanningWallet = true;

promag commented at 12:19 PM on January 9, 2018:

Remove, since reserver already set fScanningWallet to true.

promag commented at 12:21 PM on January 9, 2018:

Should also remove fScanningWallet = false at the end?

jonasschnelli commented at 8:07 PM on January 15, 2018:

Ah. I see your point. Removed those sets and mentioned that fScanningWallet is controlled by WalletRescanReserver.

in src/wallet/wallet.h:672 in b2cc702095 outdated

 656 | @@ -656,6 +657,9 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 657 |      static std::atomic<bool> fFlushScheduled;
 658 |      std::atomic<bool> fAbortRescan;
 659 |      std::atomic<bool> fScanningWallet;

promag commented at 12:23 PM on January 9, 2018:

Add a comment saying this is controlled by WalletRescanReserver.

jonasschnelli commented at 8:02 PM on January 15, 2018:

It's not really controlled by WalletRescanReserver. This was added before and ScanForWalletTransactions sets it to true.

TheBlueMatt commented at 5:37 PM on January 11, 2018: member

Needs rebase and to address @sipa's note. Personally I think just documenting the case is sufficient.

jonasschnelli force-pushed on Jan 11, 2018

jonasschnelli commented at 7:29 PM on January 11, 2018: contributor

Rebased (non trivial). Another review would help. Thanks

in src/wallet/rpcdump.cpp:153 in b5aaefb2d5 outdated

 169 | -        pwallet->LearnAllRelatedScripts(pubkey);
 170 | +        CKey key = vchSecret.GetKey();
 171 | +        if (!key.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Private key outside allowed range");
 172 |  
 173 | -        // whenever a key is imported, we need to scan the whole chain
 174 | -        pwallet->UpdateTimeFirstKey(1);

TheBlueMatt commented at 9:33 PM on January 11, 2018:

This line didn't make it into the new rebase, I believe.

jonasschnelli commented at 11:30 PM on January 11, 2018:

Oh. Yes. Thanks Will fix

jonasschnelli commented at 6:21 AM on January 12, 2018:

Fixed.

TheBlueMatt commented at 6:45 PM on January 14, 2018:

nit: Why'd you move it up in the function instead of keeping its position?

jonasschnelli force-pushed on Jan 12, 2018

in src/wallet/rpcwallet.cpp:3444 in e43105c17a outdated

3439 | @@ -3430,21 +3440,23 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
3440 |          }
3441 |      }
3442 |  
3443 | -    CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, true);
3444 | -    if (!stopBlock) {
3445 | -        if (pwallet->IsAbortingRescan()) {
3446 | -            throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
3447 | +    CBlockIndex *stopBlock = nullptr;
3448 | +    {

TheBlueMatt commented at 7:05 PM on January 14, 2018:

nit: why scope this? There's no lock to scope, could you not just leave this as-is and only change the chainActive.Tip() call?

jonasschnelli commented at 7:46 PM on January 15, 2018:

Right... fixing.

jonasschnelli commented at 8:07 PM on January 15, 2018:

Your right,.. removed the unnecessary scope.

in src/wallet/wallet.cpp:1674 in e43105c17a outdated

1673 | +        double dProgressTip;
1674 | +        {
1675 | +            LOCK(cs_main);
1676 | +            tip = chainActive.Tip();
1677 | +            dProgressStart = GuessVerificationProgress(chainParams.TxData(), pindex);
1678 | +            dProgressTip = GuessVerificationProgress(chainParams.TxData(), tip);

TheBlueMatt commented at 7:10 PM on January 14, 2018:

Not changed in this PR, but it may be nice (here or in a follow-up before we start showing rescan as a progress bar in more places) to not use the tip here and instead use pindexStop. It makes sense to always give progress towards tip if there are (potentially) API clients which are calling with a stop a ways back from our tip.

jonasschnelli commented at 7:46 PM on January 15, 2018:

Lets leave this open for a possible followup PR (out of scope here)

promag commented at 10:13 AM on January 24, 2018:

Agree with @TheBlueMatt, the follow the tip thing could be elsewhere (another PR). @TheBlueMatt do you suggest to rescan up to pIndexStop even if at some point a block is no longer in the active chain?

in src/wallet/wallet.cpp:1714 in e43105c17a outdated

1703 | @@ -1683,7 +1704,15 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1704 |              if (pindex == pindexStop) {
1705 |                  break;
1706 |              }
1707 | -            pindex = chainActive.Next(pindex);
1708 | +            {
1709 | +                LOCK(cs_main);
1710 | +                pindex = chainActive.Next(pindex);
1711 | +                if (tip != chainActive.Tip()) {
1712 | +                    tip = chainActive.Tip();

TheBlueMatt commented at 7:13 PM on January 14, 2018:

Same goes for here - while this check makes the behavior not change, I'm not sure the current behavior is exactly optimal...we scan until pindexStop, not chainActive.Tip().

in src/wallet/rpcdump.cpp:90 in 0dad810ade outdated

  85 | @@ -86,7 +86,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
  86 |              "1. \"privkey\"          (string, required) The private key (see dumpprivkey)\n"
  87 |              "2. \"label\"            (string, optional, default=\"\") An optional label\n"
  88 |              "3. rescan               (boolean, optional, default=true) Rescan the wallet for transactions\n"
  89 | -            "\nNote: This call can take minutes to complete if rescan is true.\n"
  90 | +            "\nNote: This call can take minutes to complete if rescan is true, during that time, other rpc calls\n"
  91 | +            "may report that the imported key exists but related transactions are still missing.\n"

TheBlueMatt commented at 7:22 PM on January 14, 2018:

You should probably add a ", leading to temporarily incorrect/bogus balances and unspent outputs until rescan completes" to the warning message.

jonasschnelli commented at 7:58 PM on January 15, 2018:

Fixed by adding @TheBlueMatt's proposed sentence-part.

TheBlueMatt commented at 7:23 PM on January 14, 2018: member

Aside from needing to add a similar warning message to rescanblockchain, utACK 0dad810adeb3bbb6a60b722240ee2b90136e8384. There are a bunch of comments here, but most of them can be addressed in a follow-up just fine.

jonasschnelli force-pushed on Jan 15, 2018

Sjors commented at 4:08 PM on January 19, 2018: member

Not sure what exactly to test here, but at least I can confirm bitcoin-cli abortrescan still works and there's no noticeable difference in QT (I assume that's good).

in src/wallet/rpcwallet.cpp:3456 in 48a57eba34 outdated

3454 |      }
3455 | -
3456 |      UniValue response(UniValue::VOBJ);
3457 |      response.pushKV("start_height", pindexStart->nHeight);
3458 | -    response.pushKV("stop_height", stopBlock->nHeight);
3459 | +    response.pushKV("stop_height", stopBlock ? stopBlock->nHeight : 0);

ryanofsky commented at 8:37 PM on January 22, 2018:

In commit "Avoid permanent cs_main"

Strange that this line is changing while the line above isn't. Seems impossible for pindexStart or stopBlock to be null, and odd to be adding a check now only for stopBlock.

jonasschnelli commented at 6:25 AM on January 24, 2018:

Yes. Your right. Fixed (removed the extra nullptr check).

in src/wallet/wallet.cpp:1617 in 48a57eba34 outdated

1613 | @@ -1614,14 +1614,15 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
1614 |   */
1615 |  int64_t CWallet::RescanFromTime(int64_t startTime, bool update)
1616 |  {
1617 | -    AssertLockHeld(cs_main);

ryanofsky commented at 8:38 PM on January 22, 2018:

In commit "Avoid permanent cs_main"

Could this now assert lock not held, instead of not asserting at all?

jonasschnelli commented at 6:17 AM on January 24, 2018:

I don't think we should enforce not locking cs_main at this point.

in src/wallet/wallet.cpp:1650 in 48a57eba34 outdated

1643 | @@ -1643,6 +1644,10 @@ int64_t CWallet::RescanFromTime(int64_t startTime, bool update)
1644 |   *
1645 |   * If pindexStop is not a nullptr, the scan will stop at the block-index
1646 |   * defined by pindexStop
1647 | + *
1648 | + * Caller needs to make sure pindexStop (and the optional pindexStart) are on
1649 | + * the main chain after to the addition of any new keys you want to detect
1650 | + * transactions for.

ryanofsky commented at 8:47 PM on January 22, 2018:

In commit "Avoid permanent cs_main"

I guess this sentence is added in response to: #11281 (review), but I don't understand what it is saying. Since the lock is released, how is the caller supposed to "make sure" pindexStop is on the main chain when it may not be? Also, I think part of the comment "after to the addition of any new keys you want to detect transactions for" should be be removed, it doesn't seem to be adding any information, and not every caller is even adding keys (e.g. bestblock startup code).

Probably this should just say that the rescan will abort if there is a reorg and scanning proceeded past the fork point on the wrong chain. Alternately, I don't think it would be hard to clean up this behavior by doing something like Cory suggested: #11281 (review), but this could be saved for another PR.

jonasschnelli commented at 6:26 AM on January 24, 2018:

I keep that comment for now and leave it open for another PR. The comment was directly proposed (word by word) by @TheBlueMatt.

in src/wallet/wallet.cpp:1680 in 48a57eba34 outdated

1679 | +        }
1680 |          while (pindex && !fAbortRescan)
1681 |          {
1682 | -            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0)
1683 | +            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0) {
1684 | +                LOCK(cs_main);

ryanofsky commented at 8:57 PM on January 22, 2018:

In commit "Avoid permanent cs_main"

Seems like cs_main only needs to be held for GuessVerificationProgress but not ShowProgress? Might be good to reduce lock scope more, or have comment if it can't be reduced.

jonasschnelli commented at 6:26 AM on January 24, 2018:

Good pont. Fixed.

in src/wallet/wallet.cpp:1697 in 48a57eba34 outdated

1697 | +                LOCK(cs_main);
1698 | +                readRet = ReadBlockFromDisk(block, pindex, Params().GetConsensus());
1699 | +            }
1700 | +            if (readRet) {
1701 | +                LOCK2(cs_main, cs_wallet);
1702 | +                if (pindex && !chainActive.Contains(pindex)) break;

ryanofsky commented at 9:15 PM on January 22, 2018:

In commit "Avoid permanent cs_main"

Because of the break here, the "Returns null if scan was successful" documentation is no longer correct. Would suggest changing so it's still easily possible to distinguish success from failure:

if (pindex && !chainActive.Contains(pindex)) {
    ret = pIndex;
    break;
}

Also, the reason for this check is obscure enough (https://github.com/bitcoin/bitcoin/pull/11281#discussion_r155326447) that it I think it deserves some kind of comment. I would suggest something like "Abort scan if current block is no longer active, to prevent marking transactions as coming from the wrong block."

jonasschnelli commented at 6:28 AM on January 24, 2018:

Nice catch! Thanks. Fixed.

ryanofsky commented at 9:51 PM on January 22, 2018: member

utACK 7aeeb11a2e6fe2671973ef18f556dbaf32ad5f81. Would be nice to see this merged.

I left various comments, and I think some of them are important enough that they should be followed up on, but this can happen in future PRs.

Avoid pemanent cs_main/cs_wallet lock during wallet rescans 8d0b610fe8

Add RAII wallet rescan reserver dbf8556b4d

Make sure WalletRescanReserver has successfully reserved the rescan bc356b4268

Reduce cs_main lock in ReadBlockFromDisk, only read GetBlockPos under the lock ccd8ef65f9

Mention that other RPC calls report keys as "imported" while txns are still missing 7f812502b7

jonasschnelli force-pushed on Jan 24, 2018

in src/wallet/wallet.h:1298 in 7f812502b7

1293 | +        return (m_could_reserve && m_wallet->fScanningWallet);
1294 | +    }
1295 | +
1296 | +    ~WalletRescanReserver()
1297 | +    {
1298 | +        std::lock_guard<std::mutex> lock(m_wallet->mutexScanning);

promag commented at 9:38 AM on January 24, 2018:

Lock inside if.

in src/wallet/wallet.h:1293 in 7f812502b7

1288 | +        return true;
1289 | +    }
1290 | +
1291 | +    bool isReserved() const
1292 | +    {
1293 | +        return (m_could_reserve && m_wallet->fScanningWallet);

promag commented at 9:46 AM on January 24, 2018:

Just return m_could_reserve;?

in src/wallet/wallet.h:672 in 7f812502b7

 668 | @@ -668,7 +669,10 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
 669 |  private:
 670 |      static std::atomic<bool> fFlushScheduled;
 671 |      std::atomic<bool> fAbortRescan;
 672 | -    std::atomic<bool> fScanningWallet;
 673 | +    std::atomic<bool> fScanningWallet; //controlled by WalletRescanReserver

promag commented at 9:48 AM on January 24, 2018:

Since it's guarded by mutexScanning, drop std::atomic?

in src/wallet/wallet.cpp:1652 in 7f812502b7

1648 | + * Caller needs to make sure pindexStop (and the optional pindexStart) are on
1649 | + * the main chain after to the addition of any new keys you want to detect
1650 | + * transactions for.
1651 |   */
1652 | -CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, bool fUpdate)
1653 | +CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver &reserver, bool fUpdate)

promag commented at 10:02 AM on January 24, 2018:

Nit, space WalletRescanReserver& reserver.

promag commented at 11:06 AM on January 24, 2018: member

Tested ACK 7f81250, some minor comments though.

Agree with @ryanofsky, for 0.16 it should be merged rather sooner than later.

promag commented at 11:07 AM on January 24, 2018: member

Forgot to mention, it is still not possible to abort the rescan at initialisation time, I guess the goal is to not change that.

laanwj commented at 11:47 AM on January 24, 2018: member

Forgot to mention, it is still not possible to abort the rescan at initialisation time, I guess the goal is to not change that.

I don't think that's a goal here? Let's not extend the scope beyond what is in the OP.

utACK 7f81250

laanwj merged this on Jan 24, 2018

laanwj closed this on Jan 24, 2018

laanwj referenced this in commit 8470e64724 on Jan 24, 2018

in src/wallet/wallet.cpp:1681 in 7f812502b7

1684 | -            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0)
1685 | -                ShowProgress(_("Rescanning..."), std::max(1, std::min(99, (int)((GuessVerificationProgress(chainParams.TxData(), pindex) - dProgressStart) / (dProgressTip - dProgressStart) * 100))));
1686 | +            if (pindex->nHeight % 100 == 0 && dProgressTip - dProgressStart > 0.0) {
1687 | +                double gvp = 0;
1688 | +                {
1689 | +                    LOCK(cs_main);

ryanofsky commented at 7:01 PM on January 25, 2018:

@jonasschnelli, is this lock necessary? It seems like either this should be removed, or else a lock needs to be added below at line 1721 (https://github.com/bitcoin/bitcoin/pull/11281/files#diff-b2bb174788c7409b671c46ccc86034bdR1721) if cs_main is required to call GuessVerificationProgress.

jonasschnelli commented at 7:43 PM on January 25, 2018:

I think GVP needs cs_main (@TheBlueMatt?), so, yes, your right, L1721 should also have one... working on a fixup.

ryanofsky commented at 7:48 PM on January 25, 2018:

Great, thanks for addressing this.

I think GVP needs cs_main

I was curious about this. It would be good for GuessVerificationProgress comment to mention that it needs cs_main, since it isn't really obvious one way or the other.

in src/wallet/wallet.cpp:1712 in 7f812502b7

1706 | @@ -1683,14 +1707,20 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1707 |              if (pindex == pindexStop) {
1708 |                  break;
1709 |              }
1710 | -            pindex = chainActive.Next(pindex);
1711 | +            {
1712 | +                LOCK(cs_main);
1713 | +                pindex = chainActive.Next(pindex);

ryanofsky commented at 7:44 PM on January 25, 2018:

I think there is still a bug here where scan will return success (nullptr) if it gets aborted due to a reorg.

Maybe this line:

pindex = chainActive.Next(pindex);

should be changed to something like:

if (CBlockIndex* new_index = chainActive.Next(pindex)) {
    pindex = new_index;
} else {
    /* aborting, reorg */
    ret = pindex;
    break;
}

Another alternative could be to resume instead of aborting on reorgs.

ryanofsky commented at 8:09 PM on January 25, 2018:

should be changed to something like:

I think my suggestion above won't work because it will return an error when pindex points to the the tip. Maybe simplest fix would be to restructure the code a little to just acquire cs_main once after ReadBlockFromDisk instead of acquiring, releasing, then immediately re-acquiring. That way there is would be no chance of reorg between the chainActive.Contains check above and this chainActive.Next call.

ryanofsky commented at 3:22 PM on January 26, 2018:

I think there is still a bug here where scan will return success

Sorry for the churn, but I no longer think there's a bug here. Opened #12275 to try to clarify the return value.

ryanofsky referenced this in commit d3b58f0cfb on Jan 26, 2018

ryanofsky referenced this in commit 8add443be1 on Jan 26, 2018

fanquake removed this from the "Blockers" column in a project

laanwj referenced this in commit f50e675e9e on Feb 13, 2018

laanwj referenced this in commit 5c910f31f7 on Feb 13, 2018

laanwj referenced this in commit 34dd7b6ede on Feb 13, 2018

laanwj referenced this in commit 9c3ce7aed8 on Feb 13, 2018

laanwj referenced this in commit 52f8a11978 on Feb 14, 2018

laanwj referenced this in commit 1d4cbd26e4 on Feb 15, 2018

laanwj referenced this in commit 4d54e7ad41 on Feb 15, 2018

Willtech referenced this in commit 0a4ec00af8 on Feb 23, 2018

jonasschnelli referenced this in commit bf3353de90 on Feb 25, 2018

HashUnlimited referenced this in commit deb02e491b on Mar 16, 2018

ryanofsky referenced this in commit 77a03151b5 on Apr 11, 2018

ccebrecos referenced this in commit 2ecd9297dd on Sep 14, 2018

lionello referenced this in commit 3b40394c9d on Nov 7, 2018

PastaPastaPasta referenced this in commit ea808be82d on Apr 13, 2020

UdjinM6 referenced this in commit 8ade2f7957 on Apr 14, 2020

PastaPastaPasta referenced this in commit 6b43c67c05 on Jun 9, 2020

PastaPastaPasta referenced this in commit 0ad89acaee on Jun 9, 2020

PastaPastaPasta referenced this in commit 3773cad14d on Jun 10, 2020

PastaPastaPasta referenced this in commit 31a01211c1 on Jun 10, 2020

PastaPastaPasta referenced this in commit fe6b44d228 on Jun 11, 2020

PastaPastaPasta referenced this in commit f1ccbca7e3 on Jun 11, 2020

martinus referenced this in commit 62f1a5b115 on Jan 25, 2021

martinus referenced this in commit ab79038ef9 on Jan 26, 2021

martinus referenced this in commit 5fece5bdb6 on Jan 30, 2021

martinus referenced this in commit 95dccd1ad9 on Feb 13, 2021

martinus referenced this in commit 4723c0c45a on Feb 14, 2021

martinus referenced this in commit d440918214 on Feb 14, 2021

ckti referenced this in commit 448bc956f6 on Mar 28, 2021

furszy referenced this in commit 169260c6aa on Apr 13, 2021

gades referenced this in commit d0be641481 on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

Avoid permanent cs_main/cs_wallet lock during RescanFromTime #11281

@TheBlueMatt commented on this pull request.