Hold cs_main while calling UpdatedBlockTip() signal

skeees commented at 3:25 pm on April 15, 2018: contributor

Resolves #12978

skeees commented at 3:37 pm on April 15, 2018: contributor

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

ryanofsky commented at 4:31 pm on April 15, 2018: member

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

Would have to look into it more, but if the problem is NotifyBlockTip events being sent to the GUI in the wrong order, we should determine whether one of @TheBlueMatt’s PRs fixes this. Assuming one does, it’s probably sufficient for this PR to just update the NotifyBlockTip documentation to say that notifications can arrive at the same time from multiple threads and there are no ordering guarantees (or whatever the current situation is).

skeees commented at 4:39 pm on April 15, 2018: contributor

#11856 is the one that I think you are referring to which eliminates that particular call to NotifyBlockTip and replaces it with the ValidationInterface signals

TheBlueMatt commented at 4:40 pm on April 15, 2018: member

I think this PR is fine as-is, the GUI I’d be surprised if it were broken, though the mining stuff may be (but #11856 pulls it all out to using validationinterface, which should be a less-likely-to-break-things fix as it doesn’t introduce new lockorders). It’d also be nice to see the no-change-callbacks go away ala a9db3dada0119c183d16627805e90c4dbca05c6a since we’re touching this code anyway.

ryanofsky commented at 4:50 pm on April 15, 2018: member

I think this PR is fine as-is

I think it would be good for this PR to update NotifyBlockTip documentation if notifications can come out of order, regardless of whether there are currently any gui bugs.

skeees commented at 5:25 pm on April 15, 2018: contributor

@ryanofsky - i’ll update docs @TheBlueMatt - will evaluate whether feasible to cherry-pick that commit into this pr

laanwj commented at 1:34 pm on April 16, 2018: member

@ryanofsky / other qt experts - the gui signals still get called out of cs_main - do you see any potential issues there?

I expect the GUI doesn’t care - it transfers the signals to a different thread anyway.

jnewbery commented at 2:25 pm on April 16, 2018: member

Looks good to me. I don’t think https://github.com/bitcoin/bitcoin/commit/a9db3dada0119c183d16627805e90c4dbca05c6a needs to be pulled in - it’s an orthogonal change for whether to fire notifications at all. It’s useful, but no need to widen the scope of this PR.

TheBlueMatt commented at 2:29 pm on April 16, 2018: member

Heh, @jnewbery pointed out that a9db3dada0119c183d16627805e90c4dbca05c6a doesn’t actually fix it, I was looking at the wrong callback, sorry for the noise.

TheBlueMatt commented at 2:44 pm on April 16, 2018: member

As for the uiInterface callback, I think its best to move it into the cs_main as well - as @laanwj points out in the GUI its immediately pushed to a queue for main thread execution so that should be fine, and the RPCNotifyBlockChange call should be cs_main-safe as it only notify_all()s and the cs_blockchange mutex is never taken with cs_main in another thread. Obviously this really sucks and risks future bugs and I’d like to see #11856 kill the uiInterface callback there entirely, but for a backport/fix, it looks pretty safe to me with current code.

in src/validation.cpp:2683 in 50ef361e66 outdated

2676@@ -2677,14 +2677,15 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
2677                 assert(trace.pblock && trace.pindex);
2678                 GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
2679             }
2680+
2681+            // Notify external listeners about the new tip.
2682+            // Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected
2683+            GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);

promag commented at 2:54 pm on April 16, 2018:

Should add

0AssertLockHeld(cs_main);

to CMainSignals::UpdatedBlockTip? And a comment.

in src/validation.cpp:2691 in 50ef361e66 outdated

2689-        // Notify external listeners about the new tip.
2690-        GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
2691-
2692         // Always notify the UI if a new block tip was connected
2693         if (pindexFork != pindexNewTip) {
2694             uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);

promag commented at 2:56 pm on April 16, 2018:

This means that the UI can present a tip that is not the tip?

skeees commented at 3:53 pm on April 16, 2018: contributor

