Address Travis spurious failures

theuni commented at 5:23 pm on September 7, 2016: member

As discussed in last week’s meeting. The failures are annoying.

I’m sure that this isn’t the best approach, but these rpcs seem generally useful, and I believe this fixes the current race issue.

The problem is that the wallet is not yet finished dealing with transactions before getblockcount/getbestblockhash return new values, so assertions about balances are racy. The approach I’ve taken here is to wait for a signal that the processing is complete, then cache the new height/hash on the rpc side. That allows for blocking in the call rather than polling, and it ensures that we’re synced up before it returns. It also means no cs_main locking.

To fix sync_blocks in particular, wait for height 0 from all nodes. That returns the current height/hash instantly, since it should never actually have to wait. Then, grab the current height from all nodes, take the max, and wait on that height for all nodes. Repeat until all heights match. @sdaftuar Mentioned that it may be useful to a wait_for_wallet() call, which would presumably work similarly, though I’m afraid it may introduce new race issues. Suggestions on that approach are also welcome.

add waitfornewblock/waitforblock/waitforblockheight rpcs and use them for tests

waitfornewblock waits until a new block is received, or the timeout expires, then
returns the current block height/hash.

waitforblock waits for a specific blockhash, or until the timeout expires, then
returns the current block height/hash. If the target blockhash is the current
tip, it will return immediately.

waitforblockheight waits until the tip has reached a certain height or higher,
then returns the current height and hash.

waitforblockheight is used to avoid polling in the rpc tests.

d6a5dc4a2e

MarcoFalke added the label Tests on Sep 7, 2016

laanwj commented at 5:21 am on September 9, 2016: member

These RPCs happen to also wait for wallet processing at this point, but that will change if wallet processing becomes asynchronous.

ACK on this as a stopgap measure to prevent the synchronization issues.

But I think there should be a warning in their help to not rely on this behavior “as the tests do”.

in src/rpc/blockchain.cpp: in d6a5dc4a2e

230+
231+UniValue waitforblock(const UniValue& params, bool fHelp)
232+{
233+    if (fHelp || params.size() < 1 || params.size() > 2)
234+        throw runtime_error(
235+            "waitforblock\n"

jonasschnelli commented at 6:24 am on September 9, 2016:

nit: s/waitforblock/waitforblock <blockhash> <timeout> I guess most commands help have it like this.

jonasschnelli commented at 6:26 am on September 9, 2016: contributor

Concept ACK. I like this approach. Also, it’s a preform of longpoll RPC notifications proposed in #7949 which I think are generally useful.

laanwj commented at 6:33 am on September 9, 2016: member

Gah I’m just going to go ahead and merge this, I need the tests to pass, it’s low-risk (largest risk is to break the tests even more, well…) and fixups can be done later. utACK d6a5dc4

laanwj merged this on Sep 9, 2016

laanwj closed this on Sep 9, 2016

laanwj referenced this in commit 666eaf03cf on Sep 9, 2016

in qa/rpc-tests/test_framework/util.py: in d6a5dc4a2e

132+        heights = [ x["height"] for x in tips ]
133         if tips == [ tips[0] ]*len(tips):
134             return True
135-        time.sleep(wait)
136+        if heights == [ heights[0] ]*len(heights): #heights are the same but hashes are not
137+            raise AssertionError("Block sync failed")

MarcoFalke commented at 4:21 pm on September 9, 2016:

Nit: mempool_packages.py relies on this not being thrown.

sdaftuar commented at 4:27 pm on September 9, 2016:

There’s a slightly bigger issue that @morcos just noticed, which is that the block notification signals doesn’t quite work with invalidateblock right now.

I think we can work around it by having the invalidateblock rpc handler explicitly call the ui signals in this case (which is probably the right thing to do in general).

MarcoFalke commented at 4:24 pm on September 9, 2016: member

Just to be sure: The new rpc calls are not meant for general consumption but serve as a temporary fix for the tests?

luke-jr commented at 10:03 pm on September 16, 2016: member

Does this need a backport?

MarcoFalke commented at 8:07 am on September 17, 2016: member

@luke-jr No, the commit that reworked the locks is only in master

MarcoFalke commented at 5:37 pm on September 20, 2016: member

This will also break the tests, when an old binary is used for comp. checks.

MarcoFalke commented at 5:38 pm on September 20, 2016: member

There were suggestions by @morcos and @TheBlueMatt to address the failures in a less breaking way during one of the last meetings, but I couldn’t find any follow ups.

in src/rpc/blockchain.cpp: in d6a5dc4a2e

272+
273+UniValue waitforblockheight(const UniValue& params, bool fHelp)
274+{
275+    if (fHelp || params.size() < 1 || params.size() > 2)
276+        throw runtime_error(
277+            "waitforblock\n"

MarcoFalke commented at 11:56 am on October 2, 2016:

waitforblockheight

Also there is a spurious opening { some lines below.

ryanofsky referenced this in commit 9a4f4fa264 on Nov 11, 2016

ryanofsky referenced this in commit 9b74bd69ce on Nov 11, 2016

ryanofsky referenced this in commit d8171a3cbe on Nov 11, 2016

ryanofsky referenced this in commit 5c7afc2caf on Nov 14, 2016

ryanofsky referenced this in commit 04b213a1d0 on Nov 14, 2016

ryanofsky referenced this in commit 60cf51c803 on Nov 15, 2016

ryanofsky referenced this in commit aa698eb55f on Nov 21, 2016

laanwj referenced this in commit 67c6326abd on Nov 23, 2016

MarcoFalke referenced this in commit d0d506518f on Nov 23, 2016

MarcoFalke referenced this in commit cca151b3a4 on Nov 23, 2016

luke-jr referenced this in commit 0c09d9f00e on Dec 2, 2016

UdjinM6 referenced this in commit 8e9a4165f5 on Mar 22, 2017

UdjinM6 referenced this in commit b659d71645 on Mar 22, 2017

UdjinM6 referenced this in commit ff30aed68f on Apr 11, 2017

codablock referenced this in commit a063f30cd4 on Sep 19, 2017

AmirolAhmad referenced this in commit e9c0c3674e on Dec 11, 2017

codablock referenced this in commit f0813d6081 on Jan 9, 2018

codablock referenced this in commit c2a601f507 on Jan 9, 2018

lateminer referenced this in commit 67c3960ddb on Jan 13, 2018

thokon00 referenced this in commit 6e72bb1bb6 on Apr 17, 2018

andvgal referenced this in commit b7896d0b76 on Jan 6, 2019

DrahtBot locked this on Sep 8, 2021

Address Travis spurious failures #8680