Skip to content

Block validation rules impossible error paths refactor #1182

Open
Open
Block validation rules impossible error paths refactor#1182
Labels
non-forking consensusChanges that involve modifying consensus code without causing any forking changes.

Description

@davecgh
davecgh
opened on May 2, 2018

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.

Activity

  1. davecgh commented on Jul 31, 2018

    @davecgh
    davecgh
    on Jul 31, 2018
    MemberAuthor

    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.

  2. davecgh
    added
    non-forking consensusChanges that involve modifying consensus code without causing any forking changes.
    on Dec 28, 2019

  3. dnldd commented on Jul 9, 2020

    @dnldd
    dnldd
    on Jul 9, 2020
    Member

    Having another go at this.

  4. AnNiran-zz commented on Jul 10, 2020

    @AnNiran-zz
    AnNiran-zz
    on Jul 10, 2020 via email

  5. dnldd commented on Jul 10, 2020

    @dnldd
    dnldd
    on Jul 10, 2020
    Member

    @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 multi: Cleanup and optimize tx input check code. #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 multi: Cleanup and optimize tx input check code. #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 multi: Cleanup and optimize tx input check code. #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 multi: Cleanup and optimize tx input check code. #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.

  6. blxtm commented on Jul 10, 2020

    @blxtm
    blxtm
    on Jul 10, 2020

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

  7. blxtm commented on Nov 12, 2020

    @blxtm
    blxtm
    on Nov 12, 2020

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    non-forking consensusChanges that involve modifying consensus code without causing any forking changes.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions