Fix versionbits warning test #12264

pull jnewbery wants to merge 3 commits into bitcoin:master from jnewbery:fix_versionbits_warning_test changing 1 files +61 −70

jnewbery commented at 6:59 PM on January 24, 2018: member

fixes #12259 (and tidies up the test)

The problem was that the node was still in IBD at the point the last block was generated. UpdateTip() will not generate a warning if the node is still in IBD:

https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2151

The 'proper' fix would be to remove the overenthusiastic latching in DoWarning:

https://github.com/bitcoin/bitcoin/blob/cc5870a4057f0322509dde5877fb08258bf4ec50/src/validation.cpp#L2135

so that more than one warning message can be output to alertnotify. Really we should suppress multiple messages of the same type, but allow messages to be output if they're for different warnings. That would mean the test wouldn't need to stop-start the node.

in test/functional/p2p-versionbits-warning.py:92 in f96b1eca3b outdated

 107 | -        # Might not get a versionbits-related alert yet, as we should
 108 | -        # have gotten a different alert due to more than 51/100 blocks
 109 | -        # being of unexpected version.
 110 | -        # Check that get*info() shows some kind of error.
 111 | +
 112 | +        self.log.info("Check that there is a warning if <50 blocks in the last 100 were an unknown version")

MarcoFalke commented at 8:02 PM on January 24, 2018:

Should say more than 50, not less than

jnewbery commented at 9:16 PM on January 24, 2018:

Thanks, fixed

in test/functional/p2p-versionbits-warning.py:109 in 9100159ddd outdated
```
 143 | +        node.generate(1)
 144 | +        # Check that get*info() shows the versionbits unknown rules warning
 145 | +        assert(WARN_UNKNOWN_RULES_ACTIVE in node.getmininginfo()["warnings"])
 146 | +        assert(WARN_UNKNOWN_RULES_ACTIVE in node.getnetworkinfo()["warnings"])
 147 | +        # Check that the alert file shows the versionbits unknown rules warning
 148 | +        wait_until(lambda: self.versionbits_in_alert_file(), timeout=60)
```
MarcoFalke commented at 8:09 PM on January 24, 2018:

nit: 60 seconds seems overkill, what about 5 or 10?

jnewbery commented at 9:16 PM on January 24, 2018:

no harm in having it longer

MarcoFalke commented at 12:10 AM on January 25, 2018:

I sometimes make tests fail on purpose, so having to wait a minute (or going into the test each time and having to modify it) is somewhat inconvenient.

MarcoFalke commented at 12:12 AM on January 25, 2018:

Also, should the test fail for some reason, the earlier you know the better. As a rule of thumb, I estimate the time it would take to fulfill the predicate, double that for travis, then double again just to be safe.

jnewbery commented at 12:13 AM on January 25, 2018:

You can change the timeout on your failure-triggering case 🙂

In my opinion it's better to have this too long than too short, to avoid spurious Travis failures.

laanwj commented at 10:37 AM on January 25, 2018:

I also prefer waiting on the long side, if it doesn't slow down the test in the passing case. Travis can be really slow when the servers are contended.
in test/functional/p2p-versionbits-warning.py:101 in 9100159ddd outdated
```
 154 | +
 155 | +        # Stop-start the node. This is required because bitcoind will only warn once about unknown versions or unknown rules activating.
 156 | +        self.restart_node(0)
 157 | +
 158 | +        # Generating one block will get us out of IBD
 159 | +        node.generate(1)
```
MarcoFalke commented at 8:14 PM on January 24, 2018:

Generating that block will not help to get us out of ibd any sooner. Am I mistaken?

jnewbery commented at 9:17 PM on January 24, 2018:

generating a block guarantees that we'll get out of IBD. I've updated the comment.
MarcoFalke commented at 8:15 PM on January 24, 2018: member

Concept ACK
jnewbery force-pushed on Jan 24, 2018
fanquake added the label Tests on Jan 24, 2018
laanwj commented at 10:37 AM on January 25, 2018: member

Seems to need rebase (already?).
MarcoFalke commented at 11:53 AM on January 25, 2018: member

utACK f36607eb8c Needs rebase
Fix flake8 warnings in p2p-versionbits-warning.py ef2beb2c13
Improve comments/logging in p2p-versionbits-warning.py 3bbd843708

Fix intermittent failure in p2p-versionbits-warning.py

Makes following changes to fix and tidy up p2p-versionbits-warning.py:
- add node alias in the run() method
- call versionbits_in_alert_file() in a wait_until loop.
- don't clear out the alert.txt file
- explicitly comment why the node needs to be stop-started
- Verify that the node is out of IBD after stop-start (nodes in IBD do
not generate alert messages)
- no need to subclass P2PInterface

1e2e09e2f6

jnewbery force-pushed on Jan 25, 2018
jnewbery commented at 1:01 PM on January 25, 2018: member

rebased (test file name change)
MarcoFalke merged this on Jan 25, 2018
MarcoFalke closed this on Jan 25, 2018
MarcoFalke referenced this in commit 598a9c4e4d on Jan 25, 2018
jnewbery deleted the branch on Jan 25, 2018
codablock referenced this in commit 1b5159277d on Jan 2, 2020
codablock referenced this in commit 387bb202ae on Jan 2, 2020
UdjinM6 referenced this in commit 7ff96c2405 on Jan 2, 2020
UdjinM6 referenced this in commit 1df3c67a8c on Jan 10, 2020
barrystyle referenced this in commit 84c991c665 on Jan 22, 2020
PastaPastaPasta referenced this in commit f1d3d7c95f on Mar 23, 2020
PastaPastaPasta referenced this in commit 3131992d61 on Mar 28, 2020
PastaPastaPasta referenced this in commit 809da14459 on Jun 13, 2020
PastaPastaPasta referenced this in commit 2078d55a4d on Jun 13, 2020
PastaPastaPasta referenced this in commit 9f9f6bb40a on Jun 17, 2020
PastaPastaPasta referenced this in commit 50c5439ac3 on Jun 18, 2020
FornaxA referenced this in commit 178a77d946 on Jul 6, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12259 [qa] p2p-versionbits-warning.py fails occasionally