refactor: 1:1 mapping for ActionCosts to fees
#8044
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
So far several actions fees were conflated right when they are added to the profile. This is a first step to make profiles universal by tracking in more fine-grained manner at the profile interface. Inside, it's still conflated but it's easier to refactor in steps. (see near#8033)
|
Context for next steps as outline in #8033 is quite important here. Notably, the The reason for this intermediate step is some circular requirements. I can't easily change the profile's inside before changing runtime config to use parameters everywhere. But I cannot use parameters for actions as long as there is no 1:1 mapping. |
matklad
approved these changes
Nov 14, 2022
Comment on lines
+188
to
+189
| Cost::ActionCost { action_cost_kind: ActionCosts::function_call_base } => 3, | ||
| Cost::ActionCost { action_cost_kind: ActionCosts::function_call_byte } => 3, |
There was a problem hiding this comment.
Yeah, I think it's fine to map them to a single entry for the time being. Long term, we should just make the array bigger. The serialization/deserialization code doesn't really care about length, is just zeros unknown cost.
It's better not to change the meaning of indicies, to simplify historical analysis, but even that, strinctly speaking, is not required. If we want, wee can reassign the meaning without protocol change.
Comment on lines
+659
to
+662
| deploy_contract_base, | ||
| deploy_contract_byte, | ||
| function_call_base, | ||
| function_call_byte, |
used gas must include burned gas on this interface
jakmeier
added a commit
to jakmeier/nearcore
that referenced
this pull request
Nov 16, 2022
This cost never made sense to me. It's cost is defined by `new_data_receipt_byte` but for some reason it was tracked as `value_return`. In near#8044 I introduced `new_data_receipt_byte` as an action cost but it is actually never used. This change replaces the old `value_return` cost with `new_data_receipt_byte`. This is an observable change on the RPC, as the profile now returns a different key for this cost. But is is not a breaking change because the profile keys are untyped.
near-bulldozer bot
pushed a commit
that referenced
this pull request
Nov 18, 2022
This cost never made sense to me. It's cost is defined by `new_data_receipt_byte` but for some reason it was tracked as `value_return`. In #8044 I introduced `new_data_receipt_byte` as an action cost but it is actually never used. This change replaces the old `value_return` cost with `new_data_receipt_byte`. This is an observable change on the RPC, as the profile now returns a different key for this cost. But is is not a breaking change because the profile keys are untyped.
nikurt
pushed a commit
that referenced
this pull request
Nov 22, 2022
This cost never made sense to me. It's cost is defined by `new_data_receipt_byte` but for some reason it was tracked as `value_return`. In #8044 I introduced `new_data_receipt_byte` as an action cost but it is actually never used. This change replaces the old `value_return` cost with `new_data_receipt_byte`. This is an observable change on the RPC, as the profile now returns a different key for this cost. But is is not a breaking change because the profile keys are untyped.
jakmeier
added a commit
to jakmeier/nearcore
that referenced
this pull request
Nov 24, 2022
We have to charge the gas in one block to preserve the old behavior in case of an "out of gas" failure in between. I will maybe submit the change that makes it "right" with a protocol feature flag, for now just revert it.
nikurt
pushed a commit
that referenced
this pull request
Nov 26, 2022
jakmeier
added a commit
to jakmeier/nearcore
that referenced
this pull request
Nov 29, 2022
Any action can fail due to gas limit exceeded or gas attached exceeded. We must preserve the exact result, including how much gas is burnt. In near#8044, we missed such a case and accidentally changed the burnt gas value if it fails in the wrong place and only exceeds the attached gas, not the gas burnt limit. Tests added here cover this case for all existing action gas costs and would catch this specific error in the future. Not covered yet are ext costs and the case where we add new action costs. We can bandage aid by adding similar tests for ext costs but we also have to come up with a more general prevention.
near-bulldozer bot
pushed a commit
that referenced
this pull request
Jan 20, 2023
Any action can fail due to gas limit exceeded or gas attached exceeded. We must preserve the exact result, including how much gas is burnt. In #8044, we missed such a case and accidentally changed the burnt gas value if it fails in the wrong place and only exceeds the attached gas, not the gas burnt limit. Tests added here cover this case for all existing action gas costs and would catch this specific error in the future. Not covered yet are ext costs and the case where we add new action costs. We can bandage aid by adding similar tests for ext costs but we also have to come up with a more general prevention.
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.
So far, several actions fees were conflated right when they are added to
the profile. Here is a first step to make profiles universal by tracking
in more fine-grained manner at the profile interface. Inside, it's
still conflated but it's easier to refactor in steps. (see #8033)