Drop upgrade-cancel callback registration for a generic "cancelable"

TheBlueMatt commented at 9:11 pm on July 7, 2017: member

Instead of passing a StartShutdown reference all the way up from txdb, give ShowProgress a “cancelable” boolean, as StartShutdown is pretty much always what you’ll want to use to cancel. Use the same boolean to allow cancel during initial block verification.

in src/qt/splashscreen.cpp:184 in 26c368a57f outdated

188-        Q_ARG(std::function<void(void)>, action));
189+    InitMessage(splash, title + strprintf("%d", nProgress) + "%" +
190+            (cancel_possible ? (std::string("\n") + _("(press q to shutdown and continue later)"))
191+                                : std::string("")));
192+
193+    if (cancel_possible) {

ryanofsky commented at 9:26 pm on July 7, 2017:

Don’t you need to invoke setBreakAction(nullptr) here when cancel_possible is false?

Maybe better would be to get rid of this std::function madness and replace the SplashScreen::breakAction pointer with a simpler boolean std::atomic<bool> m_cancel_possible which you can just assign directly.

TheBlueMatt commented at 9:33 pm on July 7, 2017:

Nice.

TheBlueMatt force-pushed on Jul 7, 2017

fanquake added the label GUI on Jul 7, 2017

jonasschnelli commented at 6:52 pm on July 9, 2017: contributor

Makes sense. I guess I over-engineered the flexible callback (I thought we should have a way to not only shutdown and have the future option to “continue” but this really makes little sense).

utACK 1d62f6eb35f248ff44aeac39673014bb1c9cbe71

in src/qt/splashscreen.h:39 in 1d62f6eb35 outdated

35@@ -36,8 +36,8 @@ public Q_SLOTS:
36     /** Show message and progress */
37     void showMessage(const QString &message, int alignment, const QColor &color);
38 
39-    /** Sets the break action */
40-    void setBreakAction(const std::function<void(void)> &action);
41+    /** Sets if cancel is currently possible */

ryanofsky commented at 2:23 pm on July 10, 2017:

I’d consider replacing “cancel possible” with “shutdown possible” everywhere in this PR. The function this is controlling is called StartShutdown. The display text is press q to shutdown. Why should this thing be named differently?

TheBlueMatt commented at 2:38 pm on July 10, 2017:

“shutdown” is always possible (just may be slow), what you care about is cancelling the currnent “in progress” thing (and shutting down). I dont really care, but also dont think its particularly unclear?

ryanofsky commented at 6:02 pm on July 10, 2017:

I dont really care, but also dont think its particularly unclear?

It’s unclear to me what the flag is supposed to indicate or why it wouldn’t always be true if shutdown is always possible. Maybe it should be called fast_shutdown_possible if that’s what it actually means. Acked this because the PR is definitely a simplification, but I don’t think the code or the reason for preventing the quit option from working after the upgrade is clear. I think better naming could help.

TheBlueMatt commented at 1:27 am on July 11, 2017:

Added some additional comments. I’m not really sure if thats a better name, though I agree cancel_possible is a bit strange.

ryanofsky commented at 2:26 pm on July 10, 2017: member

utACK 1d62f6eb35f248ff44aeac39673014bb1c9cbe71

sipa commented at 7:43 am on July 11, 2017: member

This begs the question whether we shouldn’t just have ‘q for shutdown’ always available? Maybe in some cases there is some initializationstep-specific cleanup, but there shouldn’t be much as we’re always allowed to crash anyway.

TheBlueMatt commented at 3:01 pm on July 11, 2017: member

@sipa yea, I thought about that. We should maybe move the boolean to just change the message to “will complete later if you shutdown now”. I still think this is a nice cleanup, and given we’re already in freeze for 0.15 it’d be too late to make that change for 0.15. If this sits for the next month without any movement I’ll make that change for 16.

ryanofsky commented at 3:43 pm on July 11, 2017: member

utACK d1db1f1112328bdee02d6f047197b353ee4916f1

sipa commented at 8:07 pm on July 15, 2017: member

Tested. Segfaults when cancelling during upgrade:

 0Thread 4 "bitcoin-shutoff" received signal SIGSEGV, Segmentation fault.
 1[Switching to Thread 0x7fffe117d700 (LWP 6034)]
 2CCoinsViewCache::DynamicMemoryUsage (this=0x0) at coins.cpp:40
 340	    return memusage::DynamicUsage(cacheCoins) + cachedCoinsUsage;
 4(gdb) bt
 5[#0](/bitcoin-bitcoin/0/)  CCoinsViewCache::DynamicMemoryUsage (this=0x0) at coins.cpp:40
 6[#1](/bitcoin-bitcoin/1/)  0x0000555555850c4e in FlushStateToDisk (chainparams=..., state=..., mode=mode@entry=FLUSH_STATE_ALWAYS, nManualPruneHeight=nManualPruneHeight@entry=0) at validation.cpp:1899
 7[#2](/bitcoin-bitcoin/2/)  0x00005555558521bc in FlushStateToDisk () at validation.cpp:1969
 8[#3](/bitcoin-bitcoin/3/)  0x00005555556f6bf5 in Shutdown () at init.cpp:219
 9[#4](/bitcoin-bitcoin/4/)  0x00005555555d0ed0 in BitcoinCore::shutdown (this=0x55555698e330) at qt/bitcoin.cpp:309
10[#5](/bitcoin-bitcoin/5/)  0x00007ffff5962359 in QObject::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
11[#6](/bitcoin-bitcoin/6/)  0x00007ffff689835c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
12[#7](/bitcoin-bitcoin/7/)  0x00007ffff689fb11 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
13[#8](/bitcoin-bitcoin/8/)  0x00007ffff59358a0 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
14[#9](/bitcoin-bitcoin/9/)  0x00007ffff593802d in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
15[#10](/bitcoin-bitcoin/10/) 0x00007ffff5989b03 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
16[#11](/bitcoin-bitcoin/11/) 0x00007ffff1644377 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
17[#12](/bitcoin-bitcoin/12/) 0x00007ffff16445e0 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
18[#13](/bitcoin-bitcoin/13/) 0x00007ffff164468c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
19[#14](/bitcoin-bitcoin/14/) 0x00007ffff5989f0f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
20[#15](/bitcoin-bitcoin/15/) 0x00007ffff593388a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
21[#16](/bitcoin-bitcoin/16/) 0x00007ffff5760fe3 in QThread::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
22[#17](/bitcoin-bitcoin/17/) 0x00007ffff5765c98 in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
23[#18](/bitcoin-bitcoin/18/) 0x00007ffff504e6da in start_thread (arg=0x7fffe117d700) at pthread_create.c:456
24[#19](/bitcoin-bitcoin/19/) 0x00007ffff387ed7f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105

TheBlueMatt commented at 11:56 pm on July 15, 2017: member

@sipa hmm, I could be wrong but that strikes me as an issue that would also be present prior to the changes in this PR.

TheBlueMatt commented at 0:01 am on July 16, 2017: member

@sipa yes, that appears to be another bug introduced in #10179 after d6af06d68aae985436cbc942f0d11078041d121b changed some semantics, I’ll fix it in #10758 with my other pile of fixes that are gonna need to land for 15 anyway.

jj1010 commented at 4:42 am on July 16, 2017: none

Como faço para criar minha própria moeda virtual

Em sábado, 15 de julho de 2017, Matt Corallo notifications@github.com escreveu:

@sipa https://github.com/sipa yes, that appears to be another bug introduced in d6af06d https://github.com/bitcoin/bitcoin/commit/d6af06d68aae985436cbc942f0d11078041d121b, I’ll fix it in #10758 https://github.com/bitcoin/bitcoin/pull/10758 with my other pile of fixes that are gonna need to land for 15 anyway.

— 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/10770#issuecomment-315571520, or mute the thread https://github.com/notifications/unsubscribe-auth/AcTu4glrwOGKax3kzh1ChzF8nR621hhHks5sOVLmgaJpZM4ORcVD .

in src/qt/splashscreen.cpp:180 in d1db1f1112 outdated

180-void SplashScreen::setBreakAction(const std::function<void(void)> &action)
181+static void ShowProgress(SplashScreen *splash, const std::string &title, int nProgress, bool cancel_possible)
182 {
183-    breakAction = action;
184-}
185+    InitMessage(splash, title + strprintf("%d", nProgress) + "%" +

sipa commented at 7:27 pm on July 16, 2017:

Please put a space or a newline between the title and the progress.

TheBlueMatt commented at 11:21 pm on July 16, 2017:

OK, cleaned it up.

TheBlueMatt force-pushed on Jul 16, 2017

ryanofsky commented at 4:07 pm on July 25, 2017: member

utACK 59147fa4d616e8f4ca05365957444cc74a30af51. Only change since last review is rearranged progress string.

achow101 commented at 11:47 pm on August 7, 2017: member

Having this would make fixing #10992 much easier.

jonasschnelli commented at 6:46 pm on August 15, 2017: contributor

@TheBlueMatt: can you rebase?

TheBlueMatt force-pushed on Aug 16, 2017

TheBlueMatt commented at 9:38 pm on August 16, 2017: member

“Rebased”, but also just dropped the “cancelable” idea (as @sipa originally suggested) and always allow shutdown, using the passd-up boolean just to change the message to inform the user the current action will resume on restart.

sipa commented at 9:54 pm on August 16, 2017: member

@TheBlueMatt There may be a use for keeping the cancellation boolean, so the interface can be reused to make the “rescanning” dialog in the GUI interruptible (cc @achow101).

in src/qt/splashscreen.cpp:175 in e7797eb770 outdated

169@@ -170,27 +170,25 @@ static void InitMessage(SplashScreen *splash, const std::string &message)
170         Q_ARG(QColor, QColor(55,55,55)));
171 }
172 
173-static void ShowProgress(SplashScreen *splash, const std::string &title, int nProgress)
174+void SplashScreen::setResumePossible(bool resume_possible)
175 {
176-    InitMessage(splash, title + strprintf("%d", nProgress) + "%");
177+    m_resume_possible = resume_possible;

ryanofsky commented at 10:05 pm on August 16, 2017:

This seems to be set but not actually used anywhere?

TheBlueMatt commented at 10:11 pm on August 16, 2017: member

@sipa that is going to need a very different interface anyway, as StartShutdown() is not appropriate there (better to separate the interface between during-init progress and other-things progress IMO).

jonasschnelli commented at 10:16 am on August 18, 2017: contributor

@achow101: for the rescan progress work, would having this PR be better or not having it?

achow101 commented at 4:22 pm on August 18, 2017: member

For rescan progress we needed the cancellation boolean, But since that has been dropped, it doesn’t matter.

Drop upgrade-cancel callback registration for a generic "resumeable"

Instead of passing a StartShutdown reference all the way up from
txdb, give ShowProgress a "resumeable" boolean, which is used to
inform the user if the action will be resumed, but cancel is always
allowed by just calling StartShutdown().

ee4d1493e2

TheBlueMatt commented at 0:04 am on August 21, 2017: member

@achow101 well the cancellation boolean wouldn’t have really helped, because the cancellation boolean in the previous version of this PR indicated to the GUI that it can call StartShutdown() to cancel, which is most definitely not what you want to do when its a rescan we’re talking about. @ryanofsky thanks, fixed.

TheBlueMatt force-pushed on Aug 21, 2017

ryanofsky commented at 3:17 pm on August 31, 2017: member

utACK ee4d1493e2a871b26201580c5a990a1df7a5e3f7. Changes since previous ACK are getting rid of cancel_possible bool to make ‘q’ shortcut to work unconditionally, and adding resume_possible bool to give an indication of when efficient resumes are possible. @achow101 implemented rescan cancellation in #11200 which is compatible with this PR (though doesn’t merge cleanly).

jonasschnelli commented at 4:20 pm on September 7, 2017: contributor

Tested ACK ee4d1493e2a871b26201580c5a990a1df7a5e3f7

jonasschnelli merged this on Sep 7, 2017

jonasschnelli closed this on Sep 7, 2017

jonasschnelli referenced this in commit ea729d55b4 on Sep 7, 2017

PastaPastaPasta referenced this in commit 469a3fcc52 on Sep 23, 2019

PastaPastaPasta referenced this in commit fb24a106b4 on Sep 24, 2019

PastaPastaPasta referenced this in commit 5ac6b519b1 on Nov 19, 2019

PastaPastaPasta referenced this in commit efa580e494 on Nov 21, 2019

PastaPastaPasta referenced this in commit 156614e202 on Dec 9, 2019

PastaPastaPasta referenced this in commit 2e6fdf7c9d on Jan 1, 2020

PastaPastaPasta referenced this in commit a67a907343 on Jan 2, 2020

PastaPastaPasta referenced this in commit 5a6fe2bb69 on Jan 2, 2020

PastaPastaPasta referenced this in commit df4b32fdaa on Jan 2, 2020

PastaPastaPasta referenced this in commit 2287b353f7 on Jan 2, 2020

PastaPastaPasta referenced this in commit ffe2790ac2 on Jan 2, 2020

PastaPastaPasta referenced this in commit 15b009b4d1 on Jan 2, 2020

PastaPastaPasta referenced this in commit 6ac77d4b82 on Jan 2, 2020

PastaPastaPasta referenced this in commit 2aa7ca5723 on Jan 3, 2020

PastaPastaPasta referenced this in commit 8ffd932edd on Jan 10, 2020

ckti referenced this in commit 8016d30806 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

Drop upgrade-cancel callback registration for a generic “cancelable” #10770