Skip to content

fullblocktests: Add ErrInvalidRevokeInput test. - #3786

Open
matthawkins90 wants to merge 1 commit into
decred:masterfrom
matthawkins90:fullblocktests_stake_input_index
Open

fullblocktests: Add ErrInvalidRevokeInput test.#3786
matthawkins90 wants to merge 1 commit into
decred:masterfrom
matthawkins90:fullblocktests_stake_input_index

Conversation

@matthawkins90

@matthawkins90 matthawkins90 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

This is work towards #1182.

This adds full block test coverage for ErrInvalidRevokeInput (which used to be named ErrInvalidSSRtxInput before #1468). The new brt7 block contains a revocation whose ticket input references the ticket change output (index 2) instead of the submission output (index 0).

Issue #1182 quotes an old TestBlockValidationRules comment that called this error impossible to hit:

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

#1307 proposed to remove the check, but Dave wrote:

while it is true that the error shouldn't be possible to hit from a block, due to the fact that an earlier check will fail on the referenced output not being in the set of missed tickets, removing this would incorrectly allow the mempool to accept revocations that don't actually reference tickets since it relies on the CheckTransactionInputs function to detect this condition and return an error accordingly.

That earlier check (checkTicketRedeemers) matches missed tickets by hash only. A revocation with the right hash and a wrong index passes it and reaches the wrong-index check in checkRevocationInputs.

Further analysis in the issue thread considered a revocation that references non-stake outputs, which other checks catch.

Notes:

  • The mutation targets the change output because the commitment output (index 1) is OP_RETURN and never in the utxo set, so referencing it fails earlier in countP2SHSigOps with ErrMissingTxOut.
  • The block is otherwise fully valid. With the mutation disabled, the harness accepts it.
  • The previous terminal block brt7 becomes brt8. This follows the convention from chaingen/fullblocktests: Add disapproval tests. #1485, which renamed brtfinal to brt7 when it extended the series: the accepted block that ends the series keeps the highest number.
  • The test exercises the legacy (pre-DCP0009) eligibility path (existsMissedTicket), because fullblocktests does not activate the automatic revocations agenda.
  • No consensus code changes. blockchain: remove impossible to hit validation errors. #1306 and blockchain: remove impossible to hit ErrInvalidSSRtxInput. #1307 were closed because removal would change consensus and break mempool-side validation. This PR only adds coverage.

A note on the companion kind ErrInvalidVoteInput (named ErrInvalidSSGenInput before #1468): it is not reachable from a block, so no test is added for it. CheckSSGenVotes rejects any nonzero ticket input index during classification (ErrSSGenWrongIndex), so the chain never classifies such a transaction as a vote and checkVoteInputs never runs. A trial block failed at sanity with ErrBadTxInput, which matches the result reported in the issue thread.

This adds a test which ensures the chain rejects a block when a
revocation references a ticket output other than the submission
output.

The test mutates the revocation to reference the ticket change output.
The change output still exists in the utxo set, so the input existence
checks pass and the index check itself rejects the block.

This also renames brt7 to brt8 so the accepted block that ends the
revocation series keeps the highest number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant