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

fix(consensus): Allow v1 contracts to be resolved immediately #258

Merged
merged 5 commits into from
Dec 18, 2024

Conversation

lukechampine
Copy link
Member

@lukechampine lukechampine commented Dec 16, 2024

After careful testing, I found that siad does allow a contract to be resolved within the same block it was created. In short:

  • Contracts must have a WindowStart of at least currentHeight+1
  • Storage proofs check that currentHeight is not greater than the "trigger height," which is WindowStart - 1
  • Therefore, if you create a contract with WindowStart = currentHeight + 1, its trigger height will be currentHeight, so a storage proof is allowed.

So in fact, you can create, revise, and resolve a v1 contract all in the same block! (On that note, the update docstrings are more descriptive now.)

Note that v2 contracts do NOT share this property. There is no such thing as an ephemeral v2 contract; that is, you can't use types.UnassignedLeafIndex when referencing a v2 contract. (We could support this, but I see little reason to.)

Lastly, I improved the validation tests by checking for specific error strings, and switched the consensus errors to use %v formatting for SC amounts, rather than Hastings.

@mike76-dev
Copy link
Contributor

Yeah, I found it a very helpful "feature" while testing Sia-Satellite. I could form a contract and renew it straight away, without waiting.

consensus/validation.go Outdated Show resolved Hide resolved
@lukechampine
Copy link
Member Author

Yeah, I found it a very helpful "feature" while testing Sia-Satellite. I could form a contract and renew it straight away, without waiting.

Hmm. Perhaps we should allow "ephemeral" contracts in v2 as well, then. It would be a relatively minor change:

--- a/consensus/validation.go
+++ b/consensus/validation.go
@@ -684,6 +684,10 @@ func validateV2FileContracts(ms *MidState, txn types.V2Transaction) error {
                        return fmt.Errorf("has already been revised by contract revision %v", i)
                } else if i, ok := resolved[fce.ID]; ok {
                        return fmt.Errorf("has already been resolved by contract resolution %v", i)
+               } else if fce.StateElement.LeafIndex == types.UnassignedLeafIndex {
+                       if !ms.isCreated(fce.ID) {
+                               return errors.New("is not present in a prior transaction within this block")
+                       }
                } else if !ms.base.Elements.containsUnresolvedV2FileContractElement(fce) {
                        if ms.base.Elements.containsResolvedV2FileContractElement(fce) {
                                return errors.New("has already been resolved in a previous block")

What do you think @n8maninger @ChrisSchinnerl?

@n8maninger
Copy link
Member

n8maninger commented Dec 16, 2024

I prefer the v2 behavior. It feels more correct to me, but I agree it doesn't look too bad.

@lukechampine
Copy link
Member Author

I suppose it's another consistency thing: if we allow siacoins and siafunds to be ephemeral, why not file contracts as well? Or: if we don't allow it for file contracts, why do we allow it for siacoins/siafunds?

@lukechampine lukechampine changed the title Allow v1 contracts to be resolved immediately fix(consensus): Allow v1 contracts to be resolved immediately Dec 16, 2024
@n8maninger
Copy link
Member

I suppose that is true

@n8maninger
Copy link
Member

n8maninger commented Dec 17, 2024

if we allow siacoins and siafunds to be ephemeral, why not file contracts as well?

Having thought about this some more I disagree with this logic since contract payouts are time locked and siacoin/siafund UTXOs are not. Resolving in the same block isn't a beneficial use case. Add in complications from rollover and I'm unsure its behavior we really should support.

Doesn't V2 use ProofHeight for the entropy directly, instead of WindowStart-1? I may be wrong, but would adding EphemeralLeafIndex to V2Contracts be enough since ProofHeight would still be required to be greater than childHeight?

consensus/validation.go Outdated Show resolved Hide resolved
consensus/validation_test.go Show resolved Hide resolved
@ChrisSchinnerl
Copy link
Member

I don't think there is any value in allowing file contracts to be ephemeral and "why not to be consistent" doesn't seem worthwhile to me. Contracts are meant for storing data over time and "ephemeral" means "no time" in this case. Uploading to a contract after a host knows the entropy for the storage proof is also not desirable.

@lukechampine
Copy link
Member Author

I disagree with this logic since contract payouts are time locked and siacoin/siafund UTXOs are not.

The payouts are still timelocked; this is about the contract itself. It's the same as how siafund outputs can be ephemeral, even though a siafund input creates a timelocked claim.

Uploading to a contract after a host knows the entropy for the storage proof is also not desirable.

That thought did cross my mind. The host could immediately throw away all segments except the selected one. On the other hand, the renter would have to be behaving really strangely in the first place to form a 0-block contract, so I feel like all bets are off.

Overall my sense is that, unlike ephemeral siacoins which have a real use case, ephemeral contracts are purely a curiosity that are maybe handy during testing sometimes, and useless otherwise. I'm inclined to be conservative and not change any more consensus rules.

consensus/state.go Outdated Show resolved Hide resolved
@ChrisSchinnerl ChrisSchinnerl merged commit b94782a into master Dec 18, 2024
10 checks passed
@ChrisSchinnerl ChrisSchinnerl deleted the create-resolve branch December 18, 2024 16:39
n8maninger added a commit that referenced this pull request Dec 18, 2024
n8maninger added a commit that referenced this pull request Dec 18, 2024
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.

4 participants