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 validation errors. #1306

Closed
wants to merge 1 commit into from

Conversation

dnldd
Copy link
Member

@dnldd dnldd commented Jun 20, 2018

This removes ErrSSGenSubsidy and its dedicated checkStakeBaseAmounts, as well as ErrInvalidSSGenInput. ErrSSGenSubsidy was removed because its triggering conditions are a subset of ErrSSGenPayeeOuts which is checked earlier in CheckTransactionInputs. ErrInvalidSSGenInput was removed because its triggering conditions are checked earlier by IsSSGen in CheckTransactionInputs.

This is work towards #1182.

This removes ErrSSGenSubsidy and its dedicated checkStakeBaseAmounts, as well as ErrInvalidSSGenInput.  ErrSSGenSubsidy was removed because its triggering conditions are a subset of ErrSSGenPayeeOuts which is checked earlier in CheckTransactionInputs. ErrInvalidSSGenInput was removed because its triggering conditions are checked earlier by IsSSGen in CheckTransactionInputs.
@davecgh davecgh added this to the 1.3.0 milestone Jun 22, 2018
Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct and would be a hard fork. The checks which determine ErrInvalidSSGenInput need to retained.

@davecgh
Copy link
Member

davecgh commented Jul 31, 2018

I'm closing this because in addition to the aforementioned change, the removal of checkStakeBaseAmounts is also not entirely correct since it contains an additional check that the others do not.

Namely, it contains the overall check that the total of all output amounts, including miner fees, less the original ticket price does not exceed the amount allowed to be generated per the subsidy rules. In other words, it ensures that the number of new coins introduced to the system does not exceed the max allowed value based on the block subsidy.

This is slightly different than that other checks which ensure that each individual output does not exceed what they should per the initial ticket purchase amount, calculated subsidy, calculated split amount proportion, and restrictions placed on the pool fee by the ticket. Since it deals with proportions, the final result is not necessarily (and will almost never be in the case of large uneven splits) the exact value which implies the tiny number of atoms leftover that can't be evenly split goes to the miner as a fee.

It is true that bounding each output in this way theoretically implicitly prevents the sum of those outputs from exceeding the overall maximum amount, however, any small consensus change in the future to the way ticket splitting is handled could inadvertently break this rather hidden assumption which could very easily lead to the ability to mint coins out of thin air.

Therefore, I think it's important to keep the independent checks which ensure that transactions which generate coins (both the regular coinbase and votes) have explicit checks to prevent generating more coins than are allowed.

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