Allow shutdown during LoadMempool, dump only when necessary #9408

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2016/12/memp_dump changing 3 files +8 −1
  1. jonasschnelli commented at 8:02 pm on December 22, 2016: contributor

    A try to address #9398. Right now, LoadMempool() has now “interruption point”. If one triggers a shutdown, It will always wait until LoadMempool() has been completed. This PR does detect a shutdown during the LoadMempool() while also taking care of not-dumping the mempool during shutdown if the user has request it before the import was completed.

    Includes also a GUI fix that ensure proper shutdowns (right now, StartShutdown() is not called when quitting the GUI).

    ping @sipa Needs testing (maybe @instagibbs?)

  2. jonasschnelli added the label Mempool on Dec 22, 2016
  3. in src/validation.h: in 66fcca9ad3 outdated 
    556@@ -557,7 +557,14 @@ static const unsigned int REJECT_CONFLICT = 0x102;
557 /** Dump the mempool to disk. */
558 void DumpMempool();
559 
560+/** Result states for loading the mempool from disk */
561+enum LoadMempoolResult {
    paveljanik commented at 10:19 pm on December 22, 2016:
    I think this is a bit overkill. bool is there already. And returning true only in case of successful mempool.dat load is enough. The only question is what to return in case of corrupted mempool.dat. Returning true instead of false makes sense here. And when true is returned, dump mempool at shutdown.
    jonasschnelli commented at 7:50 am on December 23, 2016:

    IMO a simple true|false would not work. You need to distinct between…

    • not yet loaded = don’t dump during shutdown
    • user_aborted (shutdown) = don’t dump during shutdown
    • failed = dump during shutdown
    • successful = dump during shutdown

    Returning false in case of a user-triggered shutdown could work, but then you would need to return true in case of an input error (somehow not good).

    luke-jr commented at 11:28 am on December 23, 2016:
    If user aborts at T=5, and doesn’t shut down the node, when it does finally shutdown at T=5000, you would want to dump it. This isn’t possible, but the enum name doesn’t make that clear. So a simply two-value enum with DoNotSave vs PleaseSave might be best?
  4. paveljanik commented at 11:32 am on December 23, 2016: contributor
    @luke-jr Two value enum aka bool? ;-)
  5. MarcoFalke commented at 11:34 am on December 23, 2016: member
    I might be missing something here but imo it is intentional to always dump the full mempool. In case the user feels it is taking too long (to load the mempool) on their hardware, they can switch to a smaller mempool with -maxmempool=?
  6. paveljanik commented at 11:40 am on December 23, 2016: contributor
    @MarcoFalke Imagine mempool.dat containing 300M of txs. User quits in the middle of loading it from the file. The mempool now contains ~150M of txs from the .dat file and some more txs from the network… What to do now?
  7. luke-jr commented at 12:29 pm on December 23, 2016: member
    @paveljanik Using an enum would give it clear values which cannot be accidentally cast to/from a number.
  8. jonasschnelli commented at 12:57 pm on December 23, 2016: contributor

    From a software design perspective, LoadMempool has three major states… a) successfully loaded mempool b) aborted, only partially loaded c) error, something went wrong

    The decision if we should dump during shutdown should probably not be part of LoadMempool(). Only a) and c) should result in a dump during shutdown, otherwise, we overwrite a correct dump with a partially loaded mempool. But, if the mempool could not be loaded because of data-corruption (c), we want that to dump again.

  9. sipa commented at 1:01 pm on December 23, 2016: member
    If you only have a partially loaded mempool file, but your node has been running for a week after that, we should probably dump again…
  10. jonasschnelli commented at 1:02 pm on December 23, 2016: contributor

    If you only have a partially loaded mempool file, but your node has been running for a week after that, we should probably dump again…

    Yes. Partially loaded mempool is only possible with a user-triggered shutdown during LoadMempool(). This is why I named the stated LoadMempoolResultUserAborted.

  11. paveljanik commented at 1:04 pm on December 23, 2016: contributor
    @jonasschnelli No, when the file is corrupted in the middle, you end up with partially loaded mempool.
  12. Allow shutdown during LoadMempool, dump only when necessary 9479f8dfcf
  13. jonasschnelli force-pushed on Dec 23, 2016
  14. jonasschnelli commented at 1:21 pm on December 23, 2016: contributor
    @paveljanik: Yes. Your right, I somehow though the mempool gets clear if a corruption was detected. Simplified after recommendation from @sipa. Reverted from enum to bool.
  15. in src/init.cpp: in ef479e19e9 outdated 
    129@@ -130,6 +130,7 @@ static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat";
130 //
131 
132 std::atomic<bool> fRequestShutdown(false);
133+std::atomic<bool> fDumpMempoolLater(false);
    instagibbs commented at 1:50 pm on December 23, 2016:
    nit: fDumpMempoolOnShutdown seems more self-explanatory and precise.
  16. instagibbs commented at 1:50 pm on December 23, 2016: member
    Concept ACK
  17. rebroad commented at 2:39 am on December 28, 2016: contributor
    testing this. seems to be working well so far.
  18. luke-jr commented at 7:28 pm on January 4, 2017: member
    utACK
  19. sipa added this to the milestone 0.14.0 on Jan 4, 2017
  20. sipa commented at 8:10 pm on January 4, 2017: member
    utACK ef479e19e9e9279c8e22cff50227ea0722f59df3
  21. [Qt] Do proper shutdown 325e400f9b
  22. in src/qt/bitcoingui.cpp: in ef479e19e9 outdated 
    1104@@ -1105,6 +1105,15 @@ void BitcoinGUI::detectShutdown()
1105     }
1106 }
1107 
1108+void BitcoinGUI::shouldQuit()
    laanwj commented at 10:08 am on January 5, 2017:

    Please don’t make the qt shutdown sequence even more complicated :(

    Wouldn’t a better idea be to add StartShutdown here to app.requestShutdown, called from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoin.cpp#L691

    jonasschnelli commented at 11:41 am on January 5, 2017:
    Argh. Yes. This is much simpler. Force push changed. Request retests.
  23. jonasschnelli force-pushed on Jan 5, 2017
  24. TheBlueMatt commented at 4:33 pm on January 6, 2017: member
    utACK 325e400f9ba89c6f5501c6cedd4dee979a3912b2 I’d call this a pretty bad regression if we cant get this for 0.14.
  25. MarcoFalke commented at 4:59 pm on January 6, 2017: member
    utACK 325e400
  26. sipa merged this on Jan 6, 2017
  27. sipa closed this on Jan 6, 2017
  28. sipa referenced this in commit 46b249e578 on Jan 6, 2017
  29. codablock referenced this in commit 17e51db71f on Jan 18, 2018
  30. andvgal referenced this in commit 31ad4e4bff on Jan 6, 2019
  31. CryptoCentric referenced this in commit a43037cef5 on Feb 26, 2019
  32. furszy referenced this in commit 1f010c0969 on Jan 23, 2021
  33. MarcoFalke locked this on Sep 8, 2021


jonasschnelli paveljanik luke-jr MarcoFalke sipa instagibbs rebroad laanwj TheBlueMatt

Labels
Mempool

Milestone
0.14.0

github-metadata-mirror

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-10-05 04:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me