gui: Fix regression in *txoutset* in GUI console

hebasto commented at 8:00 pm on June 18, 2020: member

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.

in src/rpc/blockchain.cpp:1000 in 369f5d263e outdated

 996@@ -997,7 +997,7 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
 997     ::ChainstateActive().ForceFlushStateToDisk();
 998 
 999     CCoinsView* coins_view = WITH_LOCK(cs_main, return &ChainstateActive().CoinsDB());
1000-    if (GetUTXOStats(coins_view, stats, RpcInterruptionPoint)) {
1001+    if (GetUTXOStats(coins_view, stats, IsRPCRunning() ? RpcInterruptionPoint : [] {})) {

MarcoFalke commented at 8:04 pm on June 18, 2020:

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

hebasto commented at 8:17 pm on June 18, 2020:

Isn’t it self-documented: “interrupt RPC only if RPC is running”?

MarcoFalke commented at 8:27 pm on June 18, 2020:

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?

hebasto commented at 8:36 pm on June 18, 2020:

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.

MarcoFalke commented at 8:49 pm on June 18, 2020:

gettxoutsetinfo
stop -> Doesn’t stop because gettxoutsetinfo is not interruptible
gettxoutsetinfo finishes and server stops

hebasto commented at 7:24 am on June 19, 2020:

Updated.

MarcoFalke added this to the milestone 0.21.0 on Jun 18, 2020

MarcoFalke added the label GUI on Jun 18, 2020

MarcoFalke added the label RPC/REST/ZMQ on Jun 18, 2020

DrahtBot commented at 2:33 am on June 19, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
#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 force-pushed on Jun 19, 2020

hebasto commented at 7:24 am on June 19, 2020: member

Updated 369f5d263ee5a26b62545b604ee0b33f24eaadfa -> 434f74365ebb265bcf04bbb1c87d368b5c6553a4 (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?

MarcoFalke commented at 11:38 am on June 19, 2020: member

weak review ACK 434f74365ebb265bcf04bbb1c87d368b5c6553a4

cc @ryanofsky is the use of NodeContext here appropriate?

ryanofsky approved

ryanofsky commented at 1:14 pm on June 19, 2020: member

Code review ACK 434f74365ebb265bcf04bbb1c87d368b5c6553a4 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 434f74365ebb265bcf04bbb1c87d368b5c6553a4 is only fixing one of the four:

https://github.com/bitcoin/bitcoin/blob/5f72ddb7ee4cc177de31f43c69390ee72687222a/src/rpc/blockchain.cpp#L1000 https://github.com/bitcoin/bitcoin/blob/5f72ddb7ee4cc177de31f43c69390ee72687222a/src/rpc/blockchain.cpp#L1979 https://github.com/bitcoin/bitcoin/blob/5f72ddb7ee4cc177de31f43c69390ee72687222a/src/rpc/blockchain.cpp#L2319 https://github.com/bitcoin/bitcoin/blob/5f72ddb7ee4cc177de31f43c69390ee72687222a/src/rpc/blockchain.cpp#L2337

hebasto force-pushed on Jun 19, 2020

hebasto commented at 6:04 pm on June 19, 2020: member

Updated 434f74365ebb265bcf04bbb1c87d368b5c6553a4 -> e75b125264e8dc8115fd035b1334b023a4913a11 (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#pullrequestreview-434066791

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.

in src/rpc/blockchain.cpp:2151 in e75b125264 outdated

2146@@ -2143,7 +2147,8 @@ UniValue scantxoutset(const JSONRPCRequest& request)
2147             tip = ::ChainActive().Tip();
2148             CHECK_NONFATAL(tip);
2149         }
2150-        bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
2151+        NodeContext& node = EnsureNodeContext(request.context);
2152+        bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins, node);

MarcoFalke commented at 8:20 pm on June 19, 2020:

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

0        bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins, node.rpc_interruption_point);

hebasto commented at 11:16 am on June 20, 2020:

Updated.

hebasto force-pushed on Jun 20, 2020

hebasto commented at 11:15 am on June 20, 2020: member

Updated e75b125264e8dc8115fd035b1334b023a4913a11 -> 44ef1170cf1466bfa3ea296c78b821605caa0311 (pr19323.03 -> pr19323.04, diff):

addressed @MarcoFalke’s comment:

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

ryanofsky approved

ryanofsky commented at 4:43 pm on July 2, 2020: member

Code review ACK 44ef1170cf1466bfa3ea296c78b821605caa0311. 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#pullrequestreview-434066791. 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.

MarcoFalke commented at 5:14 pm on July 2, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3Review ACK 44ef1170cf1466bfa3ea296c78b821605caa0311 , checked that all RpcInterruptionPoint are replaced by node.rpc_interruption_point 👥
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiyEgwAzwkY2V+cNbtFvl4G/65/9I3dhHDE68NmHRgq6qQxP0/oA4MKcUrSjaoO
 8naJMn9LP4L8NMmNR3VQe9lhxu63qzhymxRYCzJJRkXHAWQ1WQMapMsR1NN1861VG
 9c3Zz0/t6okQHx1o6N5XwMx1YiecPcMEs8h61JGXLq1f17ECVRI8z5oSo4nL3j+FH
10oeS4iIszG46CSWlueqZE7Vq7h7J1aEVj3CidHIPsopYRs/WhlvkQDAIkoxqdbWIU
11xpk/wQZzUWOFL9C0dhMVybvouJW3ZPuw+AnS11nPBkagmw7DLUZ13+u4kon2qi6a
12UNJA670fqlR0mEK6scKy3/PoRJCPl2z05iyB4Kcxv7/FISPQOc9MJ1IZ/fuyikn7
139XsTITd5C+7a3KUEaROe94tEsTOJJ+ZrvAgzNQ93coy+f7LnPyMOLTGVUPagmNpY
14B8lpF7manTmkKjuzCHKUXzTqq0505eXYvGP7L+kGmiPxA69ajEbBGgdqKvPv+/NC
15ntWM9mto
16=KOfS
17-----END PGP SIGNATURE-----

Timestamp of file with hash 30e3c19b4b83028940a62fea9a5feb72bad494a01063d4dacab284af3dfd0f8a -

MarcoFalke commented at 5:15 pm on July 2, 2020: member

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

DrahtBot added the label Needs rebase on Jul 6, 2020

gui: Fix regression in GUI console

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

314b49bd50

hebasto force-pushed on Jul 8, 2020

hebasto commented at 4:20 pm on July 8, 2020: member

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

DrahtBot removed the label Needs rebase on Jul 8, 2020

ryanofsky approved

ryanofsky commented at 5:36 pm on July 10, 2020: member

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

MarcoFalke merged this on Jul 14, 2020

MarcoFalke closed this on Jul 14, 2020

sidhujag referenced this in commit d67b0940db on Jul 14, 2020

hebasto deleted the branch on Jul 14, 2020

Fabcien referenced this in commit 23bcefb204 on Dec 10, 2020

DrahtBot locked this on Feb 15, 2022

gui: Fix regression in txoutset in GUI console #19323

Conflicts