Fix segwit getblocktemplate test #9843

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:fixsegwitgetblocktemplate changing 1 files +2 −13

jnewbery commented at 10:22 PM on February 23, 2017: member

Fixes #9841
jnewbery force-pushed on Feb 23, 2017
luke-jr commented at 10:27 PM on February 23, 2017: member

It's not broken...?
fanquake added the label Tests on Feb 23, 2017
Fix segwit getblocktemplate test. b23dcd2bf9
jnewbery force-pushed on Feb 23, 2017
jnewbery commented at 11:03 PM on February 23, 2017: member

Yes it is. There are lines of code in the test which aren't testing anything, and the test code is catching an error without asserting the error type or error text.

A future commit could break GBT in some other way and the broad exception handling would mask that.

There's also a print() two lines above which says: "Verify non-segwit miners get a valid GBT response after the fork". Non-segwit miners are not supported after segwit activation.
TheBlueMatt commented at 7:28 PM on February 24, 2017: member

utACK b23dcd2bf98d7b4b672c3ea3782ca5c5b9c64d7b
gmaxwell commented at 7:39 AM on February 25, 2017: contributor

Non-segwit miners are not supported after segwit activation.

I believe this is an error in the software. But agree that this is the current behavior.

Concept ACK fixing the test as it is.
in qa/rpc-tests/segwit.py:None in b23dcd2bf9
```
 261 | -            assert(tmpl['transactions'][0]['sigops'] == 2)
 262 | -            assert(('!segwit' in tmpl['rules']) or ('segwit' not in tmpl['rules']))
 263 | -        except JSONRPCException:
 264 | -            # This is an acceptable outcome
 265 | -            pass
 266 | +        assert_raises_jsonrpc(-8, "Support for 'segwit' rule requires explicit client support", self.nodes[0].getblocktemplate, {})
```
MarcoFalke commented at 1:10 PM on February 25, 2017:

You could restore the deleted lines and put this line over pass, so the test need not be rewritten in case this is ever changed.

jnewbery commented at 4:25 PM on February 25, 2017:

Thanks @MarcoFalke. I'm not a fan of leaving unexecuted lines in the test code just in case the product code is changed in the future. Re-adding a few asserts in this test case is negligible effort compared to actually adding the functionality to bitcoin core (which would require significant changes to the mining code).

And if writing 7 assert lines is really too much effort, git history is a far better place to keep this dead code :)
MarcoFalke commented at 1:11 PM on February 25, 2017: member

utACK b23dcd2bf98d7b4b672c3ea3782ca5c5b9c64d7b for checking the error code and message.
sdaftuar commented at 8:20 PM on February 27, 2017: member

utACK b23dcd2
MarcoFalke added the label Needs backport on Feb 27, 2017
MarcoFalke commented at 8:22 PM on February 27, 2017: member

Tagging for (potential?) backport.
laanwj merged this on Mar 6, 2017
laanwj closed this on Mar 6, 2017
laanwj referenced this in commit c78adbf450 on Mar 6, 2017
laanwj added this to the milestone 0.14.1 on Mar 6, 2017
MarcoFalke removed the label Needs backport on Mar 27, 2017
MarcoFalke removed this from the milestone 0.14.1 on Mar 27, 2017
MarcoFalke commented at 9:34 PM on March 27, 2017: member

Backport not needed due to #10006
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9841 getblocktemplate fails for old clients after segwit activation