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

Safe math vm logic #2013

Merged
merged 9 commits into from
Jan 24, 2020
Merged

Safe math vm logic #2013

merged 9 commits into from
Jan 24, 2020

Conversation

evgenykuzyakov
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov commented Jan 23, 2020 •
edited
Loading

  • Refactoring around Errors from VMLogic.
  • Add safe math and InconsistentStateError into VMLogic.
  • Add safety check to Runtime for attached_balance + account.amount.

Ref #1461

@codecov
Copy link

codecov bot commented Jan 23, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (staging@a17a75d). Click here to learn what that means.
The diff coverage is 69.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##             staging    #2013   +/-   ##
==========================================
  Coverage           ?   87.37%           
==========================================
  Files              ?      169           
  Lines              ?    33289           
  Branches           ?        0           
==========================================
  Hits               ?    29085           
  Misses             ?     4204           
  Partials           ?        0
Impacted Files Coverage Δ
runtime/runtime/src/actions.rs 75.29% <100%> (ø)
chain/jsonrpc/tests/rpc_query.rs 98.19% <100%> (ø)
core/primitives/src/views.rs 61.21% <100%> (ø)
chain/chain/src/test_utils.rs 85.79% <100%> (ø)
near/src/runtime.rs 70.99% <12.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a17a75d...7894b00. Read the comment docs.

@evgenykuzyakov evgenykuzyakov requested a review from frol as a code owner January 23, 2020 21:04
frol
frol requested changes Jan 23, 2020
View reviewed changes
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Outdated Show resolved Hide resolved
runtime/near-vm-logic/src/logic.rs Show resolved Hide resolved
runtime/near-vm-logic/tests/test_miscs.rs Outdated Show resolved Hide resolved
runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
MaksymZavershynskyi
MaksymZavershynskyi approved these changes Jan 23, 2020
View reviewed changes
runtime/near-vm-errors/src/lib.rs
StorageError(Vec<u8>),
FunctionCallError(FunctionCallError),
/// Serialized external error from External trait implementation.
ExternalError(Vec<u8>),
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason we are serializing error here (CC @fckt )?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The serialized error is part of RuntimeExt which comes from primitives. Either near-vm-logic has to depend on it, or we have to replicate the error. Right now the plan is to allow any error here but in serialized form.
Later we deserialize it back in the Runtime and get the original error type.

runtime/runtime/src/actions.rs
result.gas_burnt = safe_add_gas(result.gas_burnt, outcome.burnt_gas)?;
result.gas_burnt_for_function_call =
safe_add_gas(result.gas_burnt_for_function_call, outcome.burnt_gas)?;
// Runtime in `generate_refund_receipts` takes care of using proper value for refunds.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding this explanation here.

@weekly-digest weekly-digest bot mentioned this pull request Jan 24, 2020
Closed
@evgenykuzyakov evgenykuzyakov merged commit a08944d into staging Jan 24, 2020
@evgenykuzyakov evgenykuzyakov deleted the safe-math-vm-logic branch January 24, 2020 17:57
@weekly-digest weekly-digest bot mentioned this pull request Jan 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaksymZavershynskyi MaksymZavershynskyi MaksymZavershynskyi approved these changes

@frol frol frol approved these changes

@lexfrl lexfrl Awaiting requested review from lexfrl

@mikhailOK mikhailOK Awaiting requested review from mikhailOK

@bowenwang1996 bowenwang1996 Awaiting requested review from bowenwang1996

@SkidanovAlex SkidanovAlex Awaiting requested review from SkidanovAlex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evgenykuzyakov @frol @MaksymZavershynskyi