Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

blockchain: remove impossible to hit ErrInvalidSSRtxInput. #1307

Closed
wants to merge 1 commit into from

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Jun 20, 2018

ErrInvalidSSRtxInput's triggering condition is a subset of the checks IsSSRtx performs on a transaction. Since IsSSRtx is the first to evaluate a tx in CheckTransactionInputs that should be enough.

This is work towards #1182

ErrInvalidSSRtxInput's triggering condition is a subset of the checks IsSSRtx performs on a transaction. Since IsSSRtx is the first to evaluate a tx in CheckTransactionInputs that should be enough.
@dajohi dajohi requested a review from davecgh June 21, 2018 15:01
@davecgh davecgh added this to the 1.3.0 milestone Jun 22, 2018
@davecgh
Copy link
Member

davecgh commented Aug 1, 2018

I'm closing this because it is not correct for a couple of reasons.

First, the description provided in the PR is incorrect because the check is ensuring the the previous output that is being referenced by the revocation is a ticket, while IsSSRtx only deals with the transaction itself, which is evidenced by the fact the function does not accept a UtxoViewpoint. That means it is impossible for IsSSRtx to perform this check.

Second, 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.

Given that, if these checks were to be removed, they would somehow need to be separated and independently checked from mempool.

@davecgh davecgh closed this Aug 1, 2018
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.

2 participants