vm: Improve skipBalance logic

[acolytec3]

Fixes #1839

This addresses the fact that skipBalance in runTx doesn't handle skipping balance checks correctly after the initial transaction options are parsed. Now, if skipBalance is set to true and the calling account balance is less than the transaction cost (i.e. intrinsic gas + value), it sets the balance equal to the transaction cost so the transaction will not fail due to an account having an insufficient balance.

It adds similar logic in runCall that checks to verify that the account has sufficient balance for any value transferred in the call.

[codecov]

Codecov Report

Merging #1849 (c6c47e0) into develop (478fb97) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head c6c47e0 differs from pull request most recent head ddb663f. Consider uploading reports for the commit ddb663f to get more accurate results

Flag	Coverage Δ
block	85.17% <ø> (-0.05%)	⬇️
blockchain	83.06% <ø> (ø)
client	77.45% <ø> (-0.02%)	⬇️
common	94.94% <ø> (ø)
devp2p	82.67% <ø> (-0.14%)	⬇️
ethash	90.71% <ø> (-0.11%)	⬇️
statemanager	84.07% <ø> (-0.06%)	⬇️
trie	80.17% <ø> (+0.02%)	⬆️
tx	92.48% <ø> (-0.05%)	⬇️
util	89.16% <ø> (+1.79%)	⬆️
vm	82.27% <100.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[acolytec3]

I'm not sure if this is the best approach or not but this solves the immediate issue of a call to reduceSenderBalance where the cost is greater than the sender's balance. It does so for skipBalance by ensuring the sender's balance equals the cost of the transaction whatever balance was set so nets out that the sender's balance gets reduced to 0 in runTx and then checks the sender's balance against the message.value in runCall if skipBalance is set to true.

Then, in reduceSenderBalance we throw an error if the balance is negative and this gets handled with an exceptionError message in executeMessage.

[acolytec3]

Okay, I think all of the current feedback has been addressed so I'm going to call this ready for review.

[ryanio]

looks great, thanks for all the requested updates, looks like ci is failing with a nonce off by one error

[acolytec3]

looks great, thanks for all the requested updates, looks like ci is failing with a nonce off by one error

Fixed that last issue. I forgot to add the nonce to the modifyAccountFields method.

[ryanio]

lgtm! since this is rather delicate let's wait for one more 👀 and approval before merging

[ryanio]

this still lgtm :)

[holgerd77]

@ryanio in doubt maybe just merge if you feel good with it. 🙂

[holgerd77]

Rebased this on latest develop.

[holgerd77]

Also had a quick look, this looks good to me as well.

Will merge.