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

WIP: Gas accounting #676

Closed
wants to merge 1 commit into from
+306 −118
Closed

WIP: Gas accounting #676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 103 additions & 63 deletions src/systems/filecoin_vm/interpreter/vm_interpreter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ func (vmi *VMInterpreter_I) ApplyTipSetMessages(inTree st.StateTree, msgs TipSet
for _, blk := range msgs.Blocks() {
// Pay block reward.
reward := _makeBlockRewardMessage(outTree, blk.Miner())
outTree, r = vmi.ApplyMessage(outTree, reward, blk.Miner())
outTree, r = vmi.ApplyMessage(outTree, reward, 0, blk.Miner())
if r.ExitCode() != exitcode.OK() {
panic("block reward failed")
}

// Process block miner's Election PoSt.
epost := _makeElectionPoStMessage(outTree, blk.Miner(), msgs.Epoch(), blk.PoStProof())
outTree, r = vmi.ApplyMessage(outTree, epost, blk.Miner())
outTree, r = vmi.ApplyMessage(outTree, epost, 0, blk.Miner())
if r.ExitCode() != exitcode.OK() {
panic("election post failed")
}
Expand All @@ -51,71 +51,129 @@ func (vmi *VMInterpreter_I) ApplyTipSetMessages(inTree st.StateTree, msgs TipSet
if found {
continue
}
outTree, r = vmi.ApplyMessage(outTree, m, blk.Miner())

TODO() // TODO: include signature length?
onChainMessageLen := len(msg.Serialize_UnsignedMessage(m))
Copy link
Member

Choose a reason for hiding this comment

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

You need to adjust the structures to pass the serialized size through from the outside. That size includes the signature for secp messages but not for BLS.

Alternatively, if you want to compute that size here, change BlockMessages to have two message lists, BLSMessages (unsigned) and SECPMessages (signed). You'll then need to extract them into a single list for execution, BLS first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(deferring pending resolution of #669)


outTree, r = vmi.ApplyMessage(outTree, m, onChainMessageLen, blk.Miner())
receipts = append(receipts, r)
seenMsgs[_msgCID(m)] = struct{}{}
}
}

// Invoke cron tick, attributing it to the miner of the first block.

TODO()
// TODO: miners shouldn't be able to trigger cron by sending messages.
// Maybe we need a separate ControlActor for this?
Copy link
Member

Choose a reason for hiding this comment

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

This TODO isn't helpful here, please remove it. It belongs in the cron actor and we already have an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The TODO isn't intended to remain in the code; it's a placeholder to signal that review is needed (and also serves to ensure that we don't accidentally leave the code incomplete). I can replace the question with a link to the issue, though, if helpful.


cronSender := msgs.Blocks()[0].Miner()
cron := _makeCronTickMessage(outTree, cronSender)
outTree, r = vmi.ApplyMessage(outTree, cron, cronSender)
outTree, r = vmi.ApplyMessage(outTree, cron, 0, cronSender)
if r.ExitCode() != exitcode.OK() {
panic("cron tick failed")
}

return
}

