feat(client): split storage to size and epoch before storing#2419
feat(client): split storage to size and epoch before storing#2419giac-mysten wants to merge 3 commits intomainfrom
Conversation
Signed-off-by: giac-mysten <giacomo@mystenlabs.com>
Signed-off-by: giac-mysten <giacomo@mystenlabs.com>
liquid-helium
left a comment
liquid-helium
left a comment
There was a problem hiding this comment.
Thanks @giac-mysten for adding this important feature! It looks good to me overall, left two minor comments.
Question: do you know if/how much this split call would increase the gas cost? If the gas cost is more expensive in dollar values than the Walrus space, then it would be tricky to split them.
| target_epoch: Option<Epoch>, | ||
| ) -> SuiClientResult<Vec<Blob>> { |
There was a problem hiding this comment.
nit: will there be calls of register_blobs with target_epoch = None, expect in tests? Does it make sense to use Epoch instead of Option?
| // Reserve space for the blobs. Collect all original storage resource objects ids. | ||
| // Each storage object has `EXTRA_BYTES_PER_STORAGE` bytes reserved, such that we can later | ||
| // check they were correctly split off. | ||
| const EXTRA_BYTES_PER_STORAGE: u64 = 2718; |
There was a problem hiding this comment.
Praise: Thanks for adding the tests!
I am wondering if we can test with spaces of different sizes pre-reserved: encoded_size - x, encoded_size, encoded_size + x. so that we can cover the case that some blobs reuse spaces, some registered new ones.
| /// | ||
| /// Storage resources are automatically split to the correct size and epoch if needed. | ||
| #[tracing::instrument(level = Level::DEBUG, skip_all)] | ||
| pub async fn register_blobs( |
There was a problem hiding this comment.
Remark/Suggestion:
I'm not sure if this is the main function that we should be concerned with.
We use this function mainly for tests, but as I've noticed now, we also use it in ResourceManager::get_existing_or_register_with_resources as separate call in addition to reserve_and_register, which we should not do, because that means potentially sending two transactions instead of one.
I think the main focus should be on reserve_and_register to ensure that existing storage is reused if possible (e.g. by passing a vector of storage resources that can then be merged & split, potentially combined with new storage), which should then also reduce the complexity in the ResourceManager.
We can then also do the splitting in this function here, but then I'd prefer an approach where instead of passing pairs of (BlobObjectMetadata, StorageResource), we pass two independent vectors (or slices) of BlobObjectMetadata and StorageResource, which then allows the storage resources to also be combined instead of just split. This would then also allow using the same logic here as in reserve_and_register for splitting & merging (modulo buying new storage, which would just cause an error here if that were needed).
|
This PR is stale because it has been open 14 days with no activity. It will be closed in 7 days unless you remove the |
|
This PR was closed because it has not seen any activity since being marked as stale. |
|
@giac-mysten Should we resurrect this PR? |
5 participants
Description
Ensures that, when reusing existing storage resources, the client splits the storage to the exact size both in amount of storage and number of epochs.
Closes WAL-208
Contributes to WAL-363
Test plan
Added relevant tests to check the splits when reusing storage.
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that
a user might notice and any actions they must take to implement updates. (Add release notes after the colon for each item)