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

TheBlueMatt · 2017-12-08T20:28:03Z

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.

jamesob · 2017-12-08T23:06:05Z

src/rpc/server.h

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


itnerrupted

jimpo

Generally like the changes.

jimpo · 2017-12-12T19:56:01Z

src/rpc/mining.cpp

+        RPCServer::BlockChangeBlocker block_waiter;
+        {
+            WaitableLock lock(block_waiter.m_cs);
+            block_waiter.m_last_block_hash = hashWatchedChain;


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.

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

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

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

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

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

jimpo · 2017-12-12T20:20:52Z

src/init.cpp

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


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

jimpo · 2017-12-12T20:22:52Z

src/init.cpp

@@ -536,22 +536,24 @@ class BlockNotifyCaller : public CValidationInterface {
        }
    }
 } g_blocknotify_caller;
-} // anonymous namespace

 static bool fHaveGenesis = false;


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.

jtimon · 2017-12-14T23:20:10Z

src/rpc/blockchain.cpp

-    }
-    cond_blockchange.notify_all();
+template<typename T>
+static UniValue AwaitBlockChangeCondition(int timeout, const T& term_condition) {


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

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

Also push down cs_main locking in getblocktemplate a bit.

laanwj · 2017-12-21T11:26:28Z

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.

ryanofsky

Started review (about halfway through).

ryanofsky · 2017-12-21T22:25:36Z

src/validationinterface.h

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


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.

ryanofsky · 2017-12-21T22:43:38Z

src/validation.cpp

@@ -2717,6 +2717,7 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c

    InvalidChainFound(pindex);
    uiInterface.NotifyBlockTip(IsInitialBlockDownload(), pindex->pprev);
+    GetMainSignals().UpdatedBlockTip(chainActive.Tip(), chainActive.Tip(), IsInitialBlockDownload());


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.

ryanofsky · 2017-12-21T22:56:39Z

src/init.cpp

@@ -721,7 +715,6 @@ bool InitSanityCheck(void)

 bool AppInitServers(boost::thread_group& threadGroup)
 {
-    RPCServer::OnStopped(&OnRPCStopped);


In commit "Replace csBestBlock/cvBlockChanged with validationinterface"

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

ryanofsky · 2017-12-21T23:08:31Z

src/rpc/mining.cpp

-            WaitableLock lock(csBestBlock);
-            while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
+            WaitableLock lock(block_waiter.m_cs);
+            while (block_waiter.m_last_block_hash == hashWatchedChain && IsRPCRunning())


In commit "Replace csBestBlock/cvBlockChanged with validationinterface "

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

ryanofsky · 2017-12-21T23:17:51Z

src/rpc/mining.cpp

@@ -447,6 +447,19 @@ UniValue getblocktemplate(const JSONRPCRequest& request)
    if (IsInitialBlockDownload())
        throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Bitcoin is downloading blocks...");

+    if (!lpval.isNull()) {


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.

ryanofsky · 2017-12-21T23:37:12Z

src/rpc/server.cpp

+{
+    {
+        LOCK(cs_main);
+        m_last_block_hash = chainActive.Tip()->GetBlockHash();


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 · 2018-04-30T17:11:49Z

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 · 2018-05-17T21:21:57Z

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

TheBlueMatt force-pushed the 2017-12-remove-cvblockchange branch 2 times, most recently from 4afd218 to 93264c3 Compare December 8, 2017 20:53

jamesob reviewed Dec 8, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-12-remove-cvblockchange branch 3 times, most recently from 2f0eb46 to 9ebcabf Compare December 9, 2017 19:27

fanquake added the Refactoring label Dec 11, 2017

jimpo reviewed Dec 12, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-12-remove-cvblockchange branch 2 times, most recently from e36037f to 5f99bf4 Compare December 12, 2017 21:39

jtimon reviewed Dec 14, 2017

View reviewed changes

TheBlueMatt added 9 commits December 15, 2017 18:22

Clarify validationinterface notification ordering

cd0f402

[validationinterface] Also call UpdatedBlockTip on invalidateblock

dce98c5

Add interface to listen for RPC interrupted events temporarily

6be15ec

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

c93c7d4

Use BlockChangeBlocker instead of hacky workaround in chain RPCs

12f067d

Replace BlockNotifyCallback with a validationinterface client

b4928b2

Use a validationinterface to wait for genesis block connection.

31e890c

Remove ui_interface.BlockTipChanged

f6837cd

Replace csBestBlock/cvBlockChanged with validationinterface

17e931d

Also push down cs_main locking in getblocktemplate a bit.

TheBlueMatt force-pushed the 2017-12-remove-cvblockchange branch from 5f99bf4 to 17e931d Compare December 15, 2017 23:25

ryanofsky reviewed Dec 21, 2017

View reviewed changes

ryanofsky mentioned this pull request Mar 6, 2018

Only call NotifyBlockTip when chainActive changes #12431

Merged

skeees mentioned this pull request Apr 15, 2018

Hold cs_main while calling UpdatedBlockTip() signal #12988

Merged

ryanofsky mentioned this pull request Apr 30, 2018

Split validationinterface into parallel validation/mempool interfaces #12979

Closed

maflcko added the Needs rebase label Jun 6, 2018

TheBlueMatt closed this Jun 14, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

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

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

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

Uh oh!

Conversation

TheBlueMatt commented Dec 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimpo Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 21, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Apr 30, 2018

Uh oh!

jnewbery commented May 17, 2018

Uh oh!

Uh oh!

jimpo Dec 12, 2017 •

edited

Loading