[workaround] Merge #676 (gas accounting) with correct branch #685

jzimmerman · 2019-11-30T18:16:25Z

Adds model for gas accounting. Closes #631.

anorth · 2019-12-02T09:31:46Z

This PR has two commits from #684, I'll review only the new commit (gas accounting)

anorth

Minor requests. Pre-emptive approval if you complete the un/signed message size costs in interpreter and agree with my other suggestions, otherwise please ping me for discussion or a final pass.

anorth · 2019-12-02T09:41:28Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

-		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)


~~It is an error to use _applyError any time after line 145, because the gas limit will be burned rather than any gas paid to the miner or refunded to the sender.~~

In this case the sender should pay the miner for message execution (i.e. gas actually consumed to this point) but then fail with out-of-gas. We could possibly simplify all out-of-gas exit codes to always consume the full gas limit, even though in this case the return value bytes that tipped over the limit won't in fact go on chain.

Oooh, I think I get it, but the control flow with these closures is really hard to follow. This needs a big comment.

// In compTreePreSend payment for the gas limit has been transferred to burnt funds actor. // When the return value serialization overflows the gas limit, pay the full balance to the miner, // refunding none to the sender (even though the offending return-value won't go on-chain), and then fail.

anorth · 2019-12-02T09:46:10Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

-		return inTree, _applyError(exitcode.ActorNotFound)
+		// Execution error; receiver actor does not exist (and could not be implicitly created)
+		// at time of message execution.
+		return _applyError(inTree, exitcode.ActorNotFound, message.From(), addr.BurntFundsActorAddr)


Suggested change

return _applyError(inTree, exitcode.ActorNotFound, message.From(), addr.BurntFundsActorAddr)

return _applyError(inTree, exitcode.ActorNotFound, minerAddr, addr.BurntFundsActorAddr)

Maybe you intentionally meant the sender to be paying here? In that case, they must pay the miner not burn the funds.

Yes, the intent is for the sender to pay here, since the sender should know in advance whether the receiver they're trying to send to exists.

anorth · 2019-12-02T09:55:49Z

src/systems/filecoin_vm/interpreter/vm_interpreter.id

+type MessageRef struct {
+    Message          msg.UnsignedMessage
+    MessageSizeOrig  int
+}


src/systems/filecoin_vm/interpreter/vm_interpreter.go

anorth

Sorry, I need to retract the optimistic approval. A problem with the "miner pays" scheme: the miner actor will have no funds.

Funds are held by the miner's worker and owner account actors. The miner actor does not receive the block reward or gas fees, nor hold collateral on balance. I think we need to look up the miner's owner address and implicitly transfer to/from that.

Another note - this miner pays scheme will be quite opaque when observing state changes from the outside: the gas amount receipt usually means an amount paid by the sender. I think we should make a clear set of exit codes that uniquely and always mean that the gas was paid by the miner rather than the sender.

anorth · 2019-12-02T11:27:22Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

-	return outTree, sendRet
+	return _applyReturn(
+		compTreeRet, vmr.InvocOutput_Make(sendRet.ReturnValue()), sendRet.ExitCode(),
+		addr.BurntFundsActorAddr, minerAddr)


Blocking: the miner address is the wrong place to send funds. The fees go to the miner actor's owner address (along with the block reward).

anorth · 2019-12-02T11:30:51Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+	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?


Blocking: please resolve this in this PR.

Note that the miner (owner) will just have been paid the block reward before processing any other messages, so can be penalised at least up to that amount. But this code should still define a state transition that will succeed.

anorth

Approved subject to changes responding to my non-nit comments here.

It's a bit odd to me that gas payments to the miner happen immediately with each message, but gas penalties from the miner happen only once at the end.

I also realise that the miner's penalty is also determined by the gas price of the failing message, which was set by the sender. I can't think of a better option, but it's not obvious. I really wish we had a written-up rationale of this to critique independent of the code.

The miner has some chance at escaping the penalty by emptying their owner account in the final message in their block (but would need to guess the balance exactly to escape entirely). This situation wouldn't be any better if the penalty was paid message-at-a-time though - the miner would empty the account before other messages instead.

This is good enough to merge and move on with though. I think we should file an issue with some of this discussion, which we may have time to return to at some point. It seems like a complete solution would require garnishing the block reward (by paying it at the end of the block instead of the beginning).