Consensus seems to be to update the gui callbacks in cs_main as well (until #11856 which moves these gui callbacks to the ValidationInterface) - latest commit does that and addresses @promag’s other review comment

jnewbery commented at 5:06 pm on April 16, 2018: member

ACK b6f486c85c85e024f03941cbbeadb348ddceaaf8. Commits can be squashed.

skeees force-pushed on Apr 16, 2018

skeees commented at 6:03 pm on April 16, 2018: contributor

Squashed

jnewbery commented at 6:19 pm on April 16, 2018: member

ACK 713c066

0→ git diff b6f486c 713c066

in src/validationinterface.cpp:145 in 713c066cdc outdated

138@@ -139,6 +139,11 @@ void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason
139 }
140 
141 void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {
142+    // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which
143+    // the chain actually updates. In order to ensure this, these signals should be enqueued
144+    // in the same critical section where the chain is updated
145+    AssertLockHeld(cs_main);

TheBlueMatt commented at 7:32 pm on April 16, 2018:

Bleh, if you’re gonna assert this here you should probably assert it in pretty much every signal, as they all have ordering requirements. It also just seems kinda strange to do so cause its not really a requirement of the validationinterface, its a requirement that they be ordered, not done in one specific cs (eg the mempool ones could probably be ordered by mempool.cs instead).

sdaftuar commented at 8:50 pm on April 16, 2018:

I was also not loving this AssertLockHeld() for the same reason. I’d prefer we drop it and just leave a comment explaining the ordering requirement (maybe with a comment mentioning the cs_main lock as one way to achieve it).

skeees commented at 10:04 pm on April 16, 2018:

Agreed - updated

promag commented at 10:26 pm on April 16, 2018:

I’m not a fan of spreading AssertLockHeld either but in this case the current fix is to lock and having the assert here makes that explicitly required. With just the comment a future refactor can break that requirement and reintroduce the bug.

But it was just a suggestion.

sdaftuar commented at 12:54 pm on April 17, 2018:

Note that the AssertLockHeld wouldn’t have prevented the caller from releasing cs_main after updating the tip, and reacquiring cs_main before the validation interface callbacks – which would still result in violating the ordering requirements.

promag commented at 1:07 pm on April 17, 2018:

@sdaftuar that’s true and the same could be said to every AssertLockHeld 😛

TheBlueMatt commented at 7:34 pm on April 16, 2018: member

utACK 713c066cdc1cc4750dfa1d41149621356ff2ad04

Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip

Ensures that callbacks are invoked in the order in which the chain is updated
Resolves #12978

d86edd3d30

skeees force-pushed on Apr 16, 2018

promag commented at 1:08 pm on April 17, 2018: member

utACK d86edd3.

sdaftuar commented at 1:34 pm on April 17, 2018: member

utACK d86edd3

laanwj merged this on Apr 17, 2018

laanwj closed this on Apr 17, 2018

laanwj referenced this in commit 39e0c65b29 on Apr 17, 2018

MarcoFalke referenced this in commit 9000fab07a on Apr 20, 2018

skeees deleted the branch on Apr 21, 2018

laanwj added the label Validation on May 3, 2018

laanwj added the label Needs backport on May 3, 2018

laanwj added this to the milestone 0.16.1 on May 3, 2018

fanquake referenced this in commit ac6c2c6bf7 on May 17, 2018

fanquake removed the label Needs backport on May 17, 2018

fanquake referenced this in commit e56783e09a on May 17, 2018

fanquake referenced this in commit acdf433822 on May 18, 2018

laanwj referenced this in commit 50b2c9e0df on May 24, 2018

HashUnlimited referenced this in commit 6dce29170c on Jun 29, 2018

ccebrecos referenced this in commit 8336ba655e on Sep 14, 2018

PastaPastaPasta referenced this in commit 4cc8596d68 on Nov 10, 2020

PastaPastaPasta referenced this in commit 804428c795 on Nov 12, 2020

PastaPastaPasta referenced this in commit 06aebf5eb0 on Nov 17, 2020

random-zebra referenced this in commit b993c56afe on Apr 4, 2021

DrahtBot locked this on Dec 16, 2021

Hold cs_main while calling UpdatedBlockTip() signal #12988