test: add tests for `bad-txns-prevout-null` reject reason #22408

theStack commented at 10:14 PM on July 5, 2021: member

This simple PR adds missing tests for the reject reason bad-txns-prevout-null, which is thrown in the function CheckTransaction(): https://github.com/bitcoin/bitcoin/blob/a62fc35a150da584d39d7cd01ade14bbb5002fb9/src/consensus/tx_check.cpp#L52-L54

Basically this condition is met for non-coinbase transactions (the code snippet above only hits if !tx.IsCoinBase()) with coinbase-like outpoints, i.e. hash=0, n=0xffffffff.

Can be tested by running the functional tests feature_block.py, p2p_invalid_tx.py and mempool_accept.py. Not sure if the redundancy in the tests is desired (I guess it would make sense if the mempool acceptance test also makes use of the invalid_txs templates?).

test: add `bad-txns-prevout-null` test case to invalid_txs.py

This reject reason is triggered for non-coinbase transactions with
a coinbase-like outpoint, i.e. hash=0, n=0xffffffff.

Note that the invalid tx templates are currently used in the
functional tests feature_block.py and p2p_invalid_tx.py.

aa0a5bb70d

test: add `bad-txns-prevout-null` test to mempool_accept.py 1f449586a9

DrahtBot added the label Tests on Jul 5, 2021

brunoerg approved

brunoerg commented at 2:45 PM on July 6, 2021: member

tACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b

Ran feature_block.py, p2p_invalid_tx.py and mempool_accept.py, all passed. (MacOS 11.3) I also ran test_runner, seems ok.

rajarshimaitra commented at 3:37 PM on July 6, 2021: contributor

tACK https://github.com/bitcoin/bitcoin/pull/22408/commits/1f449586a9e39bc4fb53cb5c7a31362e47aea19b

Few generic points I was pondering about.

Is such invalid transaction handling and intended behavior is same for both p2p and mempool validation? If not we might wanna have some checks there too??
What's the purpose of PrevoutNullInput and other classes in invalid_txs.py in general? It seems they are not much used anywhere?
It seems here we are having a previously valid tx, and then appending an invalid input https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/mempool_accept.py#L252-L253 Which is same as https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/data/invalid_txs.py#L160-L161 in PrevoutNullInput get_tx() construction.

So maybe it can be used there instead of doing it via hand (adding a reason to have the class)? Or if its just that easy to append an invalid input, does it make sense to have the class?
As you are touching invalid_txs.py, maybe regarding #22363 (review), it makes sense to apply same changes here too? https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/data/invalid_txs.py#L233

It probably makes more sense to change it in #22363 , just wanna note it, as I i missed it in that review.

Edit: I am not sure why code snippets are not showing in the above comments. (Or its just in my local).

theStack commented at 2:24 PM on July 7, 2021: member

Thanks for the reviews so far! 🎉 @rajarshimaitra:

Is such invalid transaction handling and intended behavior is same for both p2p and mempool validation? If not we might wanna have some checks there too??

The routine throwing bad-txns-prevout-null (-> CheckTransaction()) is called on two places in the codebase: block validation (CheckBlock(...)) and mempool acceptance (MemPoolAccept::PreChecks(...)). Both are covered by the tests in this PR: block validation in feature_block.py and mempool acceptance in p2p_invalid_txs.py (tx sent to peer, which tries to add it to its mempool) and mempool_accept.py (tx locally added to mempool).

What's the purpose of PrevoutNullInput and other classes in invalid_txs.py in general? It seems they are not much used anywhere?

invalid_txs.py provides a general template framework for invalid txs. It is currently used in the tests feature_block.py and p2p_invalid_tx.py (see also first commit message). All classes inheriting of BadTxTemplate can be iterated with the following loop: for TxTemplate in invalid_txs.iter_all_templates():. Also found this a bit confusing that the classes are never directly referenced anywhere :)

It seems here we are having a previously valid tx, and then appending an invalid input [...] So maybe it can be used there instead of doing it via hand (adding a reason to have the class)? Or if its just that easy to append an invalid input, does it make sense to have the class?

Good point, I think reusing the invalid_tx templates in mempool_accept.py would be worth a separate PR.

As you are touching invalid_txs.py, maybe regarding test: refactor: use script_util helpers for creating P2{PKH,SH,WPKH,WSH} scripts [#22363](/bitcoin-bitcoin/22363/) (comment), it makes sense to apply same changes here too? https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/data/invalid_txs.py#L233 It probably makes more sense to change it in test: refactor: use script_util helpers for creating P2{PKH,SH,WPKH,WSH} scripts [#22363](/bitcoin-bitcoin/22363/) , just wanna note it, as I i missed it in that review.

Since I am quite confident that #22363 gets merged (has already several Concept ACKs), I don't think there is a need to repeat its style changes in invalid_txs.py in this PR too.

rajarshimaitra commented at 9:25 AM on July 8, 2021: contributor

All classes inheriting of BadTxTemplate can be iterated with the following loop: for TxTemplate in invalid_txs.iter_all_templates():

Thanks for this, yes I was confused for that reason only. Makes sense.

Since I am quite confident that #22363 gets merged (has already several Concept ACKs), I don't think there is a need to repeat its style changes in invalid_txs.py in this PR too.

I think the change I pointed here is not present in #22363, The basic_p2sh was changed to have a use CScript directly, but this line wasn't. https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/data/invalid_txs.py#L233 Otherwise, the same file will contain different formatting of using CScript. I probably should have made the comment in #22363 instead of here.

kristapsk approved

kristapsk commented at 10:49 AM on July 8, 2021: contributor

ACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b, code looks correct and all tests pass.

jonatack commented at 3:43 PM on July 8, 2021: member

ACK 1f449586a9e39bc4fb53cb5c7a31362e47aea19b

MarcoFalke merged this on Jul 8, 2021

MarcoFalke closed this on Jul 8, 2021

theStack commented at 5:00 PM on July 8, 2021: member

@rajarshimaitra:

Since I am quite confident that #22363 gets merged (has already several Concept ACKs), I don't think there is a need to repeat its style changes in invalid_txs.py in this PR too.

I think the change I pointed here is not present in #22363, The basic_p2sh was changed to have a use CScript directly, but this line wasn't. https://github.com/theStack/bitcoin/blob/1f449586a9e39bc4fb53cb5c7a31362e47aea19b/test/functional/data/invalid_txs.py#L233 Otherwise, the same file will contain different formatting of using CScript. I probably should have made the comment in #22363 instead of here.

In the latest version in #22363, all references in invalid_txs.py to sc were eliminated after your suggestion, see commit https://github.com/bitcoin/bitcoin/pull/22363/commits/285a65ccfde2e811cfe01e916b998c02ee534a97, e.g. https://github.com/bitcoin/bitcoin/commit/285a65ccfde2e811cfe01e916b998c02ee534a97#diff-d390e9e12bfa34a462fdff9f1894d7dc3edb45c26cf55df486d92864a3052fafR224-R226 :)

rajarshimaitra commented at 5:41 PM on July 8, 2021: contributor

In the latest version in #22363, all references in invalid_txs.py to sc were eliminated after your suggestion, see commit 285a65c, e.g. 285a65c#diff-d390e9e12bfa34a462fdff9f1894d7dc3edb45c26cf55df486d92864a3052fafR224-R226 :)

Ah Ok, I missed that, my bad..

sidhujag referenced this in commit 51cb2c4456 on Jul 10, 2021

theStack deleted the branch on Jul 31, 2021

gwillen referenced this in commit cf9d252d10 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022