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

Add revision ordering test #196

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Add revision ordering test #196

merged 5 commits into from
Sep 13, 2024

Conversation

n8maninger
Copy link
Member

No description provided.

@lukechampine
Copy link
Member

good find. I'm not really sure how to handle this case. There are a few options:

  1. Disallow it entirely, i.e. enforce that a contract is revised at most once per block. This is obviously inconvenient.
  2. Track the revisions within a block, and require the Parent field to reflect the most recent revision within a block. This is a bit awkward because the Parent won't actually be present in the accumulator.
  3. Leave the Parent field as-is, but track the revisions so that we can reject lower revision numbers. This feels a bit contradictory: on one hand, since the Parent of all revisions is the same, the implication is that only one of those is the "real" revision. On the other hand, if only one revision is "real," why do we care if a later revision has a lower revision number, since it won't be getting applied anyway?
  4. Mark as wontfix -- you could make the case that, as long as the revision number increases at block-granularity, it doesn't matter if it decreases at transaction-granularity. This is weird because it lets you create multiple revisions of a contract that have the same revision number.

I'm leaning towards 2; I like that it forces the transaction creator to acknowledge the prior revision. And there's some precedent: It's similar to the check we have for ephemeral siacoin outputs (which are also not in the accumulator). Thoughts?

@n8maninger
Copy link
Member Author

n8maninger commented Sep 7, 2024

I think option one would be okay. If a renter and host both broadcast different versions of a contract, the higher one should always be the only one applied to the state. I can't think of a use case where we would need more than one revision per block. The tpool would need to be modified to eject lower revisions when a higher one is broadcast though.

Option three would be my preference over two. Having to check the tpool for an ephemeral parent is an extra step for broadcasting that feels unnecessary and adds additional friction since two parties can broadcast the revision.

On the other hand, if only one revision is "real," why do we care if a later revision has a lower revision number, since it won't be getting applied anyway

I might be misunderstanding this comment, but it looks like the lower revision is getting applied to the consensus state. If the second lower revision was ignored, I think we could safely mark this as wontfix. As it stands, you can overwrite a higher revision with a lower one as long it's in the same block.

@lukechampine
Copy link
Member

Yeah, I think I agree. Although option 1 is probably ok in practice, it feels unnecessarily restrictive; of the others, option 3 is the most convenient, and it doesn't have any practical drawbacks, only conceptual ones.

@n8maninger n8maninger merged commit 2cdf6f6 into master Sep 13, 2024
8 checks passed
@n8maninger n8maninger deleted the nate/v2-revision-ordering branch September 13, 2024 12:50
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