anorth · 2019-12-03T10:16:57Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+	ok := _vmiUseGas(gascost.OnChainMessage(onChainMessageSize))
+	if !ok {
+		// Invalid message; insufficient gas limit to pay for the on-chain message size.
+		_applyError(inTree, exitcode.OutOfGas, nil, nil, MinerPenaltySpec_ChargeUsedGas)


This will result in zero penalty because used gas is zero (b/c SubtractWhileNonNegative failed). The miner needs to pay more than the gas limit here: the actual message size cost.

anorth · 2019-12-03T10:19:20Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

-	var err error
+	_applyReturn := func(
+		tree st.StateTree, invocOutput vmr.InvocOutput, exitCode exitcode.ExitCode,
+		gasUsedFrom *addr.Address, gasUsedTo *addr.Address, minerPenaltySpec MinerPenaltySpec) {


Nit: This method is either called with final parameters (..., nil, nil, MinerPenaltySpec_ChargeUsedGas) or non-nils and MinerPenaltySpec_ChargeUsedGas, so it would be less noisy and easier to get right if there were separate methods and no penalty spec type.

anorth · 2019-12-03T10:40:31Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+		// When the return value serialization overflows the gas limit, pay the full balance to the miner,
+		// refunding none to the sender (even though the offending return-value won't go on-chain), and then fail.
+		_applyError(compTreePreSend, exitcode.OutOfGas,
+			addr.BurntFundsActorAddr.Ref(), minerAddr.Ref(), MinerPenaltySpec_None)


This still isn't quite right. compTreePreSend has the sender paying gas, but the CallSeqNum is updated only in compTreePostSend. These two things should come together: the actor pays gas if and only if its CallSeqNum is updated.

I think this is made difficult by the CallSeqNum bump happening inside _applyMessageInternal: it would be easier to get right if it was pulled up here to line 192 so that both state changes happened together, or not at all.

Alternatively, the contract for _applyMessageInternal should be to return the input tree if the exit code is nonzero, and then there'll be no need to hold on to the pre-send one here, or the exit code check below.

anorth · 2019-12-03T10:45:55Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+
+func _applyMessageBuiltinAssert(tree st.StateTree, message msg.UnsignedMessage, topLevelBlockWinner addr.Address) st.StateTree {
+	gasRemainingInit := msg.GasAmount_SentinelUnlimited()
+	Assert(gasRemainingInit.Equals(message.GasLimit()))


Just use message.GasLimit? I don't understand why a sentinel or "unlimited" is involved, this seems to be duplicating a concept that is already expressed at the builtin message construction.

This is due to the fact that it's a system invocation and we should never allow it to run out of gas (moreover, it's not clear who we would charge the gas to, if it did).

Hard disagree. We have to put an actual number into the real computer! There should be a real limit and our blockchain should halt if it's exceeded (it's gunna halt anyway), and we can fix it at the social consensus layer of humans.

Please pick a number after which "unlimited" runs out, preferably by restoring the constants I had at the top of this file. It can be large enough to be practically "never", but you can't IMPL_TODO explicitly dodging this.

anorth · 2019-12-03T10:47:16Z

src/systems/filecoin_vm/message/message.go

+
+func GasAmount_SentinelUnlimited() GasAmount {
+	// Amount of gas larger than any feasible execution; meant to indicated unlimited gas
+	// (e.g., for builtin system method invocations).


I don't think we need this. We should set a real limit based on expected max cost, placeholders for which are already in the interpreter.

(see above)

anorth · 2019-12-03T10:50:17Z

src/systems/filecoin_vm/state_tree/state_tree.go

@@ -30,6 +33,18 @@ func (st *StateTree_I) GetActorState(a addr.Address) actor.ActorState {
 	panic("TODO")
 }

+// Get the owner account address associated to a given miner actor.
+func (st *StateTree_I) GetMinerOwnerAddress(a addr.Address) (addr.Address, error) {


Convenient as this looks, it's a huge dependency inversion to have the state tree depending on the miner actor substate. It's not obvious because the absent implementation means you didn't have to do an import. This method sends the wrong signal about the dependency relationships.

Please don't commit this: add a helper function (unimplemented) in the interpreter instead (which still has a circular dep problem, but at least at a less deep level). See the existing commented-out calls to loadActorSubstate.

The Go dependency graph is a bit difficult to resolve here, but I've tentatively moved it to StorageMinerActor for now.

anorth · 2019-12-03T10:50:53Z

src/systems/filecoin_vm/state_tree/state_tree.id

@@ -25,6 +25,10 @@ type StateTree struct {

    GetActorState(a addr.Address) actor.ActorState

+    // Get the owner account address associated to a given miner actor.
+    GetMinerOwnerAddress(a addr.Address) (addr.Address, error)
+    GetMinerOwnerAddress_Assert(a addr.Address) addr.Address


See note above: these don't belong here.

src/systems/filecoin_vm/interpreter/vm_interpreter.go

anorth · 2019-12-04T10:16:35Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+		vmiGasUsedPre := vmiGasUsed
+		vmiBurnGasOK = _vmiAllocGas(amount)
+		if !vmiBurnGasOK {
+			Assert(vmiGasRemaining.Equals(msg.GasAmount_Zero()))


That's not how SubtractIfNonnegative works. vmiGasRemaining will be its prior value, not zero.

anorth · 2019-12-04T10:27:17Z

src/systems/filecoin_vm/interpreter/vm_interpreter.go

+
+func _applyMessageBuiltinAssert(tree st.StateTree, message msg.UnsignedMessage, topLevelBlockWinner addr.Address) st.StateTree {
+	gasRemainingInit := msg.GasAmount_SentinelUnlimited()
+	Assert(gasRemainingInit.Equals(message.GasLimit()))


Hard disagree. We have to put an actual number into the real computer! There should be a real limit and our blockchain should halt if it's exceeded (it's gunna halt anyway), and we can fix it at the social consensus layer of humans.

Please pick a number after which "unlimited" runs out, preferably by restoring the constants I had at the top of this file. It can be large enough to be practically "never", but you can't IMPL_TODO explicitly dodging this.

anorth · 2019-12-04T10:29:26Z

src/systems/filecoin_vm/sysactors/init_actor.go

@@ -72,6 +72,8 @@ func (a *InitActorCode_I) Exec(rt Runtime, codeID actor.CodeID, constructorParam
 		rt.AbortArgMsg("cannot exec an actor of this type")
 	}

+	rt.CreateActor_DeductGas()


It already calls rt.CreateActor: move the charge there and simplify the Runtime interface.

mishmosh · 2019-12-04T17:17:10Z

@jzimmerman saw that you pushed some changes. Does that address all of @anorth's comments so far; is it ready for re-review? (Please explicitly add a comment pinging your reviewer(s) in the future.)

anorth · 2019-12-05T15:09:51Z

To follow up:

update the prose for tipset message execution to match the new ordering of block reward last and garnishing of penalty gas fees
restore actual numbers for implicit message gas limits
maybe try to restructure for legibility.

jzimmerman requested a review from anorth November 30, 2019 18:16

jzimmerman self-assigned this Nov 30, 2019

anorth mentioned this pull request Dec 2, 2019

Remove Message union, specifying SignedMessage or UnsignedMessage explicitly. #669

Merged

anorth approved these changes Dec 2, 2019

View reviewed changes

anorth requested changes Dec 2, 2019

View reviewed changes

jzimmerman force-pushed the tmp-merge-pr-676 branch 2 times, most recently from a27be13 to 229d989 Compare December 2, 2019 19:47

anorth approved these changes Dec 3, 2019

View reviewed changes

jzimmerman force-pushed the tmp-merge-pr-676 branch from 229d989 to 6caa755 Compare December 3, 2019 13:58

mishmosh changed the title ~~[workaround] Temporary PR to merge #676 with correct branch~~ [workaround] Merge #676 (gas accounting) with correct branch Dec 3, 2019

jzimmerman force-pushed the tmp-merge-pr-676 branch 2 times, most recently from 2278eff to 3b25eee Compare December 4, 2019 00:55

anorth reviewed Dec 4, 2019

View reviewed changes

jzimmerman force-pushed the tmp-merge-pr-676 branch from 3b25eee to ed7cf39 Compare December 4, 2019 15:32

Gas accounting

54ee7c9

jzimmerman force-pushed the tmp-merge-pr-676 branch from ed7cf39 to 54ee7c9 Compare December 5, 2019 14:50

anorth merged commit d3248ea into master Dec 5, 2019

jzimmerman mentioned this pull request Dec 5, 2019

WIP: Gas accounting #676

Closed

mishmosh mentioned this pull request Jan 24, 2020

state machine spec is out of date re: gas #79

Closed

hugomrdias deleted the tmp-merge-pr-676 branch July 5, 2020 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[workaround] Merge #676 (gas accounting) with correct branch #685

[workaround] Merge #676 (gas accounting) with correct branch #685

jzimmerman commented Nov 30, 2019 •

edited by mishmosh

Loading

anorth commented Dec 2, 2019

anorth left a comment

anorth Dec 2, 2019

anorth Dec 2, 2019

jzimmerman Dec 2, 2019

anorth Dec 2, 2019

anorth left a comment

anorth Dec 2, 2019

anorth Dec 2, 2019

anorth left a comment

anorth Dec 3, 2019

anorth Dec 3, 2019

anorth Dec 3, 2019

anorth Dec 3, 2019

jzimmerman Dec 4, 2019

anorth Dec 4, 2019

anorth Dec 3, 2019

jzimmerman Dec 4, 2019

anorth Dec 3, 2019

jzimmerman Dec 4, 2019

anorth Dec 3, 2019

anorth Dec 4, 2019

anorth Dec 4, 2019

anorth Dec 4, 2019

mishmosh commented Dec 4, 2019

anorth commented Dec 5, 2019

	return _applyError(inTree, exitcode.ActorNotFound, message.From(), addr.BurntFundsActorAddr)
	return _applyError(inTree, exitcode.ActorNotFound, minerAddr, addr.BurntFundsActorAddr)

[workaround] Merge #676 (gas accounting) with correct branch #685

[workaround] Merge #676 (gas accounting) with correct branch #685

Conversation

jzimmerman commented Nov 30, 2019 • edited by mishmosh Loading

anorth commented Dec 2, 2019

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mishmosh commented Dec 4, 2019

anorth commented Dec 5, 2019

jzimmerman commented Nov 30, 2019 •

edited by mishmosh

Loading