test: add invalid tx templates for use in functional tests #14457

jamesob · 2018-10-10T09:30:52Z

This change adds a list of CTransaction-generating templates which each correspond to a specific type of invalid transaction. We then use this list to test for a wider variety of invalid tx types in p2p_invalid_tx.py and feature_block.py.

Consolidating all invalid tx types will allow us to more easily cover all tx reject cases from a variety of tests without repeating ourselves. Validation logic doesn't differ much between mempool and block acceptance, but there is a difference and we should be sure we're testing both comprehensively.

Right now, I've only added templates covering the tx reject types listed below but if this approach seems worthwhile I will expand the list to be fully comprehensive.

bad-txns-in-belowout
bad-txns-inputs-duplicate
bad-txns-too-many-sigops
bad-txns-vin-empty
bad-txns-vout-empty
bad-txns-vout-negative

practicalswift · 2018-10-10T09:34:35Z

Excellent! Thanks for doing this

Concept ACK

promag · 2018-10-11T06:42:24Z

Concept ACK! And looking the code LGTM. IMO there is no need to be comprehensive.

jimmysong

A couple of nits, but overall, very solid PR. Thanks!

jimmysong · 2018-10-12T23:09:45Z

test/functional/feature_block.py

nit: put these one per line so diffs are easier to see later.

test/functional/feature_block.py

practicalswift · 2018-10-13T13:46:41Z

test/functional/data/invalid_txs.py

Since this is a new file you might want to run it file through black to get PEP-8 formatting. Including fixing this case of too many blank lines :-)

test/functional/feature_block.py

test/functional/data/invalid_txs.py

practicalswift · 2018-10-16T13:06:54Z

test/functional/data/invalid_txs.py

Should be "construction" :-)

jamesob · 2018-10-16T18:47:08Z

Addressed @practicalswift @jimmysong feedback. Thanks for the looks.

sipa · 2018-10-17T07:19:21Z

Concept ACK

jonasschnelli · 2018-10-17T07:41:58Z

Neat!

utACK 74306a55a5fd15ed194d8addb50cb55a849f293c

maflcko

Concept ACK

test/functional/data/invalid_txs.py

jamesob · 2018-10-24T19:45:20Z

Addressed @MarcoFalke's feedback.

conscott

Tested ACK 428c144fc3acc02d36cb0e01dd631d39f0e9a00c

Nice! I guess the follow up is to also use these in the mempool_accept.py test, possibly adding bad-txns-vout-toolarge, bad-txns-txouttotal-toolarge

ryanofsky

utACK 428c144fc3acc02d36cb0e01dd631d39f0e9a00c, with caveat that I didn't really take time to understand changes to feature_block.py, though they seem reasonable. The new module and changes to p2p_invalid_tx.py look good.

test/functional/feature_block.py

