rpc/blockchain: Reset scantxoutset progress before inferring descriptors #19362

1. 
   prusnak commented at 11:56 AM on June 23, 2020: contributor

   Fixes #19361 by moving resetting the g_scan_progress variable before inferring the descriptors

2. fanquake added the label RPC/REST/ZMQ on Jun 23, 2020
3. 
   promag commented at 10:18 PM on June 25, 2020: member

   Code review ACK 71fee9cd12898c74b46659a3e16c90df051af632.

   There's still a slight chance of getting non zero progress. See b6b1945005a8205af6781adb8cc258f629417f25 for an alternative.

   IMO could be backport.

4. 
   prusnak commented at 8:51 AM on June 27, 2020: contributor

   At first sight, it seems that your patch b6b1945005a8205af6781adb8cc258f629417f25 makes more sense.

5. luke-jr approved
6. 
   luke-jr commented at 10:52 PM on July 23, 2020: member

   Still a race, but I don't see an easy fix and this is probably good enough

   utACK

7. 
   luke-jr commented at 10:53 PM on July 23, 2020: member

   Oh, @promag's solution does look better though.

8. rpc: reset scantxoutset progress on finish 8c4129b454
9. prusnak force-pushed on Jul 24, 2020
10. 
    prusnak commented at 9:05 AM on July 24, 2020: contributor

    I replaced my commit with the change suggested @promag in 8c4129b4540f4f739413ed9a6fbfc78afc252f42

11. 
    promag commented at 9:23 AM on July 24, 2020: member

    ACK

12. in src/rpc/blockchain.cpp:2024 in 8c4129b454

    2020 | @@ -2021,13 +2021,15 @@ class CoinsViewScanReserver
    2021 |          if (g_scan_in_progress.exchange(true)) {
    2022 |              return false;
    2023 |          }
    2024 | +        CHECK_NONFATAL(g_scan_progress == 0);
    

    ---

    ---

    

    luke-jr commented at 7:21 AM on August 1, 2020:

    If this were to fail, wouldn't it likely block all future reserves?

    ---

    

    promag commented at 9:11 AM on August 1, 2020:

    You mean this should be fatal?

    ---

    

    luke-jr commented at 3:37 PM on August 1, 2020:

    Since it should be impossible, maybe simplest solution is to just remove it.

    Otherwise, it might need something like:

            if (g_scan_progress) {
                error("ERROR: g_scan_progress was %s when it should be 0, in %s", g_scan_progress, __func__);
                g_scan_progress = 0;
            }
    

    Or:

            if (g_scan_progress) {
                g_scan_in_progress = false;
                CHECK_NONFATAL(g_scan_progress == 0);
            }
    

    ---

    

    promag commented at 9:06 PM on August 1, 2020:

    Then this should be assert(g_scan_progress == 0). I don't think all RPC code should use CHECK_NONFATAL, at least #17192 doesn't say it.

    ---

    

    MarcoFalke commented at 9:18 AM on June 25, 2021:

    You can restart your node if this fails, but I don't think there is a need to crash the node.

13. luke-jr referenced this in commit ebcb7a2aaf on Aug 15, 2020
14. 
    prusnak commented at 10:50 PM on November 23, 2020: contributor

    Can we decide what to do with this one?

    Should we just drop the CHECK_NONFATAL(g_scan_progress == 0); line?

    I really want this to get into 0.21 @luke-jr @promag

15. 
    promag commented at 11:03 PM on November 23, 2020: member

    Should we just drop the CHECK_NONFATAL(g_scan_progress == 0); line? @prusnak I tend to like these invariants, it makes easier to understand the code. I think @luke-jr concern could be addressed by:

    const int scan_progress = g_scan_progress.exchange(0);
    CHECK_NONFATAL(scan_progress == 0);
    

    I think @jonasschnelli, @MarcoFalke, @laanwj should take a look too for 0.21.

16. 
    jonatack commented at 10:17 AM on November 24, 2020: member

    Concept ACK. In the absence of test coverage, I agree this should check and fail somewhat loudly if g_scan_progress isn't 0 here.

17. MarcoFalke added the label Needs backport (0.21) on Nov 25, 2020
18. MarcoFalke added this to the milestone 0.21.0 on Nov 25, 2020
19. 
    achow101 commented at 4:36 PM on December 9, 2020: member

    Code review ACK 8c4129b4540f4f739413ed9a6fbfc78afc252f42

20. MarcoFalke removed this from the milestone 0.21.0 on Dec 15, 2020
21. MarcoFalke added this to the milestone 0.21.1 on Dec 15, 2020
22. laanwj removed this from the milestone 0.21.1 on Apr 16, 2021
23. laanwj added this to the milestone 0.21.2 on Apr 16, 2021
24. 
    laanwj commented at 5:18 AM on April 16, 2021: member

    Sorry for bumping the milestone here again. It seems almost ready for merge (could use one more ACK) but we don't want to rush a backport into 0.21.1.

25. laanwj removed this from the milestone 0.21.2 on Jun 24, 2021
26. laanwj added this to the milestone 22.0 on Jun 24, 2021
27. 
    laanwj commented at 7:31 PM on June 24, 2021: member

    This came up in the IRC meeting. Adding this to the 22.0 milestone so it can hopefully be included there.

28. MarcoFalke merged this on Jun 25, 2021
29. MarcoFalke closed this on Jun 25, 2021

    ---

30. prusnak deleted the branch on Jun 25, 2021
31. fanquake locked this on Jun 25, 2021
32. fanquake deleted a comment on Jun 25, 2021
33. 
    fanquake commented at 8:24 AM on July 29, 2021: member

    Backported in #22580.

34. fanquake removed the label Needs backport (0.21) on Jul 29, 2021

Contributors

prusnakpromagluke-jrMarcoFalkejonatackachow101laanwjfanquake

Milestone
22.0