[RFC] I Have a Hammer! (Replace parts of ui_interface with validationinterface)

TheBlueMatt commented at 8:28 pm on December 8, 2017: member

Look at my pretty harrmer, watch as I make everything into a nail!

This de-duplicates the NotifyBlockTip/BlockTipChanged callbacks by removing the NotifyBlockTip callback from ui_interface, cleaning up a few things along the way. It does, however, add a good bit of overhead where there was previously none - instead of a simple boost::signal things are now being called on the scheduler background thread. Still, I think its worth it because a) background-threading this stuff makes us less vulnerable to latency spikes in different subsystems because some other subsystem takes forever (at least once validationitnerface is parallel across different clients) and b) avoids lockorder issues creeping in due to cs_main complexity.

TheBlueMatt force-pushed on Dec 8, 2017

in src/rpc/server.h:32 in 93264c3a86 outdated

26@@ -25,6 +27,45 @@ namespace RPCServer
27 {
28     void OnStarted(std::function<void ()> slot);
29     void OnStopped(std::function<void ()> slot);
30+
31+    struct InterruptedListenerInternals;
32+    /** A scoped listener for when RPC is itnerrupted (ie IsRPCRunning moves

jamesob commented at 11:06 pm on December 8, 2017:

itnerrupted

TheBlueMatt force-pushed on Dec 9, 2017

fanquake added the label Refactoring on Dec 11, 2017

in src/rpc/mining.cpp:490 in 9ebcabf49a outdated

485@@ -473,15 +486,21 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
486             nTransactionsUpdatedLastLP = nTransactionsUpdatedLast;
487         }
488 
489+        RPCServer::BlockChangeBlocker block_waiter;
490+        {
491+            WaitableLock lock(block_waiter.m_cs);
492+            block_waiter.m_last_block_hash = hashWatchedChain;

jimpo commented at 7:56 pm on December 12, 2017:

Why is this necessary if m_last_block_hash in block_waiter is initialized to the chain tip? Seems like it would block longer than necessary below if lpval has an old block hash.

TheBlueMatt commented at 8:38 pm on December 12, 2017:

hashWatchedChain could be set a few lines up by lpstr instead of by chainActive.Tip().

jimpo commented at 8:54 pm on December 12, 2017:

Yes, I see that, but it looks like the old behavior would have been to return immediately if the longpollid was out of date.

TheBlueMatt commented at 9:13 pm on December 12, 2017:

Yea, but without setting m_last_block_hash to hashWatchedChain we would instead still wait until chainActive.Tip() advances?

jimpo commented at 9:20 pm on December 12, 2017:

It would never even enter the while (block_waiter.m_last_block_hash == hashWatchedChain && ...) loop, right?

TheBlueMatt commented at 9:39 pm on December 12, 2017:

Ohoh, yea, I think I had changed the loop condition since a previous iteration. Fixed.

in src/init.cpp:1636 in adb7ce305f outdated

1634@@ -1633,7 +1635,7 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1635         while (!fHaveGenesis) {
1636             condvar_GenesisWait.wait(lock);

jimpo commented at 8:20 pm on December 12, 2017:

Maybe worth changing to condvar_GenesisWait.wait(lock, []{ return fHaveGenesis; }); and dropping the loop.

TheBlueMatt commented at 9:13 pm on December 12, 2017:

Done.

in src/init.cpp:540 in adb7ce305f outdated

535@@ -536,22 +536,24 @@ class BlockNotifyCaller : public CValidationInterface {
536         }
537     }
538 } g_blocknotify_caller;
539-} // anonymous namespace
540 
541 static bool fHaveGenesis = false;

jimpo commented at 8:22 pm on December 12, 2017:

I’d rather see more of these globals encapsulated in the GenesisWaiter. AppInitMain could be simplified if GenesisWaiter just had a method like BlockUntilHaveGenesis or something.

TheBlueMatt commented at 9:13 pm on December 12, 2017:

Done.

jimpo commented at 8:26 pm on December 12, 2017: contributor

Generally like the changes.

TheBlueMatt force-pushed on Dec 12, 2017

Clarify validationinterface notification ordering cd0f402a8c

[validationinterface] Also call UpdatedBlockTip on invalidateblock dce98c5e7c

Add interface to listen for RPC interrupted events temporarily 6be15ece76

Add RPC utility to block until tip changes or RPC is interrupted c93c7d46f2

Use BlockChangeBlocker instead of hacky workaround in chain RPCs 12f067d176

Replace BlockNotifyCallback with a validationinterface client b4928b2c78

Use a validationinterface to wait for genesis block connection. 31e890c72f

Remove ui_interface.BlockTipChanged f6837cdb86

Replace csBestBlock/cvBlockChanged with validationinterface

Also push down cs_main locking in getblocktemplate a bit.

17e931d2ff

in src/rpc/blockchain.cpp:178 in 5f99bf4e73 outdated

181-        latestblock.hash = pindex->GetBlockHash();
182-        latestblock.height = pindex->nHeight;
183-    }
184-    cond_blockchange.notify_all();
185+template<typename T>
186+static UniValue AwaitBlockChangeCondition(int timeout, const T& term_condition) {

jtimon commented at 11:20 pm on December 14, 2017:

style nit: “break before braces on function, namespace and class definitions”

https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L15 http://clang.llvm.org/docs/ClangFormatStyleOptions.html

TheBlueMatt commented at 11:25 pm on December 15, 2017:

Fixed (for at least not-two-line-functions).

TheBlueMatt force-pushed on Dec 15, 2017

laanwj commented at 11:26 am on December 21, 2017: member

It does, however, add a good bit of overhead where there was previously none

Adding overhead sounds bad. Can you quantify this somehow?

Note that none of the GUI notifications take significant time. They already just queue a signal to Qt, to be handled later. Adding another layer sounds somewhat bad.

in src/validationinterface.h:54 in cd0f402a8c outdated

50+     * Notifies listeners of updated block chain tip.
51+     *
52+     * Is called after a series of BlockConnected/BlockDisconnected events once
53+     * the chain has made forward progress and is now at the best-known-tip.
54+     *
55+     * If a block is found to be invalid, this event may trigger without

ryanofsky commented at 10:25 pm on December 21, 2017:

In commit “Clarify validationinterface notification ordering”

All of the other comments in this commit are very good, but for some reason I’m finding this sentence really confusing. I think this is saying that UpdateBlockTip might be called repeatedly with the same pindexNew value in cases where invalid blocks are being received. But the current wording seems to imply that this might actually pass an invalid block pointer, which I don’t think is the case (unless invalidateblock is called?). And “without forward progress” seems to imply that pindexNew might go backwards? I think it’d be helpful to replace this sentence with a list of some specific invariants the notification (1) does guarantee (2) doesn’t guarantee, or (3) aspires to guarantee (in case of the TODO) instead of being so vague.

in src/validation.cpp:2720 in dce98c5e7c outdated

2716@@ -2717,6 +2717,7 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
2717 
2718     InvalidChainFound(pindex);
2719     uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev);
2720+    GetMainSignals().UpdatedBlockTip(chainActive.Tip(), chainActive.Tip(), IsInitialBlockDownload());

ryanofsky commented at 10:43 pm on December 21, 2017:

In commit “Also call UpdatedBlockTip on invalidateblock”

This seems like a good change, but was it just a bug that that this call wasn’t being made before? Or is this preparation for some new feature? Should this have a test? Should it have a mention in release notes if it affects RPC or ZMQ? It’s good when commits like this have some test or documentation update, or a sentence in the commit message that sheds light on what the motivation for the change is.

in src/init.cpp:724 in 17e931d2ff

714@@ -721,7 +715,6 @@ bool InitSanityCheck(void)
715 
716 bool AppInitServers(boost::thread_group& threadGroup)
717 {
718-    RPCServer::OnStopped(&OnRPCStopped);

ryanofsky commented at 10:56 pm on December 21, 2017:

In commit “Replace csBestBlock/cvBlockChanged with validationinterface”

Maybe remove the OnStopped function and stopped signal as they no longer have any listeners.

in src/rpc/mining.cpp:495 in 17e931d2ff

492             checktxtime = std::chrono::steady_clock::now() + std::chrono::minutes(1);
493 
494-            WaitableLock lock(csBestBlock);
495-            while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
496+            WaitableLock lock(block_waiter.m_cs);
497+            while (block_waiter.m_last_block_hash == hashWatchedChain && IsRPCRunning())

ryanofsky commented at 11:08 pm on December 21, 2017:

In commit “Replace csBestBlock/cvBlockChanged with validationinterface "

IsRPCRunning does not appear to be thread safe. Maybe fRPCRunning be a changed to an atomic bool.

in src/rpc/mining.cpp:450 in 17e931d2ff

446@@ -447,6 +447,19 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
447     if (IsInitialBlockDownload())
448         throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Bitcoin is downloading blocks...");
449 
450+    if (!lpval.isNull()) {

ryanofsky commented at 11:17 pm on December 21, 2017:

In commit “Replace csBestBlock/cvBlockChanged with validationinterface”

Is the ! here a mistake? I could see how the RPC call might return too early if lpval is null, but if it’s non-null and actually set to a hash, it seems like this would only lead to unnecessary delays.

in src/rpc/server.cpp:74 in c93c7d46f2 outdated

69+            m_cv.notify_all();
70+        })
71+{
72+    {
73+        LOCK(cs_main);
74+        m_last_block_hash = chainActive.Tip()->GetBlockHash();

ryanofsky commented at 11:37 pm on December 21, 2017:

In commit “Add RPC utility to block until tip changes or RPC is interrupted”

Do you need to acquire m_cs while updating m_last_block variables here? It doesn’t seem like there is a guard against these writes being delayed and UpdatedBlockTip trying to update the values at the same time. Or if this is safe, should add a comment here saying why m_cs isn’t needed.

ryanofsky commented at 11:45 pm on December 21, 2017: member

Started review (about halfway through).

ryanofsky commented at 5:11 pm on April 30, 2018: member

Note: The first commit in this PR (“Clarify validationinterface notification ordering”) also appears in #11775 and #12979, but I think the other the commits are unique.

jnewbery commented at 9:21 pm on May 17, 2018: member

This needs rebase, but probably shouldn’t be updated until #12979 gets merged (with which this PR shares a commit)

MarcoFalke added the label Needs rebase on Jun 6, 2018

TheBlueMatt closed this on Jun 14, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

[RFC] I Have a Hammer! (Replace parts of ui_interface with validationinterface) #11856