DrahtBot · 2018-11-21T09:08:23Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14696 (qa: Add explicit references to related CVE's in p2p_invalid_block test. by lucash-dev)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko

Concept ACK 428c144fc3acc02d36cb0e01dd631d39f0e9a00c

test/functional/data/invalid_txs.py

test/functional/feature_block.py

maflcko · 2018-11-27T22:32:05Z

travis fails :(

cc @practicalswift

practicalswift · 2018-11-27T22:44:02Z

@MarcoFalke Thanks for the ping!

@jamesob vulture identifies MAX_P2SH_SIGOPS as unused. Regarding the other warnings: it seems like vulture doesn't understand that the classes are referenced using BadTxTemplate.__subclasses__(). Could the class usage be made more explicit so that vulture (and humans) can infer it? If not, add the class names to --ignore-names in test/lint/lint-python-dead-code.sh :-)

Add templates for easily constructing different kinds of invalid transactions and use them in feature_block and p2p_invalid_tx.

jamesob · 2018-11-27T22:55:58Z

@practicalswift thanks for the advice. I've excluded the whole invalid_txs.py file from analysis by vulture because there are more than a few class names spuriously detected, and any time someone adds a new invalid txn class they'd have to update that file otherwise.

maflcko · 2018-11-30T20:47:29Z

test/functional/feature_block.py

+            # Something about the serialization code for missing inputs creates
+            # a different hash in the test client than on bitcoind, resulting
+            # in a mismatching merkle root during block validation.
+            # Skip until we figure out what's going on.


missing inputs means the vin is empty and is thus interpreted as the witness dummy vin?

See:

tx as sent: CTransaction(nVersion=1 vin=[] vout=[CTxOut(nValue=0.00000000 scriptPubKey=51515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151515151)] wit=CTxWitness() nLockTime=0) tx as received: CTransaction(nVersion=1 vin=[] vout=[] wit=CTxWitness() nLockTime=0)

One solution would be:

diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 697a0b19ac..edbdcf2d55 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -137,12 +137,6 @@ class FullBlockTest(BitcoinTestFramework): for TxTemplate in invalid_txs.iter_all_templates(): template = TxTemplate(spend_tx=attempt_spend_tx) - # Something about the serialization code for missing inputs creates - # a different hash in the test client than on bitcoind, resulting - # in a mismatching merkle root during block validation. - # Skip until we figure out what's going on. - if TxTemplate == invalid_txs.InputMissing: - continue if template.valid_in_block: continue @@ -150,7 +144,10 @@ class FullBlockTest(BitcoinTestFramework): blockname = "for_invalid.%s" % TxTemplate.__name__ badblock = self.next_block(blockname) badtx = template.get_tx() - self.sign_tx(badtx, attempt_spend_tx) + if TxTemplate != invalid_txs.InputMissing: + self.sign_tx(badtx, attempt_spend_tx) + else: + badtx.vout = [] # Also set outputs empty, so we can calculate the correct hash badtx.rehash() badblock = self.update_block(blockname, [badtx]) self.sync_blocks(

ryanofsky

utACK 59e3877. Changes since last review: rebase, minor tweaks, variable, renames, dead code linter update.

laanwj · 2019-01-02T13:43:57Z

utACK 59e3877

59e3877 test: add invalid tx templates for use in functional tests (James O'Beirne) Pull request description: This change adds a list of `CTransaction`-generating templates which each correspond to a specific type of invalid transaction. We then use this list to test for a wider variety of invalid tx types in `p2p_invalid_tx.py` and `feature_block.py`. Consolidating all invalid tx types will allow us to more easily cover all tx reject cases from a variety of tests without repeating ourselves. Validation logic doesn't differ much between mempool and block acceptance, but there *is* a difference and we should be sure we're testing both comprehensively. Right now, I've only added templates covering the tx reject types listed below but if this approach seems worthwhile I will expand the list to be fully comprehensive. ``` bad-txns-in-belowout bad-txns-inputs-duplicate bad-txns-too-many-sigops bad-txns-vin-empty bad-txns-vout-empty bad-txns-vout-negative ``` Tree-SHA512: 05407f4a953fbd7c44c08bb49bb989cefd39a2b05ea00f5b3c92197a3f05e1b302f789e33832445734220e1c333d133aba385740b77b84139b170c583471ce20

jamesob force-pushed the 2018-10-invalid-tx-tests branch from 53cd753 to 1483640 Compare October 10, 2018 09:31

fanquake added the Tests label Oct 10, 2018

jimmysong reviewed Oct 12, 2018

View reviewed changes

practicalswift reviewed Oct 13, 2018

View reviewed changes

test/functional/feature_block.py Outdated Show resolved Hide resolved

practicalswift reviewed Oct 13, 2018

View reviewed changes

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

practicalswift reviewed Oct 16, 2018

View reviewed changes

test/functional/data/invalid_txs.py Outdated

Copy link

Contributor

practicalswift Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "construction" :-)

jamesob force-pushed the 2018-10-invalid-tx-tests branch 2 times, most recently from c0ea4ad to 74306a5 Compare October 16, 2018 16:57

practicalswift mentioned this pull request Oct 22, 2018

Enable flake8 rule E225 which checks for missing whitespace around op… #14534

Closed

maflcko reviewed Oct 23, 2018

View reviewed changes

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

jamesob force-pushed the 2018-10-invalid-tx-tests branch from 74306a5 to 428c144 Compare October 24, 2018 19:45

conscott reviewed Oct 25, 2018

View reviewed changes

ryanofsky approved these changes Nov 7, 2018

View reviewed changes

test/functional/feature_block.py Outdated Show resolved Hide resolved

maflcko reviewed Nov 27, 2018

View reviewed changes

test/functional/data/invalid_txs.py Outdated Show resolved Hide resolved

test/functional/feature_block.py Outdated Show resolved Hide resolved

jamesob force-pushed the 2018-10-invalid-tx-tests branch from 57c6e5b to e45f4a8 Compare November 27, 2018 22:22

test: add invalid tx templates for use in functional tests

59e3877

Add templates for easily constructing different kinds of invalid transactions and use them in feature_block and p2p_invalid_tx.

jamesob force-pushed the 2018-10-invalid-tx-tests branch from e45f4a8 to 59e3877 Compare November 27, 2018 22:54

maflcko reviewed Nov 30, 2018

View reviewed changes

ryanofsky approved these changes Dec 4, 2018

View reviewed changes

laanwj merged commit 59e3877 into bitcoin:master Jan 2, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

test: add invalid tx templates for use in functional tests #14457

test: add invalid tx templates for use in functional tests #14457

Uh oh!

Conversation

jamesob commented Oct 10, 2018

Uh oh!

practicalswift commented Oct 10, 2018

Uh oh!

promag commented Oct 11, 2018

Uh oh!

jimmysong left a comment

Choose a reason for hiding this comment

Uh oh!

jimmysong Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

practicalswift Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob commented Oct 16, 2018

Uh oh!

sipa commented Oct 17, 2018

Uh oh!

jonasschnelli commented Oct 17, 2018

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jamesob commented Oct 24, 2018

Uh oh!

conscott left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Nov 27, 2018

Uh oh!

practicalswift commented Nov 27, 2018

Uh oh!

jamesob commented Nov 27, 2018

Uh oh!

maflcko Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 22, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 22, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 2, 2019

Uh oh!

Reviewers

Assignees

Labels

practicalswift Oct 13, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

DrahtBot commented Nov 21, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading