Address Travis spurious failures #8680
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… 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.
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". |
| { | ||
| if (fHelp || params.size() < 1 || params.size() > 2) | ||
| throw runtime_error( | ||
| "waitforblock\n" |
Contributor
There was a problem hiding this comment.
nit: s/waitforblock/waitforblock <blockhash> <timeout>
I guess most commands help have it like this.
Contributor
|
Concept ACK. |
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. |
laanwj
added a commit
that referenced
this pull request
Sep 9, 2016
d6a5dc4 add waitfornewblock/waitforblock/waitforblockheight rpcs and use them for tests (Cory Fields)
| return True | ||
| time.sleep(wait) | ||
| if heights == [ heights[0] ]*len(heights): #heights are the same but hashes are not | ||
| raise AssertionError("Block sync failed") |
Member
There was a problem hiding this comment.
Nit: mempool_packages.py relies on this not being thrown.
Member
There was a problem hiding this comment.
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).
Member
|
Just to be sure: The new rpc calls are not meant for general consumption but serve as a temporary fix for the tests? |
Member
|
Does this need a backport? |
Member
|
@luke-jr No, the commit that reworked the locks is only in master |
Member
|
This will also break the tests, when an old binary is used for comp. checks. |
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. |
maflcko
reviewed
Oct 2, 2016
| { | ||
| if (fHelp || params.size() < 1 || params.size() > 2) | ||
| throw runtime_error( | ||
| "waitforblock\n" |
Member
There was a problem hiding this comment.
waitforblockheight
Also there is a spurious opening { some lines below.
laanwj
pushed a commit
that referenced
this pull request
Nov 23, 2016
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in #8680 (comment)
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Nov 23, 2016
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment) Github-Pull: bitcoin#9196 Rebased-From: 67c6326
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
Dec 2, 2016
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment) Github-Pull: bitcoin#9196 Rebased-From: 67c6326
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Mar 22, 2017
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment) Github-Pull: bitcoin#9196 Rebased-From: 67c6326
UdjinM6
added a commit
to dashpay/dash
that referenced
this pull request
Apr 11, 2017
* Implement BIP 9 GBT changes - BIP9DeploymentInfo struct for static deployment info - VersionBitsDeploymentInfo: Avoid C++11ism by commenting parameter names - getblocktemplate: Make sure to set deployments in the version if it is LOCKED_IN - In this commit, all rules are considered required for clients to support * qa/rpc-tests: bip9-softforks: Add tests for getblocktemplate versionbits updates * getblocktemplate: Explicitly handle the distinction between GBT-affecting softforks vs not * getblocktemplate: Use version/force mutation to support pre-BIP9 clients * Don't use floating point Github-Pull: bitcoin#8317 Rebased-From: 477777f * Send tip change notification from invalidateblock This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment) Github-Pull: bitcoin#9196 Rebased-From: 67c6326 * torcontrol: Explicitly request RSA1024 private key When generating a new service key, explicitly request a RSA1024 one. The bitcoin P2P protocol has no support for the longer hidden service names that will come with ed25519 keys, until it does, we depend on the old hidden service type so make this explicit. See bitcoin#9214. Github-Pull: bitcoin#9234 Rebased-From: 7d3b627 * Bugfix: FRT: don't terminate when keypool is empty Github-Pull: bitcoin#9295 Rebased-From: c24a4f5 * add fundrawtransaction test on a locked wallet with empty keypool Github-Pull: bitcoin#9295 Rebased-From: 1a6eacb
AmirolAhmad
pushed a commit
to AmirolAhmad/gobytes-test
that referenced
this pull request
Dec 11, 2017
* Implement BIP 9 GBT changes - BIP9DeploymentInfo struct for static deployment info - VersionBitsDeploymentInfo: Avoid C++11ism by commenting parameter names - getblocktemplate: Make sure to set deployments in the version if it is LOCKED_IN - In this commit, all rules are considered required for clients to support * qa/rpc-tests: bip9-softforks: Add tests for getblocktemplate versionbits updates * getblocktemplate: Explicitly handle the distinction between GBT-affecting softforks vs not * getblocktemplate: Use version/force mutation to support pre-BIP9 clients * Don't use floating point Github-Pull: #8317 Rebased-From: 477777f2503e3a56a267556f0fc5091042d93340 * Send tip change notification from invalidateblock This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin/bitcoin#8680 (comment) Github-Pull: #9196 Rebased-From: 67c6326abd1788e6f411feb4f44b69774e76aae2 * torcontrol: Explicitly request RSA1024 private key When generating a new service key, explicitly request a RSA1024 one. The bitcoin P2P protocol has no support for the longer hidden service names that will come with ed25519 keys, until it does, we depend on the old hidden service type so make this explicit. See #9214. Github-Pull: #9234 Rebased-From: 7d3b627395582ae7c9d54ebdbc68096d7042162b * Bugfix: FRT: don't terminate when keypool is empty Github-Pull: #9295 Rebased-From: c24a4f5981d47d55aa9e4eb40294832a4d38fb80 * add fundrawtransaction test on a locked wallet with empty keypool Github-Pull: #9295 Rebased-From: 1a6eacbf3b7e3d5941fec1154079bbc4678ce861
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Jan 9, 2018
d6a5dc4 add waitfornewblock/waitforblock/waitforblockheight rpcs and use them for tests (Cory Fields)
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Jan 9, 2018
d6a5dc4 add waitfornewblock/waitforblock/waitforblockheight rpcs and use them for tests (Cory Fields)
lateminer
pushed a commit
to lateminer/bitcoin
that referenced
this pull request
Jan 13, 2018
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment)
thokon00
pushed a commit
to faircoin/faircoin
that referenced
this pull request
Apr 17, 2018
This change is needed to prevent sync_blocks timeouts in the mempool_reorg test after the sync_blocks update in the upcoming commit "[qa] Change sync_blocks to pick smarter maxheight". This change was initially suggested by Suhas Daftuar <[email protected]> in bitcoin#8680 (comment) Github-Pull: bitcoin#9196 Rebased-From: 67c6326
andvgal
pushed a commit
to energicryptocurrency/gen2-energi
that referenced
this pull request
Jan 6, 2019
d6a5dc4 add waitfornewblock/waitforblock/waitforblockheight rpcs and use them for tests (Cory Fields)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.