[Test Suite] Fix test for null tx input #6863

pull domob1812 wants to merge 1 commits into bitcoin:master from domob1812:null-txin-test changing 1 files +7 −3

domob1812 commented at 9:58 AM on October 21, 2015: contributor

Update the unittest that is meant to catch a transaction that is invalid because it has a null input. The old test failed not because of that but because it was considered a coinbase with too large script. This is already checked with a different test, though.

The new test is not a coinbase since it has two inputs, but one of them is null. This really checks the corresponding code path in CheckTransaction. If this check is disabled there in main.cpp, the old unittest still succeeded. It does no longer succeed with this patch.
domob1812 referenced this in commit f8a3f56e50 on Oct 21, 2015
laanwj added the label Tests on Oct 21, 2015
laanwj commented at 10:19 AM on October 23, 2015: member

utACK
laanwj commented at 8:00 AM on October 26, 2015: member

From @gmaxwell on IRC: Personally I would have put the 0100 input in the first position, for fear that some coinbase checking code only checks the nullness of the first input.
petertodd commented at 9:27 PM on October 26, 2015: contributor

utACK

I'd test both first and second being the null. :)
domob1812 commented at 7:24 AM on October 27, 2015: contributor

Sounds like a reasonable plan! ;) I'll update the patch accordingly later today.
domob1812 force-pushed on Oct 27, 2015

unittest: fix test for null tx input

Update the unittest that is meant to catch a transaction that is invalid
because it has a null input.  The old test failed not because of that
but because it was considered a coinbase with too large script.  This is
already checked with a different test, though.

The new test is *not* a coinbase since it has two inputs, but one of
them is null.  This really checks the corresponding code path in
CheckTransaction.

0be387a536

domob1812 force-pushed on Oct 27, 2015
domob1812 commented at 7:13 PM on October 27, 2015: contributor

Pushed the extended patch.
petertodd commented at 7:21 PM on October 27, 2015: contributor

utACK
jgarzik commented at 7:29 PM on October 27, 2015: contributor

ut ACK
sipa commented at 1:48 AM on October 28, 2015: member

The new test contains a much more structured transaction only. Is there a reason to delete the previous one?
domob1812 commented at 6:44 AM on October 28, 2015: contributor

The previous test didn't do what it was advertised to do - it failed not because of the null txin but because of the coinbase length restriction. This is already tested by the preceding test, so I don't see much of a point in having two tests for that. But if you think it should stay, I can leave it in (but "fix" the comment for what it really tests).
laanwj merged this on Oct 29, 2015
laanwj closed this on Oct 29, 2015
laanwj referenced this in commit 725539ea03 on Oct 29, 2015
domob1812 deleted the branch on Oct 29, 2015
domob1812 referenced this in commit 2d6a3a8f6e on Nov 17, 2015
luke-jr referenced this in commit b3e7fe2bc6 on Nov 18, 2015
luke-jr referenced this in commit fca4cf193e on Dec 8, 2015
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels