Switch to isReplayable #205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
I think we already discussed isReplayable in #quark, and I'm in favor. I do see the compelling case that we need it in the signature to give users control over whether replays are allowed. I like it as a boolean; I think there was some discussion about using maxReplays and it doubling as an extra(neous) stop condition for computing the current replay count, and I don't care for that idea, but I also wouldn't want to die on that hill.
This patch fixes some items in regards to a user signing `isReplayable`. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce _must be_ `bytes32(uint256(-1))`, but when it is replayable, the first play _must be_ `op.nonce`. If the user doesn't sign `isReplayable`, then we'd need to enforce that the first play is `replayToken = op.nonce`, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world. Also, we start to fix a bit of verbiage to make things a bit nicer. Techncially, while it's missing about 200 new tests, this should be a complete implementation.
hayesgm
force-pushed
the
hayesgm/is-replayable
branch
from
16d4a98 to
4f02f25
Compare
fluffywaffles
approved these changes
Sep 10, 2024
kevincheng96
approved these changes
Sep 11, 2024
There was a problem hiding this comment.
Look great overall! No blocking issues, other than to re-run the formatter and generate a new gas snapshot.
EDIT: Well, actually, this comment I think is worth discussing.
hayesgm
added a commit
that referenced
this pull request
Sep 11, 2024
Previously in `QuarkNonceManager`, we used `submissionToken=EXHAUSTED` for non-replayable submissions and `submissionToken=nonce` for the first play of replayable submissions. Overall, this behavior was inconsistent (though otherwise fine) and generally unobservable. With the addition of `getActiveSubmissionToken`, this behavior became obervable and showed its inconsistencies. This patch changes the defined behavior making it where we submit `submissionToken=nonce` for the first play or a single-use or replayable quark operation, but still retain the behavior that `submissions[nonce]=EXHAUSTED` for non-replayables. This ends up as a reduction in the overall code complexity. The only true behavior this changes is that we must (and should) ban `nonce=bytes32(0)` (and for the sake of consistency also `nonce=bytes32(uint_max)`). This is because we would be submitting with `submissionToken=FREE` for that nonce and overall it isn't worth the risk of allowing that (since it wouldn't, but _could_ lead to unlimited replays if other code isn't implemented correctly). Overall, I think this change is a net benefit in reduction of code complexity and better understandability from the end-user's perspective. Patches: * Add a test where we check what values a user can submit for submission tokens * Addressed some #205 comments
hayesgm
added a commit
that referenced
this pull request
Sep 11, 2024
Previously in `QuarkNonceManager`, we used `submissionToken=EXHAUSTED` for non-replayable submissions and `submissionToken=nonce` for the first play of replayable submissions. Overall, this behavior was inconsistent (though otherwise fine) and generally unobservable. With the addition of `getActiveSubmissionToken`, this behavior became obervable and showed its inconsistencies. This patch changes the defined behavior making it where we submit `submissionToken=nonce` for the first play or a single-use or replayable quark operation, but still retain the behavior that `submissions[nonce]=EXHAUSTED` for non-replayables. This ends up as a reduction in the overall code complexity. The only true behavior this changes is that we must (and should) ban `nonce=bytes32(0)` (and for the sake of consistency also `nonce=bytes32(uint_max)`). This is because we would be submitting with `submissionToken=FREE` for that nonce and overall it isn't worth the risk of allowing that (since it wouldn't, but _could_ lead to unlimited replays if other code isn't implemented correctly). Overall, I think this change is a net benefit in reduction of code complexity and better understandability from the end-user's perspective. Patches: * Add a test where we check what values a user can submit for submission tokens * Addressed some #205 comments
hayesgm
added a commit
that referenced
this pull request
Sep 11, 2024
Previously in `QuarkNonceManager`, we used `submissionToken=EXHAUSTED` for non-replayable submissions and `submissionToken=nonce` for the first play of replayable submissions. Overall, this behavior was inconsistent (though otherwise fine) and generally unobservable. With the addition of `getActiveSubmissionToken`, this behavior became obervable and showed its inconsistencies. This patch changes the defined behavior making it where we submit `submissionToken=nonce` for the first play or a single-use or replayable quark operation, but still retain the behavior that `submissions[nonce]=EXHAUSTED` for non-replayables. This ends up as a reduction in the overall code complexity. The only true behavior this changes is that we must (and should) ban `nonce=bytes32(0)` (and for the sake of consistency also `nonce=bytes32(uint_max)`). This is because we would be submitting with `submissionToken=FREE` for that nonce and overall it isn't worth the risk of allowing that (since it wouldn't, but _could_ lead to unlimited replays if other code isn't implemented correctly). Overall, I think this change is a net benefit in reduction of code complexity and better understandability from the end-user's perspective. Patches: * Add a test where we check what values a user can submit for submission tokens * Addressed some #205 comments
hayesgm
added a commit
that referenced
this pull request
Sep 11, 2024
Previously in `QuarkNonceManager`, we used `submissionToken=EXHAUSTED` for non-replayable submissions and `submissionToken=nonce` for the first play of replayable submissions. Overall, this behavior was inconsistent (though otherwise fine) and generally unobservable. With the addition of `getActiveSubmissionToken`, this behavior became obervable and showed its inconsistencies. This patch changes the defined behavior making it where we submit `submissionToken=nonce` for the first play or a single-use or replayable quark operation, but still retain the behavior that `submissions[nonce]=EXHAUSTED` for non-replayables. This ends up as a reduction in the overall code complexity. The only true behavior this changes is that we must (and should) ban `nonce=bytes32(0)` (and for the sake of consistency also `nonce=bytes32(uint_max)`). This is because we would be submitting with `submissionToken=FREE` for that nonce and overall it isn't worth the risk of allowing that (since it wouldn't, but _could_ lead to unlimited replays if other code isn't implemented correctly). Overall, I think this change is a net benefit in reduction of code complexity and better understandability from the end-user's perspective. Patches: * Add a test where we check what values a user can submit for submission tokens * Addressed some #205 comments
This patch adds support for setting the active nonce and submission token in temporary storage so scripts can read it and get the current nonce, submission token, and if they so choose, N. This is useful for certain scripts.
hayesgm
added a commit
that referenced
this pull request
Sep 12, 2024
This patch fixes some items in regards to a user signing `isReplayable`. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce _must be_ `bytes32(uint256(-1))`, but when it is replayable, the first play _must be_ `op.nonce`. If the user doesn't sign `isReplayable`, then we'd need to enforce that the first play is `replayToken = op.nonce`, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world. Also, we start to fix a bit of verbiage to make things a bit nicer. Techncially, while it's missing about 200 new tests, this should be a complete implementation. Whitespace changes for fmt check
hayesgm
added a commit
that referenced
this pull request
Sep 13, 2024
This patch fixes some items in regards to a user signing `isReplayable`. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce _must be_ `bytes32(uint256(-1))`, but when it is replayable, the first play _must be_ `op.nonce`. If the user doesn't sign `isReplayable`, then we'd need to enforce that the first play is `replayToken = op.nonce`, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world. Also, we start to fix a bit of verbiage to make things a bit nicer. Techncially, while it's missing about 200 new tests, this should be a complete implementation. Whitespace changes for fmt check
hayesgm
added a commit
that referenced
this pull request
Sep 13, 2024
This patch fixes some items in regards to a user signing `isReplayable`. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce _must be_ `bytes32(uint256(-1))`, but when it is replayable, the first play _must be_ `op.nonce`. If the user doesn't sign `isReplayable`, then we'd need to enforce that the first play is `replayToken = op.nonce`, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world. Also, we start to fix a bit of verbiage to make things a bit nicer. Techncially, while it's missing about 200 new tests, this should be a complete implementation. Whitespace changes for fmt check Fix two minor comments
hayesgm
added a commit
that referenced
this pull request
Sep 13, 2024
This patch fixes some items in regards to a user signing `isReplayable`. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce _must be_ `bytes32(uint256(-1))`, but when it is replayable, the first play _must be_ `op.nonce`. If the user doesn't sign `isReplayable`, then we'd need to enforce that the first play is `replayToken = op.nonce`, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world. Also, we start to fix a bit of verbiage to make things a bit nicer. Techncially, while it's missing about 200 new tests, this should be a complete implementation. Whitespace changes for fmt check Fix two minor comments Minor gas improvements
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes some items in regards to a user signing
isReplayable. Specifically, when a user signs an operation, they need to specify if the quark operation is replayable or not. When not, the state manager nonce must bebytes32(uint256(-1)), but when it is replayable, the first play must beop.nonce. If the user doesn't signisReplayable, then we'd need to enforce that the first play isreplayToken = op.nonce, which is bad since it that script could (if a bad nonce is chosen) end up replayable, which is an attack vector for a semi-bad client decision. Thus, we add this check to the user's signature and all things should be good in the world.Also, we start to fix a bit of verbiage to make things a bit nicer.
Techncially, while it's missing about 200 new tests, this should be a complete implementation.