-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fix renewal payout validation #242
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very much like the way the various test cases are outlined!
return fmt.Errorf("file contract renewal %v changes renter public key", i) | ||
} else if fc.HostPublicKey != renewal.NewContract.HostPublicKey { | ||
return fmt.Errorf("file contract renewal %v changes host public key", i) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could allow this though, right? I don't think it would be unsafe, since the whole renewal is validated using the old keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, we allow revisions to change the keys. Granted, I don't see a ton of utility there either; key rotation is really not as much of a thing as people assume. So I'd be ok disallowing it in both cases too -- just wanna be consistent.
Add(renewal.FinalHostOutput.Value).Add(renewal.HostRollover) | ||
existingPayout := fc.RenterOutput.Value.Add(fc.HostOutput.Value) | ||
if totalPayout != existingPayout { | ||
return fmt.Errorf("file contract renewal %d renewal payout (%s) does not match existing contract payout %s", i, totalPayout, existingPayout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
error is a bit inconsistent with the others, which use (%d H)
. That might predate #146, so perhaps we should switch them all to %s
now?
Did not notice this until after trying to update coreutils with #238. The new renewal validation logic was checking the individual payout values against the parent file contract element. This does not account for the payouts changing in an unbroadcast revision. This changes the logic to check that the sum of the outputs is equal to the sum of the parent outputs. Also adds some additional validation tests.