Block validation rules impossible error paths refactor

[davecgh]

PRs #1060, #1095, and #1141 resolved issue #1030 by porting all of the block validation consensus rule tests which previously were in the blockchain package test function named TestBlockValidationRules to the newer fullblocktests framework which programmatically generates a fully valid chain on the fly and only munges the blocks to cause a very specific condition to be tested while ensuring the block is otherwise entirely valid.

However, the TestBlockValidationRules function also contained several comments regarding error paths which are not possible to hit.

I'm copying those comments here as ideally the code should be refactored to avoid having impossible to hit error code paths unless they are assertions.

-	// ----------------------------------------------------------------------------
-	// ErrStakeBelowMinimum still needs to be tested, can't on this blockchain
-	// because it's above minimum and it'll always trigger failure on that
-	// condition first.

-	// ----------------------------------------------------------------------------
-	// ErrInvalidSSGenInput
-	// It doesn't look like this one can actually be hit since checking if
-	// IsSSGen should fail first.

-	// ----------------------------------------------------------------------------
-	// ErrSSGenSubsidy
-	// It appears that ErrSSGenSubsidy is impossible to hit due to the
-	// check above that returns ErrSSGenPayeeOuts.

-	// ----------------------------------------------------------------------------
-	// ErrSStxInImmature
-	// This is impossible to hit from a block's perspective because the
-	// ticket isn't in the ticket database. So it fails prematurely.

-	// ----------------------------------------------------------------------------
-	// ErrSStxInScrType
-	// The testbed blockchain doesn't have any non-P2PKH or non-P2SH outputs
-	// so we can't test this. Independently tested and verified, but should
-	// eventually get its own unit test.

-	// ----------------------------------------------------------------------------
-	// ErrInvalidSSRtxInput
-	// It seems impossible to hit this from a block test because it fails when
-	// it can't detect the relevant tickets in the missed ticket database
-	// bucket.

-	// ----------------------------------------------------------------------------
-	// ErrBadStakebaseValue doesn't seem be be able to be hit because
-	// ErrSSGenPayeeOuts is hit first. The code should be kept in in case
-	// the first check somehow fails to catch inflation.

-	// ----------------------------------------------------------------------------
-	// ErrStakeFees
-	// It should be impossible for this to ever be triggered because of the
-	// paranoid around transaction inflation, but leave it in anyway just
-	// in case there is database corruption etc.

[davecgh]

See the comments on PR #1306 which attempted to remove some of these for more details, however, an important takeaway is that these comments which were in the tests are not necessarily accurate. A full analysis of each case needs to be done rather than assuming they are correct.

[dnldd]

Having another go at this.

[AnNiran-zz]

I have started to work on this last few days ago.

On Thu, Jul 9, 2020 at 1:21 AM Donald Adu-Poku [email protected] wrote:

Having another go at this.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/decred/dcrd/issues/1182#issuecomment-655839848, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEOKU7JWADUYSVZ6L32RZFTR2ULSDANCNFSM4E6AAX3Q .

[dnldd]

@AnNiran alright, I'll post my findings to start the discussion on what error codes should stay or get removed, will leave any removals to you. @davecgh please assert the following when you can, thanks.

• ErrStakeBelowMinimum: This is already covered by block bsd1 in the full block tests.

• ErrInvalidSSGenInput: This was renamed to ErrInvalidVoteInput in #1468 and is impossible to hit because isNullOutpoint currently (erroneously?) classifies outpoints from the regular tree as null outpoints. ErrBadTxInput gets triggered when the vote input is changed to prevOut on the regular tree. This should not be removed since its call sites assert the expected relationship between a ticket and its associated vote with regards to the index position of the first output of the ticket.

• ErrSStxInImmature: This was renamed to ErrImmatureTicketSpend in #1468 and is impossible to hit without being able to set a ticket as a winning ticket since those are the only tickets that's can be used to generate a vote. This should not be removed since its call sites asserting the stake output can be spent.

• ErrSStxInScrType: This was renamed in #1468 to ErrTicketInputScript, This is currently impossible to hit because setting a bad script triggers ErrRegTxCreateStakeOut, yet to conclude on the reason why since its a stake tx's first output pk script being modified in the test. This should not be removed however since it asserts the expected inputs for a ticket.

• ErrInvalidSSRtxInput: This was renamed to ErrInvalidRevokeInput in #1468 and is currently impossible to hit because ErrRegTxCreateStakeOut already accounts for using non-stake outputs in creating a stake transaction. This should not be removed since its call sites assert the expected relationship for a revocation's input.

• ErrBadStakebaseValue: The test for this is currently failing and returning ErrBadCoinbaseValue even though a stake tree value is getting modified in the test. Yet to find out why.

• ErrStakeFees: This currently is impossible to hit because to increase fees of of a vote for example the stake base amount or the ticket value will have to be increased. ErrBadStakebaseAmountIn gets triggered when the stake base is increased while ErrFraudAmountIn is triggered when the ticket value is increased. This should not be removed since its call sites assert the expected behaviour of stake fees being the remaining amount when outputs are subtracted from the stake inputs.

[blxtm]

@dnldd I started to analyze the errors and the stake logic more thoroughly a week ago and understood parts of what you are saying. Thank you for sharing this, it is helpful for me to match with someone else's findings.

I am starting with working with the code and I am much slower than rest of the people, so it takes me longer to realize some of the aspects.

[blxtm]

Apologies for not being active on this issue; I have managed to find time to work on it now and in the future on others.