[TieredStorage] Remove the general-purposed TieredStorageWriter#196
Conversation
brooksprumo
left a comment
brooksprumo
left a comment
There was a problem hiding this comment.
tiered_storage/writer.rs was added when we planned to support multiple
tiers in the tiered-storage (i.e., at least hot and cold). However, as we
changed our plan to handle cold accounts as state-compressed accounts,
we don't need a general purposed tiered-storage writer at this moment.
Why does this not also apply to TieredStorageReader?
We probably want to keep the TieredStorageReader so that we can provide some backward compatibility |
|
But at least I find TieredReadableAccount can be repurposed to HotAccount #218 |
| }) | ||
| } | ||
|
|
||
| pub fn write_accounts< |
There was a problem hiding this comment.
Who—if anyone—called this function before? I'm guessing no one? Maybe only tests are writing accounts? And those are calling HotStorageWriter::write_accounts() directly?
There was a problem hiding this comment.
No one in master. (Previously both hot and cold storage called this function in my draft development)
…-xyz#196) #### Problem tiered_storage/writer.rs was added when we planned to support multiple tiers in the tiered-storage (i.e., at least hot and cold). However, as we changed our plan to handle cold accounts as state-compressed accounts, we don't need a general purposed tiered-storage writer at this moment. #### Summary of Changes Remove tiered_storage/writer.rs as we currently don't have plans to develop cold storage. #### Test Plan Existing tiered-storage tests.
Transactions that use maximum possible account locks. Read and write lock variants.
It seems simpler to have two distinct generator templates: one for attacks that want to use the maximum possible number of accounts in a transaction, and another one that uses the maximum possible number of accounts of the maximum size. These two templates have constraints that are quite different, and it simplifies both the implementation and the interfaces of the templates. SQUASH: This change should be incorporated into the following changes: Brennan <brennan.watt@anza.xyz> Tue Sep 3 21:28:31 2024 -0700 constrain max accounts, shrink batch size (anza-xyz#419) Brennan <brennan.watt@anza.xyz> Thu Apr 18 18:10:43 2024 -0700 read max accounts attack (anza-xyz#327) Illia Bobyr <illia.bobyr@solana.com> Mon Oct 23 12:21:02 2023 -0700 Read and write lock attack (anza-xyz#196)
Original names were not specific enough. "Max" in the name could refer
to the transaction size, account size, number of accounts or, possibly,
even more other things. And we already have a `max_accounts_tx`
template that indeed is about putting as many accounts into a
transaction - it is confusing that `read_max_accounts` and
`max_accounts_tx` are unrelated.
FIXUP: This change should be absorbed into the changes that touch
`read_max_accounts.rs` and `write_max_accounts.rs`:
```
❯ git log --format=%s agave/master..upstream/master -- core/src/banking_stage/adversary/transaction_generators/{read,write}_max_accounts.rs
SQUASH Split max_size_accounts_tx out of max_accounts_tx (anza-xyz#454)
constrain max accounts, shrink batch size (anza-xyz#419)
write max accounts test (anza-xyz#328)
read max accounts attack (anza-xyz#327)
Read and write lock attack (anza-xyz#196)
```
2 participants
Problem
tiered_storage/writer.rs was added when we planned to support multiple
tiers in the tiered-storage (i.e., at least hot and cold). However, as we
changed our plan to handle cold accounts as state-compressed accounts,
we don't need a general purposed tiered-storage writer at this moment.
Summary of Changes
Remove tiered_storage/writer.rs as we currently don't have plans to develop cold storage.
Test Plan
Existing tiered-storage tests.