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

Stateful replay protection #1405

Merged
merged 3 commits into from
May 21, 2023
Merged

Stateful replay protection #1405

merged 3 commits into from
May 21, 2023

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented May 19, 2023

Implements stateful replay protection in prepare_proposal to avoid adding the same transaction more than once in a block

@grarco grarco force-pushed the grarco/bugfix-replay-prepare branch from c221e9b to ae79f1b Compare May 19, 2023 15:26
bengtlofgren
bengtlofgren previously approved these changes May 19, 2023
Copy link
Contributor

@bengtlofgren bengtlofgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good in my 👀

@@ -648,6 +651,55 @@ where
response
}

/// Checks that neither the wrapper nor the inner transaction have already
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anoma anoma deleted a comment from bengtlofgren May 19, 2023
@juped juped self-requested a review May 19, 2023 20:10
Copy link
Member

@juped juped left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be based on the commit that introduced the bug; if that isn't identifiable, then the closest possible thing (like the replay protection branch).

replay_protection::get_tx_hash_key(&wrapper.tx_hash);
if temp_wl_storage
.has_key(&inner_hash_key)
.expect("Error while checking inner tx hash key in storage")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect()s should say something like "expected [invariant you expected to hold]"

apps/src/lib/node/ledger/shell/process_proposal.rs Outdated Show resolved Hide resolved
@@ -1236,8 +1198,8 @@ mod test_process_proposal {
assert_eq!(
response[1].result.info,
format!(
"Inner transaction hash {} already in storage, replay \
attempt",
"Transaction replay attempt: Inner transaction hash \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to rewrite strings let's do that in its own branch, not en passant

adrianbrink
adrianbrink previously approved these changes May 20, 2023
@juped juped dismissed stale reviews from adrianbrink and bengtlofgren via 1480727 May 20, 2023 16:57
@juped juped force-pushed the grarco/bugfix-replay-prepare branch from ae79f1b to 1480727 Compare May 20, 2023 16:57
juped added a commit that referenced this pull request May 20, 2023
…1405) into maint-0.15

* namada/grarco/bugfix-replay-prepare:
  Adds replay protection checks in prepare_proposal
grarco added a commit that referenced this pull request May 20, 2023
@juped juped force-pushed the grarco/bugfix-replay-prepare branch from 6f65fe8 to 52f803c Compare May 20, 2023 17:19
juped added a commit that referenced this pull request May 20, 2023
…1405) into maint-0.15

* namada/grarco/bugfix-replay-prepare:
  changelog: add #1405
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
juped added a commit that referenced this pull request May 20, 2023
…1405) into main

* namada/grarco/bugfix-replay-prepare:
  changelog: add #1405
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
juped added a commit that referenced this pull request May 20, 2023
…1405) into maint-0.15

* namada/grarco/bugfix-replay-prepare:
  changelog: add #1405
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
juped added a commit that referenced this pull request May 20, 2023
…1405) into maint-0.15

* namada/grarco/bugfix-replay-prepare:
  changelog: add #1405
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
@tzemanovic tzemanovic merged commit 0412cf4 into main May 21, 2023
@tzemanovic tzemanovic deleted the grarco/bugfix-replay-prepare branch May 21, 2023 16:08
tzemanovic added a commit that referenced this pull request May 21, 2023
Namada 0.15.4

* tag 'v0.15.4':
  Namada 0.15.4
  changelog: add #1407
  Fixes e2e tests
  changelog: add #1399
  Tendermint consensus params settable in Namada config
  Updates `InternalStats` display
  changelog: add #1405
  process_proposal: fix typos in test names
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
  Logs validation error in `process_proposal`
tzemanovic added a commit that referenced this pull request May 21, 2023
Namada 0.15.4

* tag 'v0.15.4':
  Namada 0.15.4
  changelog: add #1407
  Fixes e2e tests
  changelog: add #1399
  Tendermint consensus params settable in Namada config
  Updates `InternalStats` display
  changelog: add #1405
  process_proposal: fix typos in test names
  prepare_proposal: add replay protection tests
  Adds replay protection checks in prepare_proposal
  Logs validation error in `process_proposal`
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.

5 participants