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

Merged
merged 1 commit into from Oct 29, 2015

Conversation

Projects
None yet
5 participants
@domob1812
Contributor

domob1812 commented Oct 21, 2015

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.

@laanwj laanwj added the Tests label Oct 21, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 23, 2015

Member

utACK

Member

laanwj commented Oct 23, 2015

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 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.

Member

laanwj commented Oct 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Oct 26, 2015

Contributor

utACK

I'd test both first and second being the null. :)

Contributor

petertodd commented Oct 26, 2015

utACK

I'd test both first and second being the null. :)

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Oct 27, 2015

Contributor

Sounds like a reasonable plan! ;) I'll update the patch accordingly later today.

Contributor

domob1812 commented Oct 27, 2015

Sounds like a reasonable plan! ;) I'll update the patch accordingly later today.

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

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Oct 27, 2015

Contributor

Pushed the extended patch.

Contributor

domob1812 commented Oct 27, 2015

Pushed the extended patch.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Oct 27, 2015

Contributor

utACK

Contributor

petertodd commented Oct 27, 2015

utACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Oct 27, 2015

Contributor

ut ACK

Contributor

jgarzik commented Oct 27, 2015

ut ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 28, 2015

Member

The new test contains a much more structured transaction only. Is there a reason to delete the previous one?

Member

sipa commented Oct 28, 2015

The new test contains a much more structured transaction only. Is there a reason to delete the previous one?

@domob1812

This comment has been minimized.

Show comment
Hide comment
@domob1812

domob1812 Oct 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).

Contributor

domob1812 commented Oct 28, 2015

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 laanwj merged commit 0be387a into bitcoin:master Oct 29, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Oct 29, 2015

Merge pull request #6863
0be387a unittest: fix test for null tx input (Daniel Kraft)

@domob1812 domob1812 deleted the domob1812:null-txin-test branch Oct 29, 2015

domob1812 added a commit to domob1812/huntercore that referenced this pull request Nov 17, 2015

Fix some of the failing unit tests.
Adapt some of the existing and failing unit tests to the changed
parameters of Huntercoin.  Not yet updated and still failing are many
name tests.  Also one of the transaction tests is still failing, but
this seems to be related to a wrongly written test itself, see
bitcoin/bitcoin#6863.  Trying to get this merged
upstream instead of a custom fix here.

Furthermore, there are not yet any actual unit tests written newly for
Huntercoin or game logic in general.

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Nov 18, 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.

Github-Pull: #6863
Rebased-From: 0be387a

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 8, 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.

Github-Pull: #6863
Rebased-From: 0be387a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment