blockchain: move block validation rule tests into fullblocktests (1 of x). by dnldd · Pull Request #1060 · decred/dcrd

dnldd · 2018-02-20T00:18:10Z

This PR addsAssertTipBlockStakeRoot helper and moves the following block rejection cases into fullblocktests.

ErrVotesOnWrongBlock
- Attempt to add block with tickets voting on wrong block
ErrIncongruentVotebit
- Attempt to add block with incorrect votebits set
  - Everyone votes yea, but block header says nay
  - Everyone votes nay, but block header says yea
  - 3x nay 2x yea, but block header says yea
  - 2x nay 3x yea, but block header says nay
ErrBadMerkleRoot
- Create block with an invalid stake root
ErrWrongBlockSize
- Create block with an invalid block size
ErrBadCoinbaseAmountIn
- Create block with an invalid subsidy for coinbase output
ErrBadStakebaseAmountIn
- Create block with an invalid subsidy for a stakebase input
ErrRevocationsMismatch
- Create block with a revocations count mismatch

Work towards #1030.

davecgh

Before I dig into the particulars, I noticed the copyright dates on the touched files need to be updated for 2018.

davecgh

I still have to go through each condition more thoroughly, but here is some initial feedback for the first couple of tests.

davecgh · 2018-02-21T16:33:54Z

Please avoid hard coding things like this since it makes the tests more fragile in the face of code changes. It also would technically allow an extremely remote possibility that the generated block actually has the hard coded hash. Instead, it's better to flip a bit in the script to mirror what might happen with an alpha particle.

davecgh · 2018-02-21T16:36:05Z

I would suggest using |= here to avoid changing any other bits that might later be used for other purposes and thus could invalidate the block for the wrong reason.

davecgh · 2018-02-21T17:05:39Z

Rather than repeating the pattern over and over, please create a function similar to ReplaceVotes from chaingen which accepts a specific vote number, version, and bits. I would suggest the following:

// ReplaceVoteBitsN returns a function that itself takes a block and modifies // it by replacing the vote bits of the vote located at the provided index. It // will panic if the stake transaction at the provided index is not already a // vote. // // NOTE: This must only be used as a munger to the 'NextBlock' function or it // will lead to an invalid live ticket pool. func ReplaceVoteBitsN(voteNum int, voteBits uint16) func(*wire.MsgBlock) { return func(b *wire.MsgBlock) { // Attempt to prevent misuse of this function by ensuring the // provided stake transaction number is actually a vote. stx := b.STransactions[voteNum] if !isVoteTx(stx) { panic(fmt.Sprintf("attempt to replace non-vote "+ "transaction #%d for for block %s", voteNum, b.BlockHash())) } // Extract the existing vote version. existingScript := stx.TxOut[1].PkScript var voteVersion uint32 if len(existingScript) >= 8 { voteVersion = binary.LittleEndian.Uint32(existingScript[4:8]) } stx.TxOut[1].PkScript = voteBitsScript(voteBits, voteVersion) } }

That way it uses the existing infrastructure to properly recreate the entire vote script and will panic if it's used improperly to help ensure the tests stay valid.

Then this function will read more like:

b.Header.VoteBits |= 0x0001 const voteBitsNo = 0x000 for i := 0; i < 4; i++ { chaingen.ReplaceVoteBitsN(i, voteBitsNo) }

davecgh · 2018-02-21T17:07:52Z

Then the same deal here with reusing the function from above.

davecgh · 2018-02-24T17:28:29Z

This is not reliable because the block being voted on could already have a zero there in which case the test would fail with a false positive.

You need to ensure some bits are flipped. So, I would suggest using xor. You could do b.STransactions[0].TxOut[0].PkScript[2] ^= 0x55 to flip every other bit in the byte.

davecgh · 2018-02-24T17:28:55Z

Total nitpick, but please add periods to these so they're consistent with the rest of the comments.

