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

NEP-455: Parameter Compute Costs #455

Merged
merged 6 commits into from
Mar 21, 2023
Merged

Conversation

aborg-dev
Copy link
Contributor

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

NEP for Parameter Compute Costs (formerly known as Parameter Weights).

For the context, this NEP was previously named "Parameter Weights" and the idea was originally proposed in https://gov.near.org/t/proposal-gas-weights-to-fight-instability-to-due-to-undercharging/30919.
Now it's renamed into "Parameter Compute Cost" to more accurately represent the fact that it is about accounting how much compute time the parameters cost and using it to limit the chunk processing time.

@aborg-dev aborg-dev added the S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. label Jan 30, 2023
@aborg-dev aborg-dev changed the title NEP: Parameter weights [WIP] NEP: Parameter weights Jan 30, 2023
neps/nep-9999.md Outdated Show resolved Hide resolved
@frol frol added WG-protocol Protocol Standards Work Group should be accountable A-NEP A NEAR Enhancement Proposal (NEP). labels Feb 1, 2023
@aborg-dev
Copy link
Contributor Author

@jakmeier , I've drafted the rest of the sections, adding the main points that need to be covered there. Please take a look if there are any other topics that the NEP should highlight/clarify.

I also proposed a new terminology for the weights to make the naming a bit more future-looking. Specifically, I'm thinking of renaming "Parameter Weights" to "Parameter Compute Weights/Factors" to reflect the fact that this is a conversion from gas costs to compute costs. Similarly, I'm thinking of introducing a "Chunk Compute Limit" (specialized "Chunk Gas Limit") for the purpose of limiting the total compute usage in the chunk.
The idea is to more accurately reflect the fact that node hardware resources are multi-dimensional (CPU, RAM, Disk IOPS) and the limits during processing of the chunk should really be on each dimension separately as reaching limits on any of the dimensions would degrade chunk processing time significantly.
Gas still stays as a unit of account to simplify UX for users. This NEP in essense introduces a mechanism to convert compute resource usage to gas costs.

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.

This is taking on a good shape already. It's a great basis to involve more protocol team members to discuss details.

neps/nep-9999.md Outdated Show resolved Hide resolved
neps/nep-9999.md Outdated Show resolved Hide resolved
neps/nep-9999.md Outdated Show resolved Hide resolved
neps/nep-9999.md Outdated Show resolved Hide resolved
neps/nep-9999.md Outdated Show resolved Hide resolved
@aborg-dev aborg-dev changed the title [WIP] NEP: Parameter weights [WIP] NEP: Compute weights Feb 9, 2023
@aborg-dev
Copy link
Contributor Author

