Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jan 2, 2019

Conversation

Projects
None yet
@jamesob
Copy link
Member

commented Oct 10, 2018

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

@jamesob jamesob force-pushed the jamesob:2018-10-invalid-tx-tests branch Oct 10, 2018

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

Excellent! Thanks for doing this

Concept ACK

@fanquake fanquake added the Tests label Oct 10, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

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

@jimmysong
Copy link
Contributor

left a comment

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

test/functional/feature_block.py Outdated
@@ -7,7 +7,9 @@
import struct
import time

from test_framework.blocktools import create_block, create_coinbase, create_tx_with_script, get_legacy_sigopcount_block
from test_framework.blocktools import (
create_block, create_coinbase, create_tx_with_script, get_legacy_sigopcount_block,

This comment has been minimized.

Copy link
@jimmysong

jimmysong Oct 12, 2018

Contributor

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

Show resolved Hide resolved test/functional/feature_block.py
test/functional/data/invalid_txs.py Outdated
return tx



This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 13, 2018

Member

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 :-)

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


class BadTxTemplate:
"""Allows simple consruction of a certain kind of invalid tx. Base class to be subclassed."""

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 16, 2018

Member

Should be "construction" :-)

@jamesob jamesob force-pushed the jamesob:2018-10-invalid-tx-tests branch 2 times, most recently Oct 16, 2018

@jamesob

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

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

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Concept ACK

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Neat!

utACK 74306a55a5fd15ed194d8addb50cb55a849f293c

@MarcoFalke
Copy link
Member

left a comment

Concept ACK

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

@jamesob jamesob force-pushed the jamesob:2018-10-invalid-tx-tests branch Oct 24, 2018

@jamesob

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

Addressed @MarcoFalke's feedback.

@conscott
Copy link
Contributor

left a comment

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
Copy link
Contributor

left a comment

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.

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

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

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.

@MarcoFalke
Copy link
Member

left a comment

Concept ACK 428c144fc3acc02d36cb0e01dd631d39f0e9a00c

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

@jamesob jamesob force-pushed the jamesob:2018-10-invalid-tx-tests branch Nov 27, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

travis fails :(

cc @practicalswift

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

@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 :-)

test: add invalid tx templates for use in functional tests
Add templates for easily constructing different kinds of invalid
transactions and use them in feature_block and p2p_invalid_tx.

@jamesob jamesob force-pushed the jamesob:2018-10-invalid-tx-tests branch to 59e3877 Nov 27, 2018

@jamesob

This comment has been minimized.

Copy link
Member Author

commented Nov 27, 2018

@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.

# 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.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 30, 2018

Member

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 22, 2018

Member

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)

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 22, 2018

Member

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
Copy link
Contributor

left a comment

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

utACK 59e3877

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 2, 2019

Merge #14457: test: add invalid tx templates for use in functional tests
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.