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.

Sign up for GitHub

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

Jump to bottom

protocol_feature_fix_contract_loading_cost tracking issue #5962

Closed
1 task
matklad opened this issue Dec 23, 2021 · 6 comments
Closed
1 task

protocol_feature_fix_contract_loading_cost tracking issue #5962

matklad opened this issue Dec 23, 2021 · 6 comments
Assignees
@jakmeier
Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc C-bug Category: This is a bug C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@matklad
Copy link
Contributor

matklad commented Dec 23, 2021 •
edited
Loading

protocol_feature_fix_contract_loading_cost features changes the logic for charging for loading contract. Roughly, this is a per-byte cost which measures the work of "dynamic linker" (loading pre-compiled x86_64 machine code from the database into memory, doing neccesary adjustments and marking the relevant pages as executable).

Specifically, the "fix" here covers:

  • charging the cost if contract execution was interrupted before the actual execution started (method not found and other such errors)
  • charging the cost before fetching data from disk, or otherwise making sure that paying just a bit for a call to a big contract isn't exploitable.

Implementation history:

Blockers:
@matklad matklad added C-bug Category: This is a bug A-contract-runtime Area: contract compilation and execution, virtual machines, etc T-contract-runtime Team: issues relevant to the contract runtime team labels Dec 23, 2021
@matklad
Copy link
Contributor Author

matklad commented Jan 4, 2022

related: #4826

@matklad
Copy link
Contributor Author

matklad commented Jan 6, 2022

Note: we also should charge this cost if contract fails to compile (ie, it is invalid wasm)

@matklad matklad changed the title Correct logic around contract_compile Correct logic around contract_compile cost Feb 2, 2022
@jakmeier jakmeier mentioned this issue Feb 25, 2022
Closed
1 task
@jakmeier jakmeier mentioned this issue Mar 15, 2022
Merged
@jakmeier jakmeier mentioned this issue Mar 30, 2022
Merged
@jakmeier jakmeier self-assigned this Apr 1, 2022
@jakmeier
Copy link
Contributor

jakmeier commented Apr 1, 2022

I started working on this. I wonder what the correct place to charge the new contract_loading is. I think ideally, it should be before we actually load the contract. Unfortunately we currently do not have the size of the contract in advance. So I see two practical options.

  1. Simplest approach: Move it to the the start of each implementation of runner::VM::run where it currently is.
  2. Lifting it out of the contract-runtime, charging it before even setting up VMContext.

This was referenced Apr 11, 2022
Merged
Merged
@jakmeier jakmeier mentioned this issue Apr 22, 2022
Merged
@jakmeier
Copy link
Contributor

jakmeier commented Jun 2, 2022

Having merged #6772 also, I consider this issue done.

@jakmeier jakmeier closed this as completed Jun 2, 2022
jakmeier added a commit to jakmeier/nearcore that referenced this issue Jul 12, 2022
@jakmeier
feat: stabilize precharged contract loading gas
6c22afd 
# Feature to stabilize
The gas cost for loading a contract executable used to be charged afterwards. However, the general rule for gas charges is that they should be charged before the cost occurs. This is only noticable when the execution fails during that step. But it is nevertheless important, as an attacker could potentially abuse this to fill up block space without being charged for it.

Note that "loading" in this context is a technical term for preparing and populating all memory regions for an executable. Reading the data from the database is NOT included in this.

On a side note, the actual gas parameter values have not been updated, yet. We know that `wasm_contract_loading_bytes` in particular is too low. But it is counter balanced by `action_function_call` which is higher than it needs to be. Stabilizing this feature paves the floor to adjust those parameters.

Main implementation PR: near#6772
Main issue: near#5962

# Testing and QA
## Tests
The original PR (near#6772) includes tests on the VM level that ensure old versions are unaffeceted while new versions charge the cost correctly.
Integration tests in near#7169 checks the same again but on the apply-block level.

## Impact on existing smart contracts
This change does not affect anything except for function calls that fail during preparation. Since those were already failing, it should not impact anyone if the error type and amount of gas charged changes.

# Checklist
- [ ] TODO Link to nightly nayduck run (`./scripts/nayduck.py`, [docs](https://github.com/near/nearcore/blob/master/nightly/README.md#scheduling-a-run)):  https://nayduck.near.org/
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
@jakmeier jakmeier mentioned this issue Jul 12, 2022
Merged
2 tasks
jakmeier added a commit that referenced this issue Jul 15, 2022
@jakmeier
feat: stabilize precharged contract loading gas (#7177)
f7aca03 
# Feature to stabilize
The gas cost for loading a contract executable used to be charged afterwards. However, the general rule for gas charges is that they should be charged before the cost occurs. This is only noticeable when the execution fails during that step. But it is nevertheless important, as an attacker could potentially abuse this to fill up block space without being charged for it.

Note that "loading" in this context is a technical term for preparing and populating all memory regions for an executable. Reading the data from the database is NOT included in this.

On a side note, the actual gas parameter values have not been updated, yet. We know that `wasm_contract_loading_bytes` in particular is too low. But it is counter balanced by `action_function_call` which is higher than it needs to be. Stabilizing this feature paves the floor to adjust those parameters.

Main implementation PR: #6772
Main issue: #5962

# Testing and QA
## Tests
The original PR (#6772) includes tests on the VM level that ensure old versions are unaffected while new versions charge the cost correctly.
Integration tests in #7169 checks the same again but on the apply-block level.

## Impact on existing smart contracts
This change does not affect anything except for function calls that fail during preparation. Since those were already failing, it should not impact anyone if the error type and amount of gas charged changes.

# Checklist
- [x] Link to nightly nayduck: https://nayduck.near.org/#/run/2562
- [x] Update CHANGELOG.md to include this protocol feature in the `Unreleased` section.
@matklad matklad reopened this Sep 20, 2022
@matklad matklad added the C-tracking-issue Category: a tracking issue label Sep 20, 2022
@matklad matklad changed the title Correct logic around contract_compile cost protocol_feature_fix_contract_loading_cost tracking issue Sep 20, 2022
@jakmeier jakmeier mentioned this issue Sep 30, 2022
Open
@jakmeier
Copy link
Contributor

We also want to reshuffle costs at some point: #7741

Maybe we should release these things together. But we can continue to treat them separately, as the cost changes probably need some more optimizations before we can really move forward with them.

@aborg-dev
Copy link
Contributor

Closing this as obsolete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jakmeier jakmeier

Labels
A-contract-runtime Area: contract compilation and execution, virtual machines, etc C-bug Category: This is a bug C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@aborg-dev @matklad @jakmeier