@jakmeier , in a process of polishing the Summary/Motivation/Spefication sections I came to conclusion that we need to do a bit more thinking around conceptual way we present this idea (as I've struggled to clearly write it down).

Here is my latest thinking trying to distill what we want to accomplish even further:

The root cause/tension behind underchargings is the fact that we want to use the average operation gas costs for the purpose of charging users (to avoid overcharging them and make their workload costs competetive with other chains), but we want to use safe gas costs for the purpose of limiting the chunk size (to limit the chunk production time in the worst case).

We mostly get away with making safe costs equal to average costs (as on average we don't hit the worst case), but in cases when the difference between the two is large (e.g. storage operations) this distinction becomes especially noticeable and calls for action.

So another conceptual way to think about this NEP is as an introduction of safe gas costs that approximate the worst case compute costs of operations.
As it is now, by default the safe gas cost matches the average gas cost.
As with weights, we can define the safe cost directly on per-parameter basis without the need for indirect weights (avoids working with rationals).

Then we will use safe gas costs for the purpose of counting towards chunk gas limit, but still use the existing gas cost for all other purposes (e.g. burning).

Overall, I think this is both easier to explain and also cuts to the root of the problem. What do you think?
As a next step, it would be good to check how this framing applies to existing undercharging situations as well as flat storage deployment.

@aborg-dev aborg-dev changed the title [WIP] NEP: Compute weights NEP-455: Compute weights Feb 10, 2023
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.

@Akashin I had a brief look over it. Left a few small grammar related comments and a thought or two on the content as well. But I think once these details are addressed, this looks ready to be submitted to the WG.

neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
@jakmeier
Copy link
Contributor

Oh and we should update the title and description to reflect the new name. (Maybe even a quick explanation that it used to be called weights but isn't anymore could be useful in the PR comment.)

@aborg-dev aborg-dev changed the title NEP-455: Compute weights NEP-455: Parameter Compute Costs Feb 13, 2023
@aborg-dev aborg-dev removed the S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. label Feb 13, 2023
@aborg-dev aborg-dev marked this pull request as ready for review February 13, 2023 14:18
@aborg-dev aborg-dev requested a review from a team as a code owner February 13, 2023 14:18
@aborg-dev
Copy link
Contributor Author

aborg-dev commented Feb 13, 2023

@near/nep-moderators , this NEP is now ready for review.

For the context, this NEP was previously named "Parameter Weights" and the idea was originally proposed in https://gov.near.org/t/proposal-gas-weights-to-fight-instability-to-due-to-undercharging/30919.
Now it's renamed into "Parameter Compute Cost" to more accurately represent the fact that it is about accounting how much compute time the parameters cost and using it to limit the chunk processing time.

@frol frol added the S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. label Feb 13, 2023
@frol
Copy link
Collaborator

frol commented Feb 13, 2023

Thank you @Akashin for submitting this NEP.

As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-protocol working group members to assign 2 Technical Reviewers to complete a technical review. If you want to assign yourself, please mention that you are acting as the Technical Reviewers.

Please find some guidelines below for completing the technical review.

Technical Review Guidelines
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.

  • Second, once all the suggestions are addressed, produce:

    • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation)

    • A summary of benefits

    • A summary of concerns and blockers that were identified on the way and their status (some will be resolved, others dismissed, etc)

Here is a nice example and a template for your convenience:

## Example

### Recommendation
Add recommendation

### Benefits

- Benefit
- Benefit

### Concerns

| # | Concern | Resolution | Status |
| - | - | - | - |   
| 1 | Concern | Resolution | Status |
| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

@bowenwang1996
Copy link
Collaborator

I nominate @jakmeier and @mzhangmzz as SME reviewers for this NEP.

@akhi3030
Copy link
Contributor

@blasrodri indicated on the Protocol Community Group that they will be interested in helping review this NEP.

@jakmeier
Copy link
Contributor

I nominate @jakmeier and @mzhangmzz as SME reviewers for this NEP.

@near/nep-moderators

As a listed co-author and one of the engineers originally proposing this idea, does it make sense for me to take on the role of an SME? Obviously, I can do it with little effort, given that I have already spent a lot of time discussing this topic with Andrei and others. But I wonder if this makes sense from a process perspective.

neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Feb 14, 2023

@jakmeier Thanks for flagging that! Indeed, it defeats the purpose of the review.

Fortunately, @blasrodri volunteered to help with the review, so together with @mzhangmzz we have 2 SMEs to review this NEP. Thanks for helping to scale the review process! With more participants, we can shard it! 😉

@frol frol added S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. and removed S-draft/needs-moderator-review A NEP in the DRAFT stage that needs a moderator review. labels Feb 14, 2023
@aborg-dev
Copy link
Contributor Author

@near/nep-moderators , given that this NEP have been approved by reviewers, can we please move it to the voting stage?

@frol
Copy link
Collaborator

frol commented Mar 8, 2023

As the moderator, I would like to thank @mzhangmzz and @blasrodri for completing the technical review. Based on your comments above, it seems like this proposal is ready for the working group members for review.

@near/wg-protocol – Can you please fully read this NEP and comment in the thread if you are leaning towards approving or rejecting it? Please make sure to include your rationale and any feedback that you have for the author.

Once we get 2/3 voting indications, we will schedule a public call for the author to present the NEP and for the working group members to formalize the voting.

@frol frol added S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. and removed S-review/needs-sme-review A NEP in the REVIEW stage is waiting for Subject Matter Expert review. labels Mar 8, 2023
@Longarithm Longarithm mentioned this pull request Mar 8, 2023
@bowenwang1996
Copy link
Collaborator

As a working group member, I lean towards approving this NEP. This NEP provides a way to mitigate the risks of undercharging and allow protocol developers to openly discuss undercharging issues without creating security risks. It would be nice to have a way to adjust compute costs dynamically, but I think this NEP is a good first step.

@jakmeier
Copy link
Contributor

@mm-near or @mfornet sorry for the spam, just a friendly reminder that this is waiting for your review. It would be great if one of you could approve this soon so we can reach the 2/3 majority and move this to the voting stage together with #399.

@mfornet mfornet self-requested a review March 15, 2023 08:24
Copy link
Member

@mfornet mfornet left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. I left some comments that are not blocking its approval.

neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Outdated Show resolved Hide resolved
neps/nep-0455.md Show resolved Hide resolved
neps/nep-0455.md Show resolved Hide resolved
@ori-near
Copy link
Contributor

As the moderator, I would like to thank @Akashin for submitting this NEP, and for the @near/wg-protocol working group members for your review. Based on the voting indications above, it seems like this proposal is close to a decision. We are therefore scheduling the fourth Protocol Work Group call, where this NEP can enter the final voting stage. Anyone can discuss the technical details by adding your comments to this discussion. And join the call to learn about the final decision and how to get more involved.

Meeting Info
📅 Date: Thursday, March 16, 4 PM UTC
✏️ Register

@ori-near ori-near added S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. and removed S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. labels Mar 15, 2023
@mm-near
Copy link

mm-near commented Mar 16, 2023

I lean towards approving this NEP.

As other folks have mentioned before, it allows us to 'stop the bleeding' (when we discover the undercharging) and buys us time to do the proper fix.

The main disadvantage I see, is that it still requires a new release + protocol change -- which might take couple weeks.

@ori-near
Copy link
Contributor

Thank you to everyone who attended the fourth Protocol Working Group meeting last Thursday! The working group members reviewed the NEP and reached the following consensus:

Status: Approved

Meeting Recording:
https://youtu.be/4VxRoKwLXIs

@Akashin and @jakmeier Thank you for authoring this NEP!

Next Steps:

  • NEP moderator to embed the benefits & concerns into the NEP document
  • @near/nep-moderators To approve and merge the NEP

@frol
Copy link
Collaborator

frol commented Mar 20, 2023

@Akashin Are we ready to merge this NEP? I just want to double-check that there are no outstanding last-minute fixes.

@frol frol added S-approved A NEP that was approved by a working group. and removed S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. labels Mar 20, 2023
near-bulldozer bot pushed a commit to near/nearcore that referenced this pull request Mar 21, 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:
- [X] Do we need to surface weights in the `views.rs` (RPC interface of the node?)
- [X] 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:
```yaml
wasm_storage_read_base: {
    old: { base: 50_000_000_000, weight: 3 },
    new: { base: 50_000_000_000, weight: 7 }
}
```
@aborg-dev
Copy link
Contributor Author

@frol , I did a few more cleanup steps, folding a large set of older commits into one and answering all pending questions and I think now the PR is ready to be merged.

@frol frol merged commit b788cbd into near:master Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). S-approved A NEP that was approved by a working group. WG-protocol Protocol Standards Work Group should be accountable
Projects
Status: APPROVED NEPs
Development

Successfully merging this pull request may close these issues.