I appreciate that you're trying to improve the naming, but be careful that this does change behaviour in the case of bitcoind.

Previously, QueueShutdown (in non-ui context) started Shutdown in a new thread, to make sure that execution returns immediately. This means that an RPC command such as "quit" got the chance to return a reply. It now calls Shutdown directly, which shuts down bitcoin in the current thread and then exit()s.

@laanwj Updated to reflect your suggestions. Can you explain, why noui.cpp is not added in bitcoin-qt.pro? I found it quite strange to not be able to edit that file directly somehow.

Looks good now.

Well, it used to be that noui.cpp conflicted with some functions in the UI. If this is no longer the case due to using boost::signals now, it could be added in bitcoin-qt.pro too... (or otherwise you could add it under "other" files)

Fine and at least on my local build it compiles just fine with noui.cpp added to the .pro. Could you verify, if this would break the official Gitian build process before I update the pull?

I'm unable to test gitian right now.

Maybe it's better to leave that for another (more trivial) pull. Focus this one on shutdown functionality, this is quite a big fish to fry, and needs heavy testing.

Okay, so let's see what other devs think.

BTW, does this solve the original issue with exit() in Shutdown? I don't see it in the diff.

If the one non-compiling change is reverted, ACK

I'm asking myself, if the change in net.cpp is correct, but as net.cpp is used by the Qt version and bitcoind it should be a StartShutdown() call.

@TheBlueMatt I reverted the changes that don't compile, they now use Shutdown(NULL); again, which should be fine for a non Qt version.

@laanwj The exit() call in Shutdown() is not an issue anymore for the Qt version, as Shutdown() was moved down (see: https://github.com/bitcoin/bitcoin/pull/1439/files#L6R304) to ensure e.g. BitcoinGUI destructor get's called and objects are not just "killed".

ACK

@TheBlueMatt One last thing that bothers me, should the 2 Shutdown(NULL); calls in init.cpp be converted to StartShutdown();, which could be considered the default function to call, when a shutdown is needed. It would end up with Shutdown() via a thread, but fits better to the rest of the shutdown code and usage.

No, in those two cases, shutdown needs to happen immediately, so launching a shutdown thread instead could cause errors.

Understood, thanks :).

@Diapolo ok, that's also a possibility, as it appears that nothing else can "fall through" to there

It does create another issue, though: the icon disappears before the shutdown starts, instead of after it ends. That's one of the things we were trying to prevent. It's useful to keep the icon until the program actually exits, to tell the user that something is running.

@laanwj Seems rather hard to ensure a clean shutdown and wanting to keep the tray-icon till the very end. Leaving the Qt Shutdown(NULL) were it was before leads to killing Qt objects. this looks much more sane now. So any further idea (perhaps in another pull if this requires additional re-work on the GUI)?

Perhaps move the Shutdown() back to where it was and re-add the #ifdef QT_GUI around the exit() in Shutdown(). This would make Bitcoin-Qt exit via the last return 0 in bitcoin.cpp.

Yeah - IMO, the idea to not call exit() at the end of the Shutdown() function in the case of the UI was good. This allows it to wind things down afterward instead of before.
(which will eventually also allow it to show some "Warning - do not shutdown computer while Bitcoin is shutting down" warning).

Edit: or even better: make it explicit and save an #ifdef. Create a ShutdownAndExit() function that shuts down and exits (calls Shutdown then exit), and a normal Shutdown() that just shuts down, then use each one at the appropriate place.

@laanwj I see 2 problems with your last suggestion.

1. Shutdown() calls CreateThread(ExitTimeout, NULL); which seems to be a 5 second timer, before the process get's killed anyway, so the Shutdown() function still somehow exits / kills the client.
2. I think an #ifdef is easier / cleaner than another function in this case.

Updated to ensure tray-icon is kept until Bitcoin-Qt exits, no further changes for no UI client.

Well, it ended up with more ifdefs than I thought it would need, but it still looks better than it was imho, ACK.

ACK

Note for whoever merges, close #1182, as this fixes it.

I think we should aim to merge this asap, but as this affects critical functionality Gavin or Sipa should probably take a look at it for a sanity check.

ACK on the changes to core; The exit(0) in Shutdown inside ifndef qt is a bit ugly though, imho.

Well, IMO the problem is that "shutdown bitcoin" and "exit program" are essentially different actions.

That's why I proposed adding a ShutdownAndExit function, and using that where appropriate: in StartShutdown [ifndef QT_GUI] and AppInit. The name would also clearly signal to the programmer that the function never returns. The normal Shutdown function would shut down bitcoin and return.

Diapolo thought it'd be less clear, but I think it'd be better to be explicit.