blockchain: move block validation rule tests into fullblocktests (2/x). #1095

dnldd · 2018-02-27T22:44:22Z

This PR moves the following block validation rule tests to
fullblocktests.

ErrBlockOneOutputs
- Create a premine block with one premine output removed.
- Create a premine block with a bad spend script.
- Create a premine block with an incorrect pay to amount.
ErrVotesMismatch
- Create block with a header that commits to more votes than the block
  actually contains.
ErrIncongruentVotebit
- Attempt to add block with incorrect votebits set.
  - 4x Voters, 2x Nay 2x Yea, but block header says Yea
  - 3x Voters, 2x Nay 1x Yea, but block header says Yea
  - 3x Voters, 1x Nay 2x Yea, but block header says Nay
ErrSStxCommitment
- Attempt to add block with a bad ticket purchase commitment.
ErrSSGenPayeeOuts
- Attempt to add a block with a bad vote payee output.
- Attempt to add a block with an incorrect vote payee output amount.
ErrBadCoinbaseFraudProof
- Create block with an invalid coinbase transaction.
ErrFraudBlockIndex
- Create block with an invalid transaction block height.
ErrFraudAmountIn
- Create block with an invalid transaction amount.
ErrExpiredTx
- Create block with an expired transaction.
ErrBadBlockHeight
- Create block with an invalid block height.
ErrPoolSize
- Create block with an invalid ticket pool size.
ErrInvalidFinalState
- Create block with an invalid final state.
ErrScriptMalformed
- Create block with a malformed spend script.

Work towards #1030.

davecgh

Just a quick comment. I'll need to dedicate some time to do a thorough review here.

davecgh · 2018-02-27T22:57:18Z

blockchain/fullblocktests/generate.go

@@ -484,11 +484,40 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 	g.CreatePremineBlock("bpbad0", 1)
 	rejected(blockchain.ErrBadCoinbaseValue)

+	// Create a premine block with one premine output removed.
+	//
+	//   genesis -> bp1


There are bad blocks. They shouldn't be depicted as being added at the tip of the best chain. Also, I'd suggest either keeping with the bpbad# naming, or change bpbad0 to just bp0 for consistency. I think bpbad is better so that the final good block can be bp and thus not need to change the rest of the comments later one.

// genesis // \-> bp1

davecgh · 2018-02-28T05:27:57Z

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

- ErrBlockOneOutputs 2
- ErrBlockOneOutputs 3
- ErrBlockOneOutputs 4
- ErrBadCoinbaseFraudProof
- ErrVotesMismatch
- ErrIncongruentVotebit 5
- ErrIncongruentVotebit 6
- ErrIncongruentVotebit 7
- ErrSStxCommitment
- ErrSSGenPayeeOuts 1
- ErrSSGenPayeeOuts 2
- ErrInvalidFinalState
- ErrPoolSize
- ErrBadBlockHeight
- ErrExpiredTx
- ErrFraudAmountIn
- ErrFraudBlockHeight
- ErrFraudBlockIndex
- ErrScriptMalformed

davecgh

Nice job overall. I thoroughly reviewed every added test and logic wise, one of the tests is invalid, and one of them should be more explicit in the number chosen.

The rest of the requested changes are comment and position related.

davecgh · 2018-02-28T05:34:06Z

blockchain/fullblocktests/generate.go

@@ -1197,6 +1227,72 @@ func Generate(includeLargeReorg bool) (tests [][]TestInstance, err error) {
 	})
 	rejected(blockchain.ErrIncongruentVotebit)

+	// Create block with a header that commits to more votes


Comment does not match the test. The header is committing to less votes than the block actually contains.

davecgh · 2018-02-28T05:37:52Z

blockchain/fullblocktests/generate.go

+
+	// Attempt to add block with incorrect votebits set.
+	// 4x Voters
+	// 2x Nay 2x Yea, but block header says Yea


Please keep the Yes/No consistent for all tests.

davecgh · 2018-02-28T05:41:59Z

blockchain/fullblocktests/generate.go

