WIP: Gas accounting #676
WIP: Gas accounting #676
Conversation
There was a problem hiding this comment.
Please add a few lines of prose to interpreter/_index.md describing the gas accounting and payment model, with which we can then compare the code semantics. It's hard to back out the desired semantics from the code, but much easier to verify correctness the other way.
Specifically include reference to what happens in cases like the sender doesn't exist, or doesn't have sufficient balance to meet the gas limit etc.
| outTree, r = vmi.ApplyMessage(outTree, m, blk.Miner()) | ||
|
|
||
| TODO() // TODO: include signature length? | ||
| onChainMessageLen := len(msg.Serialize_UnsignedMessage(m)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(deferring pending resolution of #669)
|
|
||
| TODO() | ||
| // TODO: miners shouldn't be able to trigger cron by sending messages. | ||
| // Maybe we need a separate ControlActor for this? |
There was a problem hiding this comment.
This TODO isn't helpful here, please remove it. It belongs in the cron actor and we already have an issue for it.
There was a problem hiding this comment.
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.
| 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( |
There was a problem hiding this comment.
Nit: this needn't be exported, so change to _lowerCase and move below ApplyMessage.
| func (vmi *VMInterpreter_I) ApplyMessage(inTree st.StateTree, message msg.UnsignedMessage, minerAddr addr.Address) ( | ||
| func StateTree_WithGasUsedFundsTransfer( | ||
| tree st.StateTree, gasUsed msg.GasAmount, message msg.UnsignedMessage, | ||
| fundsFrom addr.Address, fundsTo addr.Address) st.StateTree { |
There was a problem hiding this comment.
message is only used to access gas price, so just pass the gas price directly.
| fundsFrom addr.Address, fundsTo addr.Address) st.StateTree { | ||
|
|
||
| TODO() // TODO: what if the miner has insufficient funds? | ||
| // Should we instead aggregate these deltas, and SubtractWhileNonnegative at the end of the tipset? |
There was a problem hiding this comment.
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.
| compTree = _withTransferFundsAssert(compTree, message.From(), addr.BurntFundsActorAddr, maxGasCost) | ||
| // deduct gas limit funds from sender first | ||
| compTreePreSend = _withTransferFundsAssert( | ||
| compTreePreSend, message.From(), addr.BurntFundsActorAddr, gasLimitCost) |
There was a problem hiding this comment.
nit: it would be more clear why this assertion is ok if you moved it up directly below where the balance is checked.
There was a problem hiding this comment.
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.
| 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") | ||
| } |
There was a problem hiding this comment.
It's not now. I recommend removing the error return from WithIncrementedCallSeqNum(). That call can panic instead.
There was a problem hiding this comment.
OK -- will modify the API (WithIncrementedCallSeqNum / WithIncrementedCallSeqNum_Assert).
| ) | ||
| ok = _vmiUseGas(sendRet.GasUsed()) | ||
| if !ok { | ||
| panic("Interpreter error: runtime execution used more gas than provided") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| Message msg.UnsignedMessage | ||
| MessageSizeOrig int | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(deferring pending #669)
| func (rt *VMContext) CreateActor_DeductGas() Runtime_CreateActor_DeductGas_FunRet { | ||
| if !rt._actorAddress.Equals(addr.InitActorAddr) { | ||
| rt.AbortAPI("Only InitActor may call rt.CreateActor_DeductGas") | ||
| } |
There was a problem hiding this comment.
This does not seem like the right place for access control. Nothing bad happens anyway.
There was a problem hiding this comment.
The desired semantics are for only InitActor to be able to call this. Yes, it might be fine to allow this attack surface, but it might not -- the ability to trigger gas consumption interacts with lots of other potentially complicated logic. I don't think we should allow operations along these lines unless there's a good reason to.
| 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) |
There was a problem hiding this comment.
This is just out-of-gas - the caller should pay gas limit and the miner should not be penalized.
|
Merged in #685. |
(continued from #644, separated from other contributions)
Closes #631