func (vmi *VMInterpreter_I) ApplyMessage(inTree st.StateTree, message msg.UnsignedMessage, minerAddr addr.Address) (
func StateTree_WithGasUsedFundsTransfer(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this needn't be exported, so change to _lowerCase and move below ApplyMessage.

tree st.StateTree, gasUsed msg.GasAmount, message msg.UnsignedMessage,
fundsFrom addr.Address, fundsTo addr.Address) st.StateTree {
Copy link
Member

Choose a reason for hiding this comment

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

message is only used to access gas price, so just pass the gas price directly.


TODO() // TODO: what if the miner has insufficient funds?
// Should we instead aggregate these deltas, and SubtractWhileNonnegative at the end of the tipset?
Copy link
Member

Choose a reason for hiding this comment

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

In almost all cases the miner is receiving funds, so this is confusing. Do you mean sender?

This may not fail in any situation, so cap the funds to be transferred at the sender's balance at some point. That probably means loading the miner actor's state (as well as the sender's). If you think this is insufficient, let's discuss approaches elsewhere.

The prose already written explicitly answers the final question: the desired semantics are immediate availability.


return _withTransferFundsAssert(
tree,
fundsFrom,
fundsTo,
_gasToFIL(gasUsed, message.GasPrice()),
)
}

func (vmi *VMInterpreter_I) ApplyMessage(
inTree st.StateTree, message msg.UnsignedMessage, onChainMessageSize int, minerAddr addr.Address) (
st.StateTree, msg.MessageReceipt) {

compTree := inTree
var outTree st.StateTree
var toActor actor.ActorState
var err error
vmiGasRemaining := message.GasLimit()

_applyReturn := func(
tree st.StateTree, invocOutput msg.InvocOutput, exitCode exitcode.ExitCode,
gasUsedFrom addr.Address, gasUsedTo addr.Address) (st.StateTree, msg.MessageReceipt) {

vmiGasUsed := message.GasLimit().Subtract(vmiGasRemaining)
tree = StateTree_WithGasUsedFundsTransfer(tree, vmiGasUsed, message, gasUsedFrom, gasUsedTo)
return tree, msg.MessageReceipt_Make(invocOutput, exitCode, vmiGasUsed)
}

fromActor := compTree.GetActorState(message.From())
_applyError := func(
tree st.StateTree, errExitCode exitcode.SystemErrorCode,
gasUsedFrom addr.Address, gasUsedTo addr.Address) (st.StateTree, msg.MessageReceipt) {

return _applyReturn(
tree, msg.InvocOutput_Make(nil), exitcode.SystemError(errExitCode), gasUsedFrom, gasUsedTo)
}

_vmiUseGas := func(amount msg.GasAmount) (vmiUseGasOK bool) {
vmiGasRemaining, vmiUseGasOK = vmiGasRemaining.SubtractWhileNonnegative(amount)
return
}

ok := _vmiUseGas(gascost.OnChainMessage(onChainMessageSize))
if !ok {
return _applyError(inTree, exitcode.OutOfGas, minerAddr, addr.BurntFundsActorAddr)
Copy link
Member

Choose a reason for hiding this comment

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

  1. These _applyX lines are a bit opaque. Please add a line comment describing the intent of each. E.g. this might read // If sender is out of gas, the miner pays (by burning the funds)

  2. Out of gas is not a case where the miner is penalised. The caller pays up to their gas limit. This should be moved below where the actor state is loaded so we can then talk about whether the sender has enough to pay.

If you don't like the idea that a message could go on chain without paying at least for its weight, then we can add a message syntactic validation rule for a minimum gas limit (and then treat the issue like any other out-of-gas here). But I don't think we need to: the miner is forgoing the gas reward.

}

fromActor := inTree.GetActorState(message.From())
if fromActor == nil {
// TODO: This was originally exitcode.InvalidMethod; which is correct?
return inTree, _applyError(exitcode.ActorNotFound)
return _applyError(inTree, exitcode.ActorNotFound, minerAddr, addr.BurntFundsActorAddr)
}

// make sure fromActor has enough money to run the max invocation
maxGasCost := _gasToFIL(message.GasLimit(), message.GasPrice())
totalCost := message.Value() + actor.TokenAmount(maxGasCost)
// make sure fromActor has enough money to run with the specified gas limit
gasLimitCost := _gasToFIL(message.GasLimit(), message.GasPrice())
totalCost := message.Value() + actor.TokenAmount(gasLimitCost)
if fromActor.Balance() < totalCost {
return inTree, _applyError(exitcode.InsufficientFunds_System)
return _applyError(inTree, exitcode.InsufficientFunds_System, minerAddr, addr.BurntFundsActorAddr)
}

// make sure this is the right message order for fromActor
// (this is protection against replay attacks, and useful sequencing)
if message.CallSeqNum() != fromActor.CallSeqNum()+1 {
return inTree, _applyError(exitcode.InvalidCallSeqNum)
return _applyError(inTree, exitcode.InvalidCallSeqNum, minerAddr, addr.BurntFundsActorAddr)
}

// WithActorForAddress may create new account actors
compTree, toActor = compTree.Impl().WithActorForAddress(message.To())
compTreePreSend := inTree
compTreePreSend, toActor := compTreePreSend.Impl().WithActorForAddress(message.To())
if toActor == nil {
return inTree, _applyError(exitcode.ActorNotFound)
return _applyError(inTree, exitcode.ActorNotFound, message.From(), addr.BurntFundsActorAddr)
}

// deduct maximum expenditure gas funds first
compTree = _withTransferFundsAssert(compTree, message.From(), addr.BurntFundsActorAddr, maxGasCost)
// deduct gas limit funds from sender first
compTreePreSend = _withTransferFundsAssert(
compTreePreSend, message.From(), addr.BurntFundsActorAddr, gasLimitCost)
Copy link
Member

Choose a reason for hiding this comment

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

nit: it would be more clear why this assertion is ok if you moved it up directly below where the balance is checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I think this would cause more confusion due to the out-of-order state tree threading. But I'll add a comment explaining why the assertion is OK.


// Increment sender call sequence number.
var err error
compTreePreSend, err = compTreePreSend.Impl().WithIncrementedCallSeqNum(message.From())
if err != nil {
// Note: if actor deletion is possible at some point, may need to allow this case
panic("Internal interpreter error: failed to increment call sequence number")
}
Copy link
Member

Choose a reason for hiding this comment

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

It's not now. I recommend removing the error return from WithIncrementedCallSeqNum(). That call can panic instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK -- will modify the API (WithIncrementedCallSeqNum / WithIncrementedCallSeqNum_Assert).


rt := vmr.VMContext_Make(
message.From(),
minerAddr, // TODO: may not exist? (see below)
minerAddr,
fromActor.CallSeqNum(),
actor.CallSeqNum(0),
compTree,
compTreePreSend,
message.From(),
actor.TokenAmount(0),
message.GasLimit(),
vmiGasRemaining,
)

sendRet, sendRetStateTree := rt.SendToplevelFromInterpreter(
sendRet, compTreePostSend := rt.SendToplevelFromInterpreter(
msg.InvocInput_Make(
message.To(),
message.Method(),
Expand All @@ -124,51 +182,33 @@ func (vmi *VMInterpreter_I) ApplyMessage(inTree st.StateTree, message msg.Unsign
),
)

if !sendRet.ExitCode().AllowsStateUpdate() {
// error -- revert all state changes -- ie drop updates. burn used gas.
outTree = inTree
outTree = _withTransferFundsAssert(
outTree,
message.From(),
addr.BurntFundsActorAddr,
_gasToFIL(sendRet.GasUsed(), message.GasPrice()),
)
} else {
// success -- refund unused gas
outTree = sendRetStateTree
refundGas := message.GasLimit() - sendRet.GasUsed()
TODO() // TODO: assert refundGas is nonnegative
outTree = _withTransferFundsAssert(
outTree,
addr.BurntFundsActorAddr,
message.From(),
_gasToFIL(refundGas, message.GasPrice()),
)
ok = _vmiUseGas(sendRet.GasUsed())
if !ok {
panic("Interpreter error: runtime execution used more gas than provided")
Copy link
Member

Choose a reason for hiding this comment

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

This is not a panic, this is just out of gas (exceeded the pre-set limit). Before execution, it's impossible to know how much will be actually consumed. An efficient implementation will of course halt execution before this point, but IMO the spec doesn't need to. In any case, the semantics are well defined: pay all the gas and revert state changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be impossible for the runtime execution to use more gas than provided -- as specced, it uses SubtractWhileNonnegative internally, and bails as soon as the gas usage would exceed the limit (without actually exceeding it).

}

outTree, err = outTree.Impl().WithIncrementedCallSeqNum(message.To())
if err != nil {
// TODO: if actor deletion is possible at some point, may need to allow this case
panic("Internal interpreter error: failed to increment call sequence number")
ok = _vmiUseGas(gascost.OnChainReturnValue(sendRet.ReturnValue()))
if !ok {
return _applyError(compTreePreSend, exitcode.OutOfGas, addr.BurntFundsActorAddr, minerAddr)
Copy link
Member

Choose a reason for hiding this comment

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

This is just out-of-gas - the caller should pay gas limit and the miner should not be penalized.

}

compTreeRet := compTreePreSend
if sendRet.ExitCode().AllowsStateUpdate() {
compTreeRet = compTreePostSend
}

// reward miner gas fees
outTree = _withTransferFundsAssert(
outTree,
// Refund unused gas to sender.
refundGas := vmiGasRemaining
compTreeRet = _withTransferFundsAssert(
compTreeRet,
addr.BurntFundsActorAddr,
minerAddr, // TODO: may not exist
_gasToFIL(sendRet.GasUsed(), message.GasPrice()),
message.From(),
_gasToFIL(refundGas, message.GasPrice()),
)

return outTree, sendRet
}

func _applyError(errCode exitcode.SystemErrorCode) msg.MessageReceipt {
// TODO: should this gasUsed value be zero?
// If nonzero, there is not guaranteed to be a nonzero gas balance from which to deduct it.
gasUsed := gascost.ApplyMessageFail
TODO()
return msg.MessageReceipt_MakeSystemError(errCode, gasUsed)
return _applyReturn(
compTreeRet, msg.InvocOutput_Make(sendRet.ReturnValue()), sendRet.ExitCode(),
addr.BurntFundsActorAddr, minerAddr)
}

func _withTransferFundsAssert(tree st.StateTree, from addr.Address, to addr.Address, amount actor.TokenAmount) st.StateTree {
Expand Down
5 changes: 5 additions & 0 deletions src/systems/filecoin_vm/interpreter/vm_interpreter.id
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ type TipSetMessages struct {
Epoch UInt64 // The chain epoch of the blocks
}

type MessageRef struct {
Message msg.UnsignedMessage
MessageSizeOrig int
}

Copy link
Member

Choose a reason for hiding this comment

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

See my comment in vm_interpreter.go.

You could replace the array elements in BlockMessages.Messages with this, but it's probably better to represent the BLS and SECP messages explicitly, so this spec can also describe which order they are evaluated in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(deferring pending #669)

type VMInterpreter struct {
ApplyTipSetMessages(inTree st.StateTree, msgs TipSetMessages) struct {outTree st.StateTree, ret [msg.MessageReceipt]}
ApplyMessage(inTree st.StateTree, msg msg.Message, minerAddr addr.Address) struct {outTree st.StateTree, ret msg.MessageReceipt}
Expand Down
46 changes: 39 additions & 7 deletions src/systems/filecoin_vm/message/message.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,20 +72,52 @@ func Verify(message SignedMessage, publicKey filcrypto.PublicKey) (UnsignedMessa
return message.Message(), nil
}

func (x GasAmount) Add(y GasAmount) GasAmount {
panic("TODO")
func (x *GasAmount_I) Add(y GasAmount) GasAmount {
IMPL_FINISH()
panic("")
}

func (x *GasAmount_I) Subtract(y GasAmount) GasAmount {
IMPL_FINISH()
panic("")
}

func (x GasAmount) Subtract(y GasAmount) GasAmount {
panic("TODO")
func (x *GasAmount_I) SubtractWhileNonnegative(y GasAmount) (ret GasAmount, ok bool) {
ret = x.Subtract(y)
ok = true
if ret.LessThan(GasAmount_Zero()) {
ret = GasAmount_Zero()
ok = false
}
return
}

func (x GasAmount) LessThan(y GasAmount) bool {
panic("TODO")
func (x *GasAmount_I) LessThan(y GasAmount) bool {
IMPL_FINISH()
panic("")
}

func (x *GasAmount_I) Equals(y GasAmount) bool {
IMPL_FINISH()
panic("")
}

func (x *GasAmount_I) Scale(count int) GasAmount {
IMPL_FINISH()
panic("")
}

func GasAmount_Affine(b GasAmount, x int, m GasAmount) GasAmount {
return b.Add(m.Scale(x))
}

func GasAmount_Zero() GasAmount {
panic("TODO")
return GasAmount_FromInt(0)
}

func GasAmount_FromInt(x int) GasAmount {
IMPL_FINISH()
panic("")
}

func InvocInput_Make(to addr.Address, method actor.MethodNum, params actor.MethodParams, value actor.TokenAmount) InvocInput {
Expand Down
11 changes: 10 additions & 1 deletion src/systems/filecoin_vm/message/message.id
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,16 @@ import actor "github.com/filecoin-project/specs/systems/filecoin_vm/actor"
import exitcode "github.com/filecoin-project/specs/systems/filecoin_vm/runtime/exitcode"

// GasAmount is a quantity of gas.
type GasAmount UVarint
type GasAmount struct {
value BigInt

Add(GasAmount) GasAmount
Subtract(GasAmount) GasAmount
SubtractWhileNonnegative(GasAmount) (ret GasAmount, ok bool)
LessThan(GasAmount) bool
Equals(GasAmount) bool
Scale(int) GasAmount
}

// GasPrice is a Gas-to-FIL cost
type GasPrice actor.TokenAmount
Expand Down
Loading