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.

Sign up for GitHub

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

Jump to bottom

Replay protection storage optimization #1977

Merged
merged 17 commits into from
Oct 24, 2023
Merged

Replay protection storage optimization #1977

merged 17 commits into from
Oct 24, 2023

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Oct 9, 2023 •
edited
Loading

Describe your changes

Addresses #1566.

Moves the replay protection storage outside of subspace to prevent writing the diff keys (still keeps the keys written at the previous height to allow for a rollback). Removes the previous solution to prevent merkelizing the replay protection keys which isn't needed anymore. Finally, implements a small optimization for which only one hash for tx is ever needed in storage (either the wrapper or the inner one, instead of both).

Improves unit tests.

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

v0.23.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-storage branch 4 times, most recently from 895eaf2 to 9178c79 Compare October 11, 2023 09:41
grarco added a commit that referenced this pull request Oct 11, 2023
@grarco
Changelog #1977
8cf0901
@grarco grarco force-pushed the grarco/replay-protection-storage branch from 9178c79 to 8cf0901 Compare October 11, 2023 10:01
grarco added a commit that referenced this pull request Oct 11, 2023
@grarco
Changelog #1977
7d476fb
@grarco grarco force-pushed the grarco/replay-protection-storage branch from 8cf0901 to 7d476fb Compare October 11, 2023 10:02
@grarco grarco marked this pull request as ready for review October 11, 2023 10:03
@grarco grarco requested a review from yito88 October 11, 2023 10:03
yito88
yito88 reviewed Oct 12, 2023
View reviewed changes
Copy link
Member

@yito88 yito88 left a comment

Choose a reason for hiding this comment

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

Can we call write_tx_hash or other functions for replay protection without wl_storage.write_log? I mean wl_storage.write_tx_hash().
There are many callers with wl_storage.write_log or wl_storage.storage. They are a bit confusing because they are not clear when these data are persistent in the storage. (Some bugs happened before by writing data with wl_storage.storage directly before the block commit. I think that the direct write to the storage only for testing.

apps/src/lib/node/ledger/storage/rocksdb.rs Outdated
//! - `pred`: predecessor values of the top-level keys of the same name
//! - `tx_queue`
//! - `next_epoch_min_start_height`
//! - `next_epoch_min_start_time`
//! - `replay_protection`
Copy link
Member

Choose a reason for hiding this comment

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

Is this stored in the state column family?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, this is a typo

grarco added a commit that referenced this pull request Oct 13, 2023
@grarco
Changelog #1977
f77aee3
@grarco grarco force-pushed the grarco/replay-protection-storage branch from 7d476fb to f77aee3 Compare October 13, 2023 18:02
grarco added a commit that referenced this pull request Oct 13, 2023
@grarco
Changelog #1977
1a00430
@grarco grarco force-pushed the grarco/replay-protection-storage branch from f77aee3 to 1a00430 Compare October 13, 2023 20:14
@grarco
Copy link
Contributor Author

grarco commented Oct 13, 2023

Can we call write_tx_hash or other functions for replay protection without wl_storage.write_log? I mean wl_storage.write_tx_hash().
There are many callers with wl_storage.write_log or wl_storage.storage. They are a bit confusing because they are not clear when these data are persistent in the storage. (Some bugs happened before by writing data with wl_storage.storage directly before the block commit. I think that the direct write to the storage only for testing.

So the keys for replay protection are meant to be written even in case the transaction failed, so the replay_protection field of WriteLog is treated as the block_write_log but I actually never directly write to storage. This is also because I need to write these keys during validation in process_proposal and prepare_proposal in which the storage is behind an immutable reference. I can reexpose the functions in WlStorage if you prefer, to avoid calling them directly from the write log

@yito88
Copy link
Member

yito88 commented Oct 16, 2023

Reexposing the functions in WlStorage is better, I think.
In my opinion, WlStorage callers should not choose the write log or the storage (all writes should be stored in the write log) except for the block commit and testing.

@grarco grarco force-pushed the grarco/replay-protection-storage branch from 1a00430 to 4fca277 Compare October 16, 2023 13:45
@grarco grarco mentioned this pull request Oct 19, 2023
Closed
grarco added a commit that referenced this pull request Oct 20, 2023
@grarco
Merge branch 'grarco/replay-protection-storage' (#1977)
4dfa8bc
@cwgoes cwgoes mentioned this pull request Oct 20, 2023
Closed
10 tasks
Fraccaman added a commit that referenced this pull request Oct 23, 2023
@Fraccaman
Merge branch 'grarco/replay-protection-storage' (#1977)
44c5cdf 
* origin/grarco/replay-protection-storage:
  Changelog #1977
  Expose replay protection methods from `WlStorage`
  Reworks replay protection check
  Fixes check for replay protection keys
  Writes only one hash at a time for replay protection
  Removes wrapper hash when committed inner tx
  Renames `finalize_tx_hashes`
  Fixes unit tests
  Improves replay protection `WriteLog` API
  New field in `WriteLog` for replay protection changes
  Refactors replay protection helper functions
  Updates `DB` and `DBIter` traits for replay protection
  Removes replay protecion internal address and vp
  Renames replay protection storage key getter
  Removes hacky solution for replay protection merkle tree
  Writes replay protection keys under separate storage root
  Replay protection column family and related methods in `DB` trait and `Storage`
Fraccaman added a commit that referenced this pull request Oct 23, 2023
@Fraccaman
fixup! Merge branch 'grarco/replay-protection-storage' (#1977)
3750365
tzemanovic added a commit that referenced this pull request Oct 24, 2023
@tzemanovic
Merge branch 'grarco/replay-protection-storage' (#1977)
de819d1 
* origin/grarco/replay-protection-storage:
  Changelog #1977
  Expose replay protection methods from `WlStorage`
  Reworks replay protection check
  Fixes check for replay protection keys
  Writes only one hash at a time for replay protection
  Removes wrapper hash when committed inner tx
  Renames `finalize_tx_hashes`
  Fixes unit tests
  Improves replay protection `WriteLog` API
  New field in `WriteLog` for replay protection changes
  Refactors replay protection helper functions
  Updates `DB` and `DBIter` traits for replay protection
  Removes replay protecion internal address and vp
  Renames replay protection storage key getter
  Removes hacky solution for replay protection merkle tree
  Writes replay protection keys under separate storage root
  Replay protection column family and related methods in `DB` trait and `Storage`
@tzemanovic tzemanovic mentioned this pull request Oct 24, 2023
Merged
@grarco grarco merged commit 4fca277 into main Oct 24, 2023
11 of 14 checks passed
@grarco grarco deleted the grarco/replay-protection-storage branch October 24, 2023 11:26
brentstone pushed a commit that referenced this pull request Nov 11, 2023
@tzemanovic @brentstone
Merge branch 'grarco/replay-protection-storage' (#1977)
a8b98e8 
* origin/grarco/replay-protection-storage:
  Changelog #1977
  Expose replay protection methods from `WlStorage`
  Reworks replay protection check
  Fixes check for replay protection keys
  Writes only one hash at a time for replay protection
  Removes wrapper hash when committed inner tx
  Renames `finalize_tx_hashes`
  Fixes unit tests
  Improves replay protection `WriteLog` API
  New field in `WriteLog` for replay protection changes
  Refactors replay protection helper functions
  Updates `DB` and `DBIter` traits for replay protection
  Removes replay protecion internal address and vp
  Renames replay protection storage key getter
  Removes hacky solution for replay protection merkle tree
  Writes replay protection keys under separate storage root
  Replay protection column family and related methods in `DB` trait and `Storage`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yito88 yito88 yito88 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grarco @yito88