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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Fix renewal payout validation #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd19106
consensus: fix renewal payout validation
n8maninger Dec 3, 2024
c435a35
consensus: add additional resolution test cases
n8maninger Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions consensus/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -780,12 +780,24 @@ func validateV2FileContracts(ms *MidState, txn types.V2Transaction) error {
case *types.V2FileContractRenewal:
renewal := *r

if fc.RenterPublicKey != renewal.NewContract.RenterPublicKey {
ChrisSchinnerl marked this conversation as resolved.
Show resolved Hide resolved
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)
}
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could, but why?

Copy link
Member

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.


// validate that the renewal value is equal to existing contract's value.
// This must be done as a sum of the outputs, since the individual payouts may have
// changed in an unbroadcast revision.
totalPayout := renewal.FinalRenterOutput.Value.Add(renewal.RenterRollover).
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)
Copy link
Member

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?

}

newContractCost := renewal.NewContract.RenterOutput.Value.Add(renewal.NewContract.HostOutput.Value).Add(ms.base.V2FileContractTax(renewal.NewContract))
if totalRenter := renewal.FinalRenterOutput.Value.Add(renewal.RenterRollover); totalRenter != fc.RenterOutput.Value {
return fmt.Errorf("file contract renewal %v renter payout plus rollover (%d H) does not match old contract payout (%d H)", i, totalRenter, fc.RenterOutput.Value)
} else if totalHost := renewal.FinalHostOutput.Value.Add(renewal.HostRollover); totalHost != fc.HostOutput.Value {
return fmt.Errorf("file contract renewal %v host payout plus rollover (%d H) does not match old contract payout (%d H)", i, totalHost, fc.HostOutput.Value)
} else if rollover := renewal.RenterRollover.Add(renewal.HostRollover); rollover.Cmp(newContractCost) > 0 {
if rollover := renewal.RenterRollover.Add(renewal.HostRollover); rollover.Cmp(newContractCost) > 0 {
return fmt.Errorf("file contract renewal %v has rollover (%d H) exceeding new contract cost (%d H)", i, rollover, newContractCost)
} else if err := validateContract(renewal.NewContract); err != nil {
return fmt.Errorf("file contract renewal %v initial revision %s", i, err)
Expand Down
84 changes: 84 additions & 0 deletions consensus/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1909,6 +1909,68 @@ func TestV2RenewalResolution(t *testing.T) {
txn.SiacoinOutputs[0].Value = txn.SiacoinInputs[0].Parent.SiacoinOutput.Value.Sub(partial).Sub(cs.V2FileContractTax(renewal.NewContract))
},
},
{
desc: "valid renewal - changed host payout",
renewFn: func(txn *types.V2Transaction) {
// transfers part of the renter payout to the host
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
renewal.FinalHostOutput.Value = renewal.HostRollover
renewal.HostRollover = types.ZeroCurrency
renewal.FinalRenterOutput.Value = renewal.RenterRollover
renewal.RenterRollover = types.ZeroCurrency
partial := renewal.FinalRenterOutput.Value.Div64(2)
renewal.FinalRenterOutput.Value = partial
renewal.FinalHostOutput.Value = renewal.FinalHostOutput.Value.Add(partial)
// subtract the cost from the change output
txn.SiacoinOutputs[0].Value = txn.SiacoinInputs[0].Parent.SiacoinOutput.Value.Sub(renewal.NewContract.RenterOutput.Value).Sub(renewal.NewContract.HostOutput.Value).Sub(cs.V2FileContractTax(renewal.NewContract))
},
},
{
desc: "valid renewal - changed renter payout",
renewFn: func(txn *types.V2Transaction) {
// transfers part of the host payout to the renter
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
renewal.FinalHostOutput.Value = renewal.HostRollover
renewal.HostRollover = types.ZeroCurrency
renewal.FinalRenterOutput.Value = renewal.RenterRollover
renewal.RenterRollover = types.ZeroCurrency
partial := renewal.FinalHostOutput.Value.Div64(2)
renewal.FinalRenterOutput.Value = partial
renewal.FinalRenterOutput.Value = renewal.FinalRenterOutput.Value.Add(partial)
// subtract the cost from the change output
txn.SiacoinOutputs[0].Value = txn.SiacoinInputs[0].Parent.SiacoinOutput.Value.Sub(renewal.NewContract.RenterOutput.Value).Sub(renewal.NewContract.HostOutput.Value).Sub(cs.V2FileContractTax(renewal.NewContract))
},
},
{
desc: "invalid renewal - total payout exceeding parent",
renewFn: func(txn *types.V2Transaction) {
// transfers part of the renter payout to the host
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
renewal.FinalRenterOutput.Value = renewal.FinalRenterOutput.Value.Add(types.Siacoins(1))
},
errString: "does not match existing contract payout",
},
{
desc: "invalid renewal - total payout less than parent",
renewFn: func(txn *types.V2Transaction) {
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
renewal.RenterRollover = renewal.RenterRollover.Sub(types.Siacoins(1))
txn.SiacoinOutputs[0].Value = txn.SiacoinInputs[0].Parent.SiacoinOutput.Value.Sub(types.Siacoins(1)).Sub(cs.V2FileContractTax(renewal.NewContract))
},
errString: "does not match existing contract payout",
},
{
desc: "invalid renewal - total payout less than parent - no rollover",
renewFn: func(txn *types.V2Transaction) {
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
renewal.FinalRenterOutput.Value = renewal.RenterRollover.Sub(types.Siacoins(1))
renewal.FinalHostOutput.Value = renewal.HostRollover
renewal.RenterRollover = types.ZeroCurrency
renewal.HostRollover = types.ZeroCurrency
txn.SiacoinOutputs[0].Value = txn.SiacoinInputs[0].Parent.SiacoinOutput.Value.Sub(renewal.FinalRenterOutput.Value).Sub(renewal.FinalHostOutput.Value).Sub(cs.V2FileContractTax(renewal.NewContract))
},
errString: "siacoin inputs (1000000000000000000000000000 H) do not equal outputs (1001000000000000000000000000 H)", // this is an inputs != outputs error because the renewal is validated there first
},
{
desc: "invalid renewal - bad new contract renter signature",
renewFn: func(txn *types.V2Transaction) {
Expand All @@ -1925,6 +1987,28 @@ func TestV2RenewalResolution(t *testing.T) {
},
errString: "invalid host signature",
},
{
desc: "invalid renewal - different host key",
renewFn: func(txn *types.V2Transaction) {
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
sk := types.GeneratePrivateKey()
renewal.NewContract.HostPublicKey = sk.PublicKey()
contractSigHash := cs.ContractSigHash(renewal.NewContract)
renewal.NewContract.HostSignature = sk.SignHash(contractSigHash)
},
errString: "changes host public key",
},
{
desc: "invalid renewal - different renter key",
renewFn: func(txn *types.V2Transaction) {
renewal := txn.FileContractResolutions[0].Resolution.(*types.V2FileContractRenewal)
sk := types.GeneratePrivateKey()
renewal.NewContract.RenterPublicKey = sk.PublicKey()
contractSigHash := cs.ContractSigHash(renewal.NewContract)
renewal.NewContract.RenterSignature = sk.SignHash(contractSigHash)
},
errString: "changes renter public key",
},
{
desc: "invalid renewal - not enough host funds",
renewFn: func(txn *types.V2Transaction) {
Expand Down
4 changes: 4 additions & 0 deletions rhp/v4/encoding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,10 +263,12 @@ func (r *RPCRenewContractResponse) maxLen() int {

func (r *RPCRenewContractSecondResponse) encodeTo(e *types.Encoder) {
r.RenterRenewalSignature.EncodeTo(e)
r.RenterContractSignature.EncodeTo(e)
types.EncodeSlice(e, r.RenterSatisfiedPolicies)
}
func (r *RPCRenewContractSecondResponse) decodeFrom(d *types.Decoder) {
r.RenterRenewalSignature.DecodeFrom(d)
r.RenterContractSignature.DecodeFrom(d)
types.DecodeSlice(d, &r.RenterSatisfiedPolicies)
}
func (r *RPCRenewContractSecondResponse) maxLen() int {
Expand Down Expand Up @@ -331,10 +333,12 @@ func (r *RPCRefreshContractResponse) maxLen() int {

func (r *RPCRefreshContractSecondResponse) encodeTo(e *types.Encoder) {
r.RenterRenewalSignature.EncodeTo(e)
r.RenterContractSignature.EncodeTo(e)
types.EncodeSlice(e, r.RenterSatisfiedPolicies)
}
func (r *RPCRefreshContractSecondResponse) decodeFrom(d *types.Decoder) {
r.RenterRenewalSignature.DecodeFrom(d)
r.RenterContractSignature.DecodeFrom(d)
types.DecodeSlice(d, &r.RenterSatisfiedPolicies)
}
func (r *RPCRefreshContractSecondResponse) maxLen() int {
Expand Down
2 changes: 2 additions & 0 deletions rhp/v4/rhp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ type (
// RPCRefreshContractSecondResponse implements Object.
RPCRefreshContractSecondResponse struct {
RenterRenewalSignature types.Signature `json:"renterRenewalSignature"`
RenterContractSignature types.Signature `json:"renterContractSignature"`
RenterSatisfiedPolicies []types.SatisfiedPolicy `json:"renterSatisfiedPolicies"`
}
// RPCRefreshContractThirdResponse implements Object.
Expand Down Expand Up @@ -344,6 +345,7 @@ type (
// RPCRenewContractSecondResponse implements Object.
RPCRenewContractSecondResponse struct {
RenterRenewalSignature types.Signature `json:"renterRenewalSignature"`
RenterContractSignature types.Signature `json:"renterContractSignature"`
ChrisSchinnerl marked this conversation as resolved.
Show resolved Hide resolved
RenterSatisfiedPolicies []types.SatisfiedPolicy `json:"renterSatisfiedPolicies"`
}
// RPCRenewContractThirdResponse implements Object.
Expand Down
Loading