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

feature: Support compute costs for ExtCosts #8426

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

aborg-dev
Copy link
Contributor

@aborg-dev aborg-dev commented Jan 24, 2023

UPDATE:
This PR has been updated to match the description of Compute Costs NEP: near/NEPs#455

Concretely, now we specify compute costs directly without weights indirection.


This PR addresses #8264, in particular allowing to add parameter weights for Storage Costs.

Some open questions and potential improvements:

  • Do we need to surface weights in the views.rs (RPC interface of the node?)
  • Ideally we would avoid specifying the old base value in the diff config if we only want to change the weight. For now we need to do:
wasm_storage_read_base: {
    old: { base: 50_000_000_000, weight: 3 },
    new: { base: 50_000_000_000, weight: 7 }
}

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Having a design for how to specify these brings us a step closer to writing a NEP with reference implementation.

I am not 100% yet if we want to merge it to master already. Ideally, we don't include changes before the NEP is approved. But if it makes your work easier, then we can also merge it already as it doesn't have any effect until we actually set some weights.

Do we need to surface weights in the views.rs (RPC interface of the node?)

I think we don't want to leak that info, it should not be of any concern to users. (If we need a way to check current weights of a running node, I think the debug page or even Grafana metrics would be more appropriate. But probably overkill and too early to add such a thing anyway.)

Ideally we would avoid specifying the old base value in the diff config if we only want to change the weight.

Hm, I see. The current solution also works okay enough I think. Including the gas value for full context does not seem all that bad anyway.

Also, there are some CI failures in existing tests.

@@ -262,9 +263,15 @@ pub struct ViewConfig {
pub max_gas_burnt: Gas,
}

#[derive(Debug, Clone, Hash, PartialEq, Eq)]
pub struct WeightedGas {
Copy link
Contributor

Choose a reason for hiding this comment

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

This naming makes it easy to be confused with https://docs.rs/near-sdk/latest/near_sdk/struct.GasWeight.html

Also, we already have "used", "prepaid", "burnt", "burned", and "attached" gas as terms used throughout the code and it often makes technical communication difficult. E.g. the question "how much gas does this TX use" already has at least 3 correct answers, depending on how you define "used gas" and transaction. Weighted gas adds one more interpretations of "used gas". 🙈

I don't know a better name, it's probably fine to keep it. But when writing the NEP (which we should start soon) we will need to make a final decision if we have an alternative naming or if we want to keep this.

Previously brainstormed ideas for what we can call weighted gas include:

