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

Simplify V2FileContractRenewal #238

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Simplify V2FileContractRenewal #238

merged 4 commits into from
Dec 3, 2024

Conversation

lukechampine
Copy link
Member

The tradeoff is that resolution transactions can't contain other outputs. That doesn't seem too restrictive to me, but maybe I'm forgetting a use case that requires it?

I also noticed that a renewal's FinalRevision wasn't being applied to the accumulator that way; instead, the accumulator would contain the most recent revision of the contract. Not a huge deal, just felt inconsistent.

@n8maninger
Copy link
Member

n8maninger commented Nov 29, 2024

This looks like it will make rhp4 require setup transactions again for partial rollovers, definitely not worth it imo

@n8maninger
Copy link
Member

#209

@lukechampine
Copy link
Member Author

ah. right. drat

@lukechampine lukechampine changed the title consensus: Allow rollover to pay for miner fee consensus: Insert final renewal revision into accumulator Nov 30, 2024
@lukechampine lukechampine changed the title consensus: Insert final renewal revision into accumulator Simplify V2FileContractRenewal Dec 2, 2024
Unlike the FinalRevision, there is no danger of allowing the new contract
to be independently valid, as it cannot be broadcast without also being
funded. Note that this is already possible with any other v2 contract
formation: anyone can "replay" the contract formation in a transaction of
their own, but they need to supply the contract funding themselves.
Instead of a full revision, renewals now specify just the final outputs
and rollover amounts. This sidesteps existing inconsistencies around
whether the "final revision" should be inserted into the accumulator or
not, as well as the issue of the revision being valid in a standalone
transaction.
@lukechampine
Copy link
Member Author

This PR now makes the following changes:

  • The new contract created by a renewal must have valid signatures. This preserves the invariant that all contracts in the accumulator have valid signatures.
  • The "final revision" in a renewal has been replaced by just the final payouts (and rollover amounts). This reflects the fact that this final revision is not actually inserted in to the accumulator, nor are its signatures valid, nor does it appear as a rev in ForEachV2FileContractElement -- in other words, it's not a "real" revision at all.

I believe it's possible for the rollover values to be omitted entirely: the rollover amount can be determined by subtracting the payout values from the original contract value. However, this would require changes to RHPv4, since helpers like RefreshCost assume that the renter's portion of the rollover can be distinguished from the host's portion. Personally I would prefer to change RHPv4 to pass the rollover values around explicitly, so that they could be removed from V2FileContractRenewal; but I recognize that, like Capacity, it's pretty convenient to have the fields in the renewal itself.

@n8maninger n8maninger merged commit 2e629a0 into master Dec 3, 2024
9 checks passed
@n8maninger n8maninger deleted the renewal-revision branch December 3, 2024 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants