Storage fee for state bytes refundable #11279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Dec 11, 2023
Merged
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
msmouse
force-pushed
the
1208-alden-no-free-quota
branch
from
4fb6d4a
to
2634170
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
39163cb
to
1f382c0
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11279 +/- ##
========================================
Coverage 71.4% 71.4%
========================================
Files 799 799
Lines 183539 183702 +163
========================================
+ Hits 131050 131217 +167
+ Misses 52489 52485 -4 ☔ View full report in Codecov by Sentry. |
msmouse
force-pushed
the
1208-alden-no-free-quota
branch
from
2634170
to
ad64c24
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
2 times, most recently
from
9b277b2
to
c38c619
Compare
Merged
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
c38c619
to
d177cc6
Compare
msmouse
changed the base branch from
1208-alden-no-free-quota
to
1206_consolidate_to_pricing
Merged
msmouse
force-pushed
the
1206_consolidate_to_pricing
branch
from
e2a17a7
to
daea84e
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
2 times, most recently
from
b745f5c
to
690c116
Compare
msmouse
changed the title
msmouse
force-pushed
the
1206_consolidate_to_pricing
branch
from
daea84e
to
2661421
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
690c116
to
4afcf64
Compare
msmouse
force-pushed
the
1206_consolidate_to_pricing
branch
from
2661421
to
bc00839
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
4afcf64
to
ba94712
Compare
msmouse
force-pushed
the
1206_consolidate_to_pricing
branch
from
bc00839
to
b054435
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
ba94712
to
7c75a2b
Compare
msmouse
force-pushed
the
1206_consolidate_to_pricing
branch
from
b054435
to
1b28cd6
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
7c75a2b
to
7a33a9a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
igor-aptos
reviewed
Feb 14, 2024
There was a problem hiding this comment.
@gelash can you take a look, and recheck would access to prev_size be properly captured and validated (especially new field in ResourceGroupWrite?
do we need test/proptest for that?
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
fff37b8
to
568e8d7
Compare
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
2 times, most recently
from
71410f1
to
6ae89bd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gelash
reviewed
Feb 14, 2024
| @@ -133,6 +154,7 @@ pub struct GroupWrite { | |||
| pub(crate) inner_ops: BTreeMap<StructTag, (WriteOp, Option<Arc<MoveTypeLayout>>)>, | |||
| /// Group size as used for gas charging, None if (metadata_)op is Deletion. | |||
| pub(crate) maybe_group_op_size: Option<u64>, | |||
| pub(crate) prev_group_size: u64, | |||
There was a problem hiding this comment.
I suppose this is 0 if group is being created?
I'm fine with it, just wondering whether we should have maybe_group_op_size and prev_group_size have a consistent convention. It's also fine to wait for later (can add a TODO)
| @@ -255,6 +255,7 @@ impl<'r> WriteOpConverter<'r> { | |||
| self.convert(state_value_metadata, metadata_op, false)?, | |||
| inner_ops, | |||
| post_group_size.get(), | |||
| pre_group_size.get(), | |||
There was a problem hiding this comment.
@msmouse if you don't mind, and are making changes to the PR anyway (otherwise i'll do it later), could you update the comment above, just noticed it's outdated (we don't encode the size, also an important point is to explain why we even need an Op now).
i.e. could you please replace 243-246 with
"Create an op to encode the proper kind for resource group operation"
(@igor-aptos I think now this is the main reason?)
Interestingly, we may be able to get rid of the ugly metadata_op, since we have both pre_group_size and post_group_size, thanks @msmouse! (but not in this PR obviously :)
Otherwise, pre_group_size should be properly validated, we rely on it for resource group correctness, in fact the kind, i.e. Creation/Modification/Deletion, and it passed replay.
proptests for resource groups do already check the resource_group_size() call (re: @igor-aptos)
msmouse
force-pushed
the
1210-alden-ephemeral-bytes-fee
branch
from
6ae89bd
to
f1aca08
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
1. legacy slots that didn't pay bytes deposits won't get charged for the bytes allocated for free.
1. Considering pricing change, charge only to the point where the total deposit for bytes don't go beyond
current_price_per_byte x num_current_bytesPricing is reduced to 4k octas per slot and 40 octas per byte. As a result:
1. allocating an item smaller than 250 bytes is cheaper than before.
2. allocating an item of the size between 250 to 4K bytes will be slightly more expensive. The price increase peaks at 1KB at about 60%
3. merely touching a slot > 1KB no longer imposes penalty so such txns get significantly cheaper. As seen in MutateLargeTokenV2, UpgradeLarge, etc.
Test Plan