davecgh · 2018-02-24T17:33:19Z

I'd personally leave these inner comments off. The intro comment already describes it. Also, the naming is inconsistent since it says "yea" while the variables are named yes. I prefer the yes/no naming.

davecgh · 2018-02-24T17:36:22Z

Rather than setting a specific number, I would suggest doing b.Header.Revocations++ so that the test will remain valid even if the actual number of revocations somehow ends up being 2 which would create a false positive.

davecgh · 2018-02-24T17:37:55Z

Rather than setting a specific number, I would suggest doing b.STransactions[0].TxIn[0].ValueIn++ so that the test can't possibly create any false positives if the actual value in somehow ultimately ends up being the magic number.

davecgh · 2018-02-24T17:38:41Z

Rather than setting a specific number, I would suggest doing b.Header.Size++ so that the test can't possibly create any false positives if the actual value in somehow ultimately ends up being the magic number.

davecgh · 2018-02-24T17:41:02Z

Rather than setting a specific number, I would suggest doing b.STransactions[0].TxIn[0].ValueIn++ so that the test can't possibly create any false positives if the actual value in somehow ultimately ends up being the magic number.

davecgh · 2018-02-24T17:42:46Z

There is already a block b56 below that this will collide with.

davecgh · 2018-02-24T17:45:03Z

The header comment already says this. Not needed.

davecgh · 2018-02-24T17:48:18Z

b52 can't be changed like this or it will cause the b52a test to not test the intended thing. More generally, you can't reuse any of the same names. I think I'll add an assertion to NextBlock to causes duplicates to fail.

davecgh · 2018-02-25T00:30:26Z

Some additional details:
List of tests removed from validate_test.go which were already covered by the existing full block tests:

- ErrBadMerkleRoot 1
- ErrUnexpectedDifficulty
- ErrHighHash
- ErrBadCoinbaseValue
- ErrFirstTxNotCoinbase
- ErrBadCoinbaseFraudProof    ***NOTE: Neither the removed nor existing covers the null block height case and this should be added.***
- ErrRegTxInStakeTree
- ErrStakeTxInRegularTree
- ErrBadStakebaseScriptLen
- ErrBadStakebaseScrVal

davecgh · 2018-02-24T23:28:43Z

davecgh · 2018-02-24T23:31:32Z

davecgh · 2018-02-24T23:39:48Z

davecgh · 2018-02-25T00:41:46Z

s/with a revocations count mismatch/with a header that commits to more revocations than the block actually contains./

davecgh · 2018-02-25T00:46:12Z

This works, but I think a better test would be to actually replace the entire script with a completely valid script that votes on bs14 (the block before the one it should be voting on which is bs14).

That way it will ensure it doesn't allow voting on the wrong block even when the block it's voting on is actually valid.

davecgh · 2018-02-25T00:49:07Z

Technically there is no need to do this. Blocks coming from the generator already vote yes by default. However, I guess it can't hurt to be explicit.

davecgh · 2018-02-25T00:52:28Z

for i := 0; i < 3; i++ { g.ReplaceVoteBitsN(i, voteBitNo)(b) } for i := 3; i < 5; i++ { g.ReplaceVoteBitsN(i, voteBitYes)(b) }

The second for loop isn't even needed since they're already yes.

davecgh · 2018-02-25T00:53:38Z

for i := 0; i < 2; i++ { g.ReplaceVoteBitsN(i, voteBitNo)(b) } for i := 2; i < 5; i++ { g.ReplaceVoteBitsN(i, voteBitYes)(b) }

The second for loop isn't even needed since they're already yes.

davecgh · 2018-02-25T00:56:08Z

The arrows on the next line should start after the parens like so:

// ... -> bf1(0) -> bf2(1) -> bf5(2) -> bf6(3) // \-> bpw1(4)

The reason is because then it can be copied and extended like so with the arrows still lining up:

