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

Replay protection improvements #1905

Merged
merged 7 commits into from
Nov 11, 2023
Merged

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Sep 19, 2023

Describe your changes

Closes #1568.

Better handles the case of an invalid wrapper tx (ABCI only) regarding fee payment and replay protection: the hash of the wrapper is committed to storage to avoid replays given that the wrapper signer, in this case, is forced to pay whatever amount of fees they can.

Remove the hash of the inner tx (for replay protection purposes) in more cases:

  • if the inner tx is actually Undecryptable
  • if the signature on the inner tx is invalid
  • if the transaction fails because of an invalid commitment to one of its sections (this is because we only use the hash of the Header for replay protection)

This makes for a more correct replay protection mechanism. To do this, I used some sentinel variables to be able to detect these errors in wasm. Also fixed a bug regarding the OutOfGas error which, for the same reason, could not be detected and was not triggering the hash removal (again, fixed using the same sentinel variable). Theoretically, the wasm error can be downcasted to the original error but the method to do so is fallible (and indeed, in some tests I ran it failed because of multiple strong references on the inner Arc): so, to guarantee a consisting behavior, I had to rely on the sentinel.

Indicate on which release or other PRs this topic is based on

v0.25.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@grarco grarco force-pushed the grarco/replay-protection-improvements branch 2 times, most recently from bbead89 to 037fbf7 Compare September 22, 2023 16:25
@grarco grarco marked this pull request as ready for review September 22, 2023 16:41
@grarco grarco requested a review from tzemanovic September 22, 2023 16:55
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 21d3385 to 49da8ff Compare September 26, 2023 16:00
@grarco grarco marked this pull request as draft September 28, 2023 09:14
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from bb1f128 to 0fdf7e2 Compare September 29, 2023 17:30
grarco added a commit that referenced this pull request Oct 2, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 30f18ad to 6b68dd7 Compare October 2, 2023 14:20
grarco added a commit that referenced this pull request Oct 3, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 6b68dd7 to 8d1932f Compare October 3, 2023 14:53
grarco added a commit that referenced this pull request Oct 3, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 14de649 to 52eda02 Compare October 3, 2023 19:00
@grarco grarco marked this pull request as ready for review October 3, 2023 19:00
@Fraccaman
Copy link
Member

is this mergeable in 0.24.0?

@Fraccaman Fraccaman mentioned this pull request Oct 16, 2023
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
@grarco grarco marked this pull request as draft October 24, 2023 12:37
@grarco
Copy link
Collaborator Author

grarco commented Oct 24, 2023

Converting this back to draft since it needs a few touches because of #1867

grarco added a commit that referenced this pull request Oct 24, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 52eda02 to 373f1b1 Compare October 24, 2023 16:11
grarco added a commit that referenced this pull request Oct 24, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 373f1b1 to 1e91762 Compare October 24, 2023 18:16
grarco added a commit that referenced this pull request Oct 25, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 1e91762 to eef404f Compare October 25, 2023 16:05
grarco added a commit that referenced this pull request Oct 25, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from eef404f to fc90634 Compare October 25, 2023 16:59
@grarco grarco marked this pull request as ready for review October 25, 2023 17:00
@grarco
Copy link
Collaborator Author

grarco commented Oct 25, 2023

@tzemanovic ready again for review

tzemanovic
tzemanovic previously approved these changes Nov 7, 2023
grarco added a commit that referenced this pull request Nov 7, 2023
@brentstone brentstone mentioned this pull request Nov 7, 2023
grarco added a commit that referenced this pull request Nov 7, 2023
tzemanovic
tzemanovic previously approved these changes Nov 7, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from a826c41 to 1e91762 Compare November 7, 2023 15:11
grarco added a commit that referenced this pull request Nov 7, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 1e91762 to 12aeb34 Compare November 7, 2023 15:35
@grarco grarco mentioned this pull request Nov 7, 2023
@grarco grarco force-pushed the grarco/replay-protection-improvements branch from 12aeb34 to 1878a4c Compare November 7, 2023 16:45
@grarco grarco mentioned this pull request Nov 7, 2023
@adrianbrink adrianbrink merged commit f16e2f1 into main Nov 11, 2023
12 of 14 checks passed
@adrianbrink adrianbrink deleted the grarco/replay-protection-improvements branch November 11, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove invalid tx hash from storage
5 participants