take snapshot after gas transfer so its not reverted on method failure

[whyrusleeping]

Should supercede #1410

[arajasek]

Yup, this looks good, and is better than #1410. Looking at the spec, though, I think there might be more nuance to some of the gas calculations, so we might still not be spec-compliant after this PR.

We can either merge this and do a review of compliance later, or hold off on this until we're confident...I don't have a preference.

[whyrusleeping]

chain validation tests failing now... hrm. Need to investigate

[whyrusleeping]

now the propose and approve multisig test is failing with invalid nonce, instead of failing with a multisig specific error. Odd...

[arajasek]

now the propose and approve multisig test is failing with invalid nonce, instead of failing with a multisig specific error. Odd...

This is failing because the test sends 2 failed messages from the same address, both with nonce 0, and expects them to fail as ErrForbidden. This was working because the sender's nonce wasn't being incremented on the first failed message, so we were ErrForbiddening both of them.

With your change, we do increment the nonce after the first failed message, and so we reject #2 as an invalid nonce, which isn't what the test expects.

[Kubuxu]

I think incrementing nonce is the correct thing to do if we charge gas for failed execution.
If we don't increment nonce and charge gas, I can drain your account by including the same invalid message over and over.

[arajasek]

I think incrementing nonce is the correct thing to do if we charge gas for failed execution.
If we don't increment nonce and charge gas, I can drain your account by including the same invalid message over and over.

That's definitely right. I don't know if gasUsed should be the entire gasLimit, though, I don't think that's what the spec says.

[Kubuxu]

AFAIK, for failures we were going to charge everything up to gas limit. If it is different in spec, can you link it?