+	//   ... -> bsl5(8)
+	//                 \-> bv15(9)
+	g.SetTip("bsl5")
+	g.NextBlock("bv15", outs[9], ticketOuts[9], g.ReplaceWithNVotes(3), func(b *wire.MsgBlock) {


Why replace with 3 votes?

davecgh · 2018-02-28T05:43:17Z

blockchain/fullblocktests/generate.go

+	//                 \-> bv15(9)
+	g.SetTip("bsl5")
+	g.NextBlock("bv15", outs[9], ticketOuts[9], g.ReplaceWithNVotes(3), func(b *wire.MsgBlock) {
+		b.STransactions[5].TxOut[1].PkScript = chaingen.VoteCommitmentScript(g.Tip().BlockHash(), 1)


This is not correct. It's not a vote and the commitment is different for a ticket purchase. You should export purchaseCommitmentScript from chaingen and use that instead.

Also, this is way over 80 cols.

davecgh · 2018-02-28T05:58:38Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrBadCoinbaseFraudProof)
+
+	// Create block with an invalid transaction block index.


with a regular transaction that commits to an invalid block index.

davecgh · 2018-02-28T06:09:32Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrFraudBlockHeight)
+
+	// Create block with an invalid transaction amount.


with a regular transaction that commits to an invalid input amount.

davecgh · 2018-02-28T06:16:15Z

blockchain/fullblocktests/generate.go

+	g.NextBlock("bmf21", outs[15], ticketOuts[15], func(b *wire.MsgBlock) {
+		txOut := chaingen.MakeSpendableOut(b, 1, 0)
+		tx := g.CreateSpendTx(&txOut, lowFee)
+		tx.Expiry = 20


This should be 1 so it is the exact expiration period of the transaction itself to help detect off by ones in the code. There should also ideally be another test added that sets the expiry of b.Transactions[1] to exactly one more than the height at which it is included to ensure it is accepted. Obviously that would cause another accepted block and thus can't go here, but such a test should be added.

davecgh · 2018-02-28T06:17:24Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrExpiredTx)
+
+	// Create block with an invalid block height.


that commits to an invalid height.

davecgh · 2018-02-28T06:17:34Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrBadBlockHeight)
+
+	// Create block with an invalid ticket pool size.


that commits to an invalid ticket pool size.

davecgh · 2018-02-28T06:17:52Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrPoolSize)
+
+	// Create block with an invalid final state.


that commits to an invalid final state.

davecgh · 2018-02-28T17:38:30Z

blockchain/fullblocktests/generate.go

+	})
+	rejected(blockchain.ErrBadBlockHeight)
+
+	// Create that commits to an invalid ticket pool size.


This PR moves the following block validation rule tests to fullblocktests. - `ErrBlockOneOutputs` - Create a premine block with one premine output removed. - Create a premine block with a bad spend script. - Create a premine block with an incorrect pay to amount. - `ErrVotesMismatch` - Create block with a header that commits to more votes than the block actually contains. - `ErrIncongruentVotebit` - Attempt to add block with incorrect votebits set. - 4x Voters, 2x Nay 2x Yea, but block header says Yea - 3x Voters, 2x Nay 1x Yea, but block header says Yea - 3x Voters, 1x Nay 2x Yea, but block header says Nay - `ErrSStxCommitment` - Attempt to add block with a bad ticket purchase commitment. - `ErrSSGenPayeeOuts` - Attempt to add a block with a bad vote payee output. - Attempt to add a block with an incorrect vote payee output amount. - `ErrBadCoinbaseFraudProof` - Create block with an invalid coinbase transaction. - `ErrFraudBlockIndex` - Create block with an invalid transaction block height. - `ErrFraudAmountIn` - Create block with an invalid transaction amount. - `ErrExpiredTx` - Create block with an expired transaction. - `ErrBadBlockHeight` - Create block with an invalid block height. - `ErrPoolSize` - Create block with an invalid ticket pool size. - `ErrInvalidFinalState` - Create block with an invalid final state. - `ErrScriptMalformed` - Create block with a malformed spend script.

davecgh requested changes Feb 27, 2018

View reviewed changes

davecgh requested changes Feb 28, 2018

View reviewed changes

dnldd force-pushed the fullblocktests-two-of-x branch from e8dfb10 to 282f233 Compare February 28, 2018 14:06

davecgh requested changes Feb 28, 2018

View reviewed changes

dnldd force-pushed the fullblocktests-two-of-x branch 4 times, most recently from cdfb253 to d9d6f60 Compare March 1, 2018 00:50

dnldd force-pushed the fullblocktests-two-of-x branch from d9d6f60 to 84820a0 Compare March 1, 2018 01:03

davecgh approved these changes Mar 1, 2018

View reviewed changes

davecgh merged commit 60c7bc0 into decred:master Mar 1, 2018

davecgh mentioned this pull request Mar 2, 2018

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

Closed

dnldd deleted the fullblocktests-two-of-x branch March 7, 2018 11:48

davecgh mentioned this pull request May 2, 2018

Block validation rules impossible error paths refactor #1182

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockchain: move block validation rule tests into fullblocktests (2/x). #1095

blockchain: move block validation rule tests into fullblocktests (2/x). #1095

dnldd commented Feb 27, 2018 •

edited by davecgh

Loading

davecgh left a comment

davecgh Feb 27, 2018

davecgh commented Feb 28, 2018 •

edited

Loading

davecgh left a comment

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

davecgh Feb 28, 2018

blockchain: move block validation rule tests into fullblocktests (2/x). #1095

blockchain: move block validation rule tests into fullblocktests (2/x). #1095

Conversation

dnldd commented Feb 27, 2018 • edited by davecgh Loading

davecgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davecgh commented Feb 28, 2018 • edited Loading

davecgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnldd commented Feb 27, 2018 •

edited by davecgh

Loading

davecgh commented Feb 28, 2018 •

edited

Loading