Replay protection refactor #2956
Conversation
7a3ad5b to
9dff833
Compare
| //! - `all`: the hashes included up to the last block | ||
| //! - `last`: the hashes included in the last block | ||
| //! - `replay_protection`: hashes of processed tx for replay protection purposes | ||
| //! - `current/{hash}`: an hash included in the current block |
There was a problem hiding this comment.
| //! - `current/{hash}`: an hash included in the current block | |
| //! - `current/{hash}`: a hash included in the current block |
| //! - `last`: the hashes included in the last block | ||
| //! - `replay_protection`: hashes of processed tx for replay protection purposes | ||
| //! - `current/{hash}`: an hash included in the current block | ||
| //! - `{hash}`: an hash included in previous blocks |
There was a problem hiding this comment.
| //! - `{hash}`: an hash included in previous blocks | |
| //! - `{hash}`: a hash included in previous blocks |
| // deleted, this is fine, in this case just continue | ||
| let all_key = replay_protection::all_key(&hash); | ||
| batch.delete_cf(reprot_cf, all_key.to_string()); | ||
| // Remove the "current" tx hashes |
There was a problem hiding this comment.
Don't we also need to remove the replay_protection::key(_) entries committed by the previous block as well?
There was a problem hiding this comment.
I don't think so. Let's say we are at block 10 and we have tx1, tx2, tx3 committed in the previous blocks and tx4, tx5 committed in 10. Say that we want to rollback to 9, then we need to remove the transactions committed in 10 (tx4, tx5) which live under the current subkey.
We could think about bringing the tx committed in block 9 back under the new current subkey but I believe this is not needed because these keys are known anyway and the validation doesn't change if the hash lives under the current subkey or not. Also, replay protection is not merklized so this is not needed
…ant. Updates tests
6d8d018 to
66f85c1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2956 +/- ##
==========================================
- Coverage 59.39% 59.37% -0.02%
==========================================
Files 298 298
Lines 92771 92685 -86
==========================================
- Hits 55104 55036 -68
+ Misses 37667 37649 -18 ☔ View full report in Codecov by Sentry. |
a887373 to
bed0517
Compare
|
is this still just awaiting a final review? |
| .expect("Test failed"); | ||
|
|
||
| // the merkle tree root should not change after finalize_block | ||
| let root_post = shell.shell.state.in_mem().block.tree.root(); |
There was a problem hiding this comment.
This is probably me forgetting details, but since the second tx should pass, wouldn't its hash be added to replay prot and change the merkle tree?
There was a problem hiding this comment.
The reprot entries of the db are not merklized
* grarco/refactor-reprot: Changelog #2956 Rebase fixes Fixes typos in rocksdb documentation Removes `ReProtStorageModification` Updates rocksdb layout docs Fixes bug in replay protection iterator Removes unused reprot action and adds one to mark tx hashes as redundant. Updates tests `has_replay_protection_entry` returns a bool instead of result Renames reprot action `Delete` to `Redundant` Removes `Finalize` replay protection action. Improves replay protection api Adds replay protection unit test
Describe your changes
Closes #2922.
Since there's no more need to keep track and possibly act on tx cases from different block, this PR removes the
ReProtStorageModificationenum and uses a simpleHashSetto keep track of the transaction hashes.Also improves replay protection unit tests.
Indicate on which release or other PRs this topic is based on
v0.33.0
Checklist before merging to
draft