  • fumes or emissions (parameter weight defines how much emissions burning gas causes)
  • cycles / compute time / chunk space (naming it after the condition checked when we include receipts in a chunk)

Again, keeping the name is probably fine. But I wanted to write this down here to give you the full context and maybe you can see a way to minimize how complicated the story around gas is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on the NEP, renamed the whole struct into

struct ParameterCost {
  gas: Gas,
  compute: u64,
}

I also plan to introduce a dedicated type for compute cost.

core/primitives/src/runtime/parameter_table.rs Outdated Show resolved Hide resolved
@@ -2624,7 +2624,8 @@ impl From<ExtCostsConfigView> for near_primitives_core::config::ExtCostsConfig {
ExtCosts::alt_bn128_g1_sum_element => view.alt_bn128_g1_sum_element,
ExtCosts::alt_bn128_pairing_check_base => view.alt_bn128_pairing_check_base,
ExtCosts::alt_bn128_pairing_check_element => view.alt_bn128_pairing_check_element,
};
}
.map(|_, value| WeightedGas { base: value, weight: 1.into() });
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be fine for now but it makes the conversion from config to view back to config lossy. We should check if we have some tests that would fail because of that. If so, we need to figure out how to deal with that test. (I don't want tests to randomly start failing because we set the first weight != 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll investigate such failures now, but I think an important question here is why do we even have a conversion back from the ConfigView to the Config? Is it used anywhere in a meaningful way? I'll try to disable it and see what breaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. So far, everything in the configuration was interesting enough to users that we wanted it to be reflected in the view and it's easy to test that by converting it back. (This argument might no longer holds with the addition of param weights).
  2. For some reasons I am not familiar, the Rosetta RPC adapter constructs a RuntimeConfig from an RPC call. There we actually need this conversion in production code.(This seems worth fixing but I don't know how much effort it would be.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added non-default compute cost to one of the parameters and verified that no tests in nearcore fail. I've also reviewed the code usage of RuntimeConfigView[1] and haven't found any examples where we do a conversion back from the view into config. I'll try removing the From in that direction and see if anything breaks and also will ask on Zulip in case anyone knows why it exists.

[1] https://cs.github.com/?scope=org%3Anear&scopeName=near&q=%22RuntimeConfigView%22

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, maybe the usage in rosetta has been removed. Even better :D

If you can remove it and all tests pass, go for it. I don't think we have to check further if it works in the latest code base. Otherwise, if for some reason we really need the conversion, I'd vote to remove the From impl anyway and replace it with a constructor that clearly indicates in the name that it is a lossy conversion.

near-bulldozer bot pushed a commit that referenced this pull request Jan 24, 2023
This PR codifies the already-existing structure of Fee parameters making parameter config easier to read and parameters easier to parse.

It has the same downside as #8426 in the diff configs that need to specify the old values in the other fields of the composite Fee value. I'll try to improve this in this PR by allowing some of the values to be Optional, but for now sending it as is to make sure that the general change makes sense.
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Jan 30, 2023
This PR codifies the already-existing structure of Fee parameters making parameter config easier to read and parameters easier to parse.

It has the same downside as near#8426 in the diff configs that need to specify the old values in the other fields of the composite Fee value. I'll try to improve this in this PR by allowing some of the values to be Optional, but for now sending it as is to make sure that the general change makes sense.
ppca pushed a commit to ppca/nearcore that referenced this pull request Jan 30, 2023
This PR codifies the already-existing structure of Fee parameters making parameter config easier to read and parameters easier to parse.

It has the same downside as near#8426 in the diff configs that need to specify the old values in the other fields of the composite Fee value. I'll try to improve this in this PR by allowing some of the values to be Optional, but for now sending it as is to make sure that the general change makes sense.
@aborg-dev aborg-dev changed the title feature: Support weighted ExtCosts feature: Support compute costs for ExtCosts Feb 14, 2023
@aborg-dev aborg-dev force-pushed the weight_definition branch 5 times, most recently from ce0b977 to 8e95ffb Compare March 20, 2023 13:59
@aborg-dev
Copy link
Contributor Author

@jakmeier , I've updated this PR according to the NEP and split it out into multiple parts.
This first part adds definition of Compute Costs to the parameter config. Given that this specific PR should not affect the behavior of nodes, do we still need to gate it behind a feature flag?

@aborg-dev aborg-dev force-pushed the weight_definition branch 2 times, most recently from 8e95ffb to 5a9897b Compare March 20, 2023 15:14
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Yeah, this PR clear does not need a protocol feature flag.
Actually, rethinking this, I am not sure we want a feature flag at all. All PRs should not affect current behavior, due to old protocol versions using the same gas and compute cost. I would probably only add one if you have a piece of code you want to not run, yet. But ultimately, we must be 100% sure the code doesn't affect old versions either way and usually the original PR review is easier to check that than a stabilization PR removing the flag.

@aborg-dev aborg-dev added S-automerge A-contract-runtime Area: contract compilation and execution, virtual machines, etc labels Mar 21, 2023
Adds ability to specify the compute costs defined in NEP-455 in the
parameter config.
near-bulldozer bot pushed a commit that referenced this pull request Apr 25, 2023
In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from `RuntimeConfigStore`. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967
nikurt pushed a commit that referenced this pull request Apr 25, 2023
In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from `RuntimeConfigStore`. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967
nikurt pushed a commit that referenced this pull request Apr 28, 2023
In #8426 this conversion became lossy due to the introduction of Compute Costs. This raised the question of whether this conversion is needed at all. I've found one place in the Indexer code that uses it and replaced it with a direct lookup for config from `RuntimeConfigStore`. As a part of this I've also simplified some code in the indexer.

This should fix the failing nayduck test #8967
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants