gui: Fix regression in txoutset in GUI console #19323

hebasto · 2020-06-18T20:00:23Z

The regression was introduced in #19056: if the GUI is running without -server=1, the *txoutset* call in the console returns "Shutting down".

Fix #19255.

maflcko · 2020-06-18T20:04:13Z

src/rpc/blockchain.cpp

@@ -997,7 +997,7 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
    ::ChainstateActive().ForceFlushStateToDisk();

    CCoinsView* coins_view = WITH_LOCK(cs_main, return &ChainstateActive().CoinsDB());
-    if (GetUTXOStats(coins_view, stats, RpcInterruptionPoint)) {
+    if (GetUTXOStats(coins_view, stats, IsRPCRunning() ? RpcInterruptionPoint : [] {})) {


If the rpc code needs such a workaround for the gui, I'd prefer to at least explain it with a comment

Isn't it self-documented: "interrupt RPC only if RPC is running"?

Also, it seems racy when the rpc is shut down during flush (which might take a long time)

Maybe the interruption point can be passed in through the request similar to how the connman is passed in?

Also, it seems racy when the rpc is shut down during flush (which might take a long time)

Did not get why. IsRPCRunning() is checked after synchronous ForceFlushStateToDisk() call. During GUI shutdown #17659 should ensure the correct RPCConsole behavior.

gettxoutsetinfo

stop -> Doesn't stop because gettxoutsetinfo is not interruptible

gettxoutsetinfo finishes and server stops

DrahtBot · 2020-06-19T02:33:49Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2020-06-19T07:24:10Z

Updated 369f5d2 -> 434f743 (pr19323.01 -> pr19323.02, diff):

addressed @MarcoFalke's comment:

Also, it seems racy when the rpc is shut down during flush (which might take a long time)

Maybe the interruption point can be passed in through the request similar to how the connman is passed in?

maflcko · 2020-06-19T11:38:20Z

weak review ACK 434f743

cc @ryanofsky is the use of NodeContext here appropriate?

ryanofsky

Code review ACK 434f743 with followup suggestions below.

weak review ACK 434f743

cc @ryanofsky is the use of NodeContext here appropriate?

This seems ok as a workaround but IMO better would be to eliminate g_rpc_running global. Might do that by moving g_rpc_running and g_rpcSignals into an RpcContext struct, adding a unique_ptr<RpcContext> or RpcContext* member to NodeContext, and passing RpcContext& to StartRPC/InterruptRPC/StopRPC.

A more immediate concern with this PR is it that it seems incomplete. I see 4 references to RpcInterruptionPoint that will throw in the GUI, but 434f743 is only fixing one of the four:

bitcoin/src/rpc/blockchain.cpp

Line 1000 in 5f72ddb

if (GetUTXOStats(coins_view, stats, RpcInterruptionPoint)) {

bitcoin/src/rpc/blockchain.cpp

Line 1979 in 5f72ddb

RpcInterruptionPoint();

bitcoin/src/rpc/blockchain.cpp

Line 2319 in 5f72ddb

    
           if (!GetUTXOStats(&::ChainstateActive().CoinsDB(), stats, RpcInterruptionPoint)) {

bitcoin/src/rpc/blockchain.cpp

Line 2337 in 5f72ddb

if (iter % 5000 == 0) RpcInterruptionPoint();

hebasto · 2020-06-19T18:04:50Z

Updated 434f743 -> e75b125 (pr19323.02 -> pr19323.03, diff):

addressed @ryanofsky's comment:

A more immediate concern with this PR is it that it seems incomplete. I see 4 references to RpcInterruptionPoint that will throw in the GUI, but 434f743 is only fixing one of the four

Re: #19323 (review)

This seems ok as a workaround but IMO better would be to eliminate g_rpc_running global. Might do that by moving g_rpc_running and g_rpcSignals into an RpcContext struct, adding a unique_ptr or RpcContext* member to NodeContext, and passing RpcContext& to StartRPC/InterruptRPC/StopRPC.

These changes appear non-trivial as g_rpc_running is used in IsRPCRunning(), and its usage is too broad.

src/rpc/blockchain.cpp

hebasto · 2020-06-20T11:15:04Z

Updated e75b125 -> 44ef117 (pr19323.03 -> pr19323.04, diff):

addressed @MarcoFalke's comment:

nit: why not pass in the rpc_interruption_point like below for GetUTXOStats?

ryanofsky

Code review ACK 44ef117. Only change since last review is replacing more uses of RpcInterruptionPoint that could also throw and fail unexpectedly.

I think there is room to improve this in the future. RPC globals be eliminated as described #19323 (review). Also more importantly these methods should always be interrupted when the gui is shutting down, whether or not -server=0 or -server=1 is used, not just when -server=1 is used.

maflcko · 2020-07-02T17:14:26Z

Review ACK 44ef117 , checked that all RpcInterruptionPoint are replaced by node.rpc_interruption_point 👥

Show signature and timestamp

Signature:

Timestamp of file with hash 30e3c19b4b83028940a62fea9a5feb72bad494a01063d4dacab284af3dfd0f8a -

maflcko · 2020-07-02T17:15:06Z

cc @fanquake you might be interested in this, based on your bug report #19255

This change prevents "Shutting down" message during "dumptxoutset", "gettxoutsetinfo" and "scantxoutset" calls.

hebasto · 2020-07-08T16:20:28Z

Rebased 44ef117 -> 314b49b (pr19323.04 -> pr19323.05) due to the conflict with #19328.

ryanofsky

Code review ACK 314b49b. Only change since last review is rebase

314b49b gui: Fix regression in GUI console (Hennadii Stepanov) Pull request description: The regression was introduced in bitcoin#19056: if the GUI is running without `-server=1`, the `*txoutset*` call in the console returns "Shutting down". Fix bitcoin#19255. ACKs for top commit: ryanofsky: Code review ACK 314b49b. Only change since last review is rebase Tree-SHA512: 8ff85641a5c249858fecb1ab69c7a1b2850af651ff2a94aa41ce352b5b5bc95bc45c41e1767e871b51e647612d09e4d54ede3e20c313488afef5678826c51b62

Summary: ``` Make it interruptible, so that shutdown doesn't block for up to one hour. ``` Backport of core [[bitcoin/bitcoin#19056 | PR19056]] and [[bitcoin/bitcoin#19323 | PR19323]] (bugfix). Test Plan: ninja all check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8645

maflcko reviewed Jun 18, 2020

View reviewed changes

maflcko added this to the 0.21.0 milestone Jun 18, 2020

maflcko added GUI RPC/REST/ZMQ labels Jun 18, 2020

DrahtBot mentioned this pull request Jun 19, 2020

Add hash_type MUHASH for gettxoutsetinfo #19145

Merged

hebasto force-pushed the 200618-utxo branch from 369f5d2 to 434f743 Compare June 19, 2020 07:16

ryanofsky approved these changes Jun 19, 2020

View reviewed changes

This was referenced Jun 19, 2020

multiprocess: Add basic spawn and IPC support #19160

Merged

Multiprocess bitcoin #10102

Draft

hebasto force-pushed the 200618-utxo branch from 434f743 to e75b125 Compare June 19, 2020 17:58

DrahtBot mentioned this pull request Jun 19, 2020

Add gettxoutsetinfo hash_type option #19328

Merged

maflcko reviewed Jun 19, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 200618-utxo branch from e75b125 to 44ef117 Compare June 20, 2020 11:13

ryanofsky approved these changes Jul 2, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jul 6, 2020

gui: Fix regression in GUI console

314b49b

This change prevents "Shutting down" message during "dumptxoutset", "gettxoutsetinfo" and "scantxoutset" calls.

hebasto force-pushed the 200618-utxo branch from 44ef117 to 314b49b Compare July 8, 2020 16:20

DrahtBot removed the Needs rebase label Jul 8, 2020

This was referenced Jul 8, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

ryanofsky approved these changes Jul 10, 2020

View reviewed changes

maflcko merged commit 834ac4c into bitcoin:master Jul 14, 2020

hebasto deleted the 200618-utxo branch July 14, 2020 15:34

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gui: Fix regression in txoutset in GUI console #19323

gui: Fix regression in txoutset in GUI console #19323

hebasto commented Jun 18, 2020

maflcko Jun 18, 2020

hebasto Jun 18, 2020

maflcko Jun 18, 2020

hebasto Jun 18, 2020 •

edited

Loading

maflcko Jun 18, 2020

hebasto Jun 19, 2020

DrahtBot commented Jun 19, 2020 •

edited

Loading

hebasto commented Jun 19, 2020

maflcko commented Jun 19, 2020

ryanofsky left a comment

hebasto commented Jun 19, 2020

hebasto commented Jun 20, 2020

ryanofsky left a comment

maflcko commented Jul 2, 2020

maflcko commented Jul 2, 2020

hebasto commented Jul 8, 2020

ryanofsky left a comment

gui: Fix regression in *txoutset* in GUI console #19323

gui: Fix regression in *txoutset* in GUI console #19323

Conversation

hebasto commented Jun 18, 2020

maflcko Jun 18, 2020

Choose a reason for hiding this comment

hebasto Jun 18, 2020

Choose a reason for hiding this comment

maflcko Jun 18, 2020

Choose a reason for hiding this comment

hebasto Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

maflcko Jun 18, 2020

Choose a reason for hiding this comment

hebasto Jun 19, 2020

Choose a reason for hiding this comment

DrahtBot commented Jun 19, 2020 • edited Loading

Conflicts

hebasto commented Jun 19, 2020

maflcko commented Jun 19, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

hebasto commented Jun 19, 2020

hebasto commented Jun 20, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

maflcko commented Jul 2, 2020

maflcko commented Jul 2, 2020

hebasto commented Jul 8, 2020

ryanofsky left a comment

Choose a reason for hiding this comment

gui: Fix regression in txoutset in GUI console #19323

gui: Fix regression in txoutset in GUI console #19323

hebasto Jun 18, 2020 •

edited

Loading

DrahtBot commented Jun 19, 2020 •

edited

Loading