// ... -> bf1(0) -> bf2(1) -> bf5(2) -> bf6(3) -> somethingelse.... // \-> bpw1(4)

davecgh · 2018-02-25T01:06:14Z

davecgh · 2018-02-25T01:08:14Z

davecgh · 2018-02-25T01:11:55Z

bpw2 and bpw3

davecgh · 2018-02-25T01:12:48Z

space just before (bpw4 added last)

davecgh · 2018-02-25T01:15:54Z

s/bdc1/bdc2/

davecgh · 2018-02-25T01:18:56Z

s/bdc5/bdc4/
s/bdc6/bdc5/

davecgh · 2018-02-25T01:19:54Z

first bdc5 should be bdc4

davecgh · 2018-02-25T01:23:49Z

Exceeds 80.

// Create block with a header that commits to more revocations than the // block actually contains..

davecgh · 2018-02-25T01:26:57Z

This isn't what I meant. It's a valid vote, but it's consuming a a ticket that is already spent and this is dependent on the order of the checks in the code. Go ahead and revert it to ^= 0x55 for now. I'll have to write some code to expose creating a vote script on an invalid block.

davecgh · 2018-02-25T02:04:33Z

Some additional details:
List of tests removed from validate_test.go which were not already covered by the existing full block tests and were ported:

- ErrBadMerkleRoot 2
- ErrWrongBlockSize
- ErrBadCoinbaseAmountIn
- ErrBadStakebaseAmountIn
- ErrRevocationsMismatch
- ErrVotesOnWrongBlock
- ErrIncongruentVotebit 1
- ErrIncongruentVotebit 2
- ErrIncongruentVotebit 3
- ErrIncongruentVotebit 4

dnldd force-pushed the update_fullblocktests_one_of_x branch 2 times, most recently from df11353 to 68a4456 Compare February 20, 2018 21:04

davecgh requested changes Feb 21, 2018

View reviewed changes

dnldd force-pushed the update_fullblocktests_one_of_x branch 3 times, most recently from a4fafa1 to b1c604c Compare February 24, 2018 16:30

davecgh requested changes Feb 24, 2018

View reviewed changes

dnldd force-pushed the update_fullblocktests_one_of_x branch 5 times, most recently from 07f122a to 4b3abad Compare February 25, 2018 00:30

davecgh requested changes Feb 25, 2018

View reviewed changes

dnldd force-pushed the update_fullblocktests_one_of_x branch from 4b3abad to 7fd2c6f Compare February 25, 2018 01:15

davecgh requested changes Feb 25, 2018

View reviewed changes

dnldd force-pushed the update_fullblocktests_one_of_x branch 2 times, most recently from 12d9c29 to 550ebee Compare February 25, 2018 01:36

davecgh requested changes Feb 25, 2018

View reviewed changes

blockchain: move block validation rule tests into fullblocktests.

6da8225

dnldd force-pushed the update_fullblocktests_one_of_x branch from 550ebee to 6da8225 Compare February 25, 2018 02:04

davecgh approved these changes Feb 25, 2018

View reviewed changes

davecgh merged commit 6da8225 into decred:master Feb 25, 2018

dnldd deleted the update_fullblocktests_one_of_x branch February 25, 2018 02:23

This was referenced Feb 25, 2018

chaingen: Prevent dup block names in NextBlock. #1079

Merged

All rules tested in the blockchain package TestBlockValidationRules should be done via fullblocktests #1030

Closed

davecgh mentioned this pull request May 2, 2018

Block validation rules impossible error paths refactor #1182

Open

Conversation

dnldd commented Feb 20, 2018 • edited by davecgh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davecgh left a comment

Choose a reason for hiding this comment

Uh oh!

davecgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecgh commented Feb 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davecgh commented Feb 25, 2018

Uh oh!

Reviewers

Assignees

dnldd commented Feb 20, 2018 •

edited by davecgh

Loading