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.

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

refactor: cleaning up RuntimeConfig #8100

Merged
merged 2 commits into from
Nov 24, 2022

Conversation

jakmeier
Copy link
Contributor

refactor: cleaning up RuntimeConfig

Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:

  • Action fees are now accessed through one single function
    RuntimeFeesConfig::fee(&self, cost: ActionCosts) instead of
    selecting a specific field deep down in the structure
  • Action fees are stored in an EnumMap instead of by hard-coded fields
  • The JSON RPC now has RuntimeConfigView to keep compatibility
  • Creation of RuntimeConfig no longer goes thorough serde JSON
    serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:

self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;

After PR:

self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;

Also it gives us simplified fee lookup on RuntimeFeesConfig.
Before, we need things like:

let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();

which becomes

let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();

The final state after all refactoring is done is described in #8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

Test

We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have test_json_unchanged that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.

Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:
- Action fees are now accessed through one single function
  `RuntimeFeesConfig::fee(&self, cost: ActionCosts)` instead of
  selecting a specific field deep down in the structure
- Action fees are stored in an EnumMap instead of by hard-coded fields
- The JSON RPC now has `RuntimeConfigView` to keep compatibility
- Creation of `RuntimeConfig` no longer goes thorough serde JSON
  serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:
```rust
self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;
```
After PR:
```rust
self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;
```

Also it gives us simplified fee lookup on `RuntimeFeesConfig`.
Before, we need things like:
```rust
let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();
```

which becomes

```rust
let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();
```

The final state after all refactoring is done is described in near#8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

# Test
We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have `test_json_unchanged` that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.
@jakmeier jakmeier requested a review from Longarithm November 22, 2022 13:44
@jakmeier jakmeier requested a review from a team as a code owner November 22, 2022 13:44
@jakmeier jakmeier requested a review from mm-near November 22, 2022 13:44
Copy link
Member

@Longarithm Longarithm left a comment

Choose a reason for hiding this comment

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

Looks great! I hope there are no incorrectly replaced costs or mixed send and exec fees, though I looked at this. On the other hand, our tests should cover this.

@near-bulldozer near-bulldozer bot merged commit 045498b into near:master Nov 24, 2022
@jakmeier jakmeier deleted the refator-gas-profiles-part2 branch November 24, 2022 13:15
nikurt pushed a commit that referenced this pull request Nov 26, 2022
refactor: cleaning up RuntimeConfig

Sorry. This is a humongous change.
But it's really just a lot of the same boilerplate code changes.

The key changes are:
- Action fees are now accessed through one single function 
  `RuntimeFeesConfig::fee(&self, cost: ActionCosts)` instead of
  selecting a specific field deep down in the structure
- Action fees are stored in an EnumMap instead of by hard-coded fields
- The JSON RPC now has `RuntimeConfigView` to keep compatibility
- Creation of `RuntimeConfig` no longer goes thorough serde JSON
  serialization + deserialization

The goal of this PR is to make gas profiling based on parameters
possible, which in turn makes parameter weights possible to implement
much more easily. The key here is that we no longer have to lookup the
gas value up independently from the profile key.

Today:
```rust
self.gas_counter.pay_action_base(
    &self.fees_config.action_creation_config.deploy_contract_cost,
    sir,
    ActionCosts::deploy_contract_base,
)?;
```
After PR:
```rust
self.pay_action_base(ActionCosts::deploy_contract_base, sir)?;
```

Also it gives us simplified fee lookup on `RuntimeFeesConfig`.
Before, we need things like:
```rust
let exec_gas = self.cfg.action_receipt_creation_config.exec_fee()
    + self.cfg.action_creation_config.add_key_cost.function_call_cost.exec_fee()
    + num_bytes
        * self
            .cfg
            .action_creation_config
            .add_key_cost
            .function_call_cost_per_byte
            .exec_fee();
```

which becomes

```rust
let exec_gas = self.cfg.fee(ActionCosts::new_action_receipt).exec_fee()
    + self.cfg.fee(ActionCosts::add_function_call_key_base).exec_fee()
     + num_bytes * self.cfg.fee(ActionCosts::add_function_call_key_byte).exec_fee();
```

The final state after all refactoring is done is described in #8033.
This PR contains step 2 and 4 combined because I was not able to
separate them due to unforeseen dependencies we have in the code.

The hardest part should be done after the current PR and what follows
should be small PRs that get rid of types one by one.

# Test
We have to make sure this is a pure refactoring change without
side-effects. Luckily, we have `test_json_unchanged` that detects
changes in any protocol version.
Otherwise, we rely on tests that have gas profile snapshots to make
sure the gas fee lookup translation did not have errors.
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Jan 19, 2023
`ActionCreationConfigView` is still around for the RPC format but inside
the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

This has been obsolete since near#8100 I just forgot to delete it.
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Jan 19, 2023
`ActionCreationConfigView` is still around for the RPC format but inside
the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

This has been obsolete since near#8100 I just forgot to delete it.
jakmeier added a commit to jakmeier/nearcore that referenced this pull request Jan 19, 2023
`ActionCreationConfigView` is still around for the RPC format but inside
the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

The same goes for `DataReceiptCreationConfig` and
`AccessKeyCreationConfig`.

These have been obsolete since near#8100 I just forgot to delete them.
near-bulldozer bot pushed a commit that referenced this pull request Jan 27, 2023
`ActionCreationConfigView` is still around for the RPC format but inside the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

The same goes for `DataReceiptCreationConfig` and
`AccessKeyCreationConfig`.

These have been obsolete since #8100 I just forgot to delete them.
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Jan 30, 2023
`ActionCreationConfigView` is still around for the RPC format but inside the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

The same goes for `DataReceiptCreationConfig` and
`AccessKeyCreationConfig`.

These have been obsolete since near#8100 I just forgot to delete them.
ppca pushed a commit to ppca/nearcore that referenced this pull request Jan 30, 2023
`ActionCreationConfigView` is still around for the RPC format but inside the transaction runtime logic we now use
`RuntimeFeesConfig::action_fees` which stores these fees by parameter.

The same goes for `DataReceiptCreationConfig` and
`AccessKeyCreationConfig`.

These have been obsolete since near#8100 I just forgot to delete them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants