Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Add gas to test vm #1408

Merged
merged 2 commits into from
May 19, 2021
Merged

Add gas to test vm #1408

merged 2 commits into from
May 19, 2021

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented May 11, 2021

Closes #1266

I'm aiming for correctness to filecoin protocol / lotus vm now but it is not dangerous if we miss something in review. All inconsistencies externalized in test vectors will register as failures when ran against lotus (which I will then debug and fix)
before releasing them as conformance tests.

@ZenGround0 ZenGround0 requested review from arajasek and anorth May 11, 2021 18:55
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #1408 (d02c103) into master (5114581) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1408   +/-   ##
======================================
  Coverage    69.9%   69.9%           
======================================
  Files          72      72           
  Lines        7773    7773           
======================================
  Hits         5438    5438           
  Misses       1444    1444           
  Partials      891     891           

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

Basically smooth, nice work.

gen/gen.go Outdated Show resolved Hide resolved
support/agent/sim.go Outdated Show resolved Hide resolved
support/vm/invocation_context.go Outdated Show resolved Hide resolved
support/vm/invocation_context.go Outdated Show resolved Hide resolved
support/vm/invocation_context.go Outdated Show resolved Hide resolved
return ic.Syscalls().VerifySeal(vi)
}

func (ic *invocationContext) BatchVerifySeals(vis map[address.Address][]proof.SealVerifyInfo) (map[address.Address][]bool, error) {
// no explicit gas charged
Copy link
Member

Choose a reason for hiding this comment

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

See #1381 (comment), I hope we can remove the special case.

support/vm/vm.go Outdated
To: to,
Nonce: nonce,
Value: value,
GasLimit: 5_000_000_000,
Copy link
Member

Choose a reason for hiding this comment

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

Reference the default gas limit constant, or use a different value (since I don't think this value is actually checked).

Value: value,
GasLimit: 5_000_000_000,
GasFeeCap: big.Zero(),
GasPremium: big.Zero(),
Copy link
Member

Choose a reason for hiding this comment

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

I was guessing that the GasLimit above was set non-zero so as to better represent the serialized size of a chain message. If so, these should be non-zero too. If not, just set the unused GasLimit to zero to match

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes GasLimit is set non-zero for serialization accuracy purposes. We can't set it to zero because then we will hit SysErrOutOfGas immediately. If we set GasFeeCap and GasPremium non-zero we will have to implement base fee burning and miner payments which I want to avoid.

actors/runtime/runtime.go Outdated Show resolved Hide resolved
if err != nil {
ic.rt.Abortf(exitcode.ErrIllegalState, "could not save new state")
}
c := ic.StorePut(obj)
actr.Head = c
err = ic.rt.setActor(ic.rt.ctx, ic.msg.to, actr)
Copy link
Member

Choose a reason for hiding this comment

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

We need to charge gas for the write of the actor struct into the state tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentionally not charging gas because as I understand the lotus vm code the filecoin protocol does not charge gas for reads and writes from the top level state tree.

@ZenGround0 ZenGround0 force-pushed the spike/aggregate-porep branch from 4aaaf11 to e638752 Compare May 13, 2021 14:13
@BigLep BigLep linked an issue May 13, 2021 that may be closed by this pull request
@arajasek
Copy link
Collaborator

Broadly looks good. Will look in a bit more detail after a rebase (it's currently a bit confusing to look at diff).

@ZenGround0 ZenGround0 force-pushed the spike/aggregate-porep branch 2 times, most recently from f41487f to 5f76287 Compare May 14, 2021 14:25
@ZenGround0 ZenGround0 changed the base branch from spike/aggregate-porep to master May 18, 2021 17:47
- introduce runtime.Pricelist - the price list interface
- copy current filecoin pricelist to testing package
- charge for gas
- return (and ignore) gas used from ApplyMessage
- setup large default gas limit
- handle out of gas aborts
@ZenGround0 ZenGround0 merged commit 6ed06ce into master May 19, 2021
@ZenGround0
Copy link
Contributor Author

Since we are all quite busy right now I went ahead and merged to unblock myself more quickly because existing review was thorough, this PR only impacts testing code, and gas calculation implementation errors will only become more apparent as we evaluate test vectors against lotus.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gas accounting to scenario test VM
4 participants