VM -> Mainnet: fix Frontier consensus bug along CREATE with not enough gas #1081
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
|
||
| if ( | ||
| !results.execResult.exceptionError || | ||
| results.execResult.exceptionError.error === ERROR.CODESTORE_OUT_OF_GAS |
There was a problem hiding this comment.
This ensures that, IF we have a CODESTORE_OUT_OF_GAS (Frontier only!), we treat it as "if the contract was created" (we know that it was not created). This means that we actually put the "created address" on stack as return value on the call frame we are returning to. (This ensures that this 0x83.. (empty contract) actually gets the 0.03 ether, instead of it being sent to 0x00.., (the value which is put on stack in case we are on Homestead if contract fails to be created)).
| // It does change the state root, but it only wastes storage. | ||
| //await this._state.putContractCode(message.to, result.returnValue) | ||
| const account = await this._state.getAccount(message.to) | ||
| await this._state.putAccount(message.to, account) |
There was a problem hiding this comment.
This puts an empty account.
packages/vm/tests/runner.ts
Outdated
| if (o.opcode.name == 'SSTORE') { | ||
| console.log( | ||
| o.stack[o.stack.length - 1].toString('hex'), | ||
| o.stack[o.stack.length - 2].toString('hex') |
There was a problem hiding this comment.
Logs the slot and the value in case we SSTORE.
In the 0x83 contract, we put 0x00..3c in slot 0x00..02. This should be 0x0000000000000000000000000000000000000000000000000000000055f4f99f.
I explicilty checked on my full node that this storage actually does exist and is not wiped if the code deposit goes out of gas. So, it should be the value above.
There was a problem hiding this comment.
Ah, I wondered during the night if would need/want some per-opcode debugging functionality, this gives me some confirmation 😋 , will see if I can add to #1080.
The we can do something like DEBUG=vm:opcodes:sstore npm run test.ts out of the box. 😄 (maybe this naming is a bit long, will think about that)
There was a problem hiding this comment.
Some other question here out of interest: how do you check this on your full node? 🤔
There was a problem hiding this comment.
In a terminal, geth attach to the current datadir (mainnet). This gives access to web3 methods, especially eth ones which are interesting.
If you call eth.getStorageAt(address, slot) you get the storage at the slot. Note: this checks by default on the latest block, but since we CREATEd the 0x83 address, it is not possible to change the code on this address. Since there is no code deposited, this means that the storage cannot be changed. Hence, by this logic, if we query eth.getStorageAt("0x8398ff6c618e9515468c1c4b198d53666cbe8462", "0x0000000000000000000000000000000000000000000000000000000000000000"), we get the storage value which was deposited in the transaction 0x8420dbe268209c4e921bf6b78bc9fb959d9d9553fd16aacb9dd8cad955dbe729.
If there would be code, then we would not be sure if that value was actually deposited in the transaction (it could have changed. Then we would either query eth.getStorageAt("0x8398ff6c618e9515468c1c4b198d53666cbe8462", "0x0000000000000000000000000000000000000000000000000000000000000000", 226522) (this would be the storage value of the slot after running each transaction of the block (so after our consensus failing transation, i.e. the deposited storage in most cases if no other transaction in the block interacted with the contract), or we would query eth.getStorageAt("0x8398ff6c618e9515468c1c4b198d53666cbe8462", "0x0000000000000000000000000000000000000000000000000000000000000000", 226521), which would be the value after we had processed the earlier block, i.e. the value in the slot right before we start processing the transaction.
Since these blocks are very old, this would usually require that we have an archive node (which I don't have).
There was a problem hiding this comment.
That's super interesting, thanks for the extensive write-up! 😄
Just discovered that Ethers has similar functionality with getStorageAt(), as a side note.
There was a problem hiding this comment.
(getting more and more of an Ethers user finally 😅 )
packages/vm/tests/runner.ts
Outdated
|
|
||
| await vm.runTx({ tx, block }) | ||
| const expectedRoot = Buffer.from( | ||
| '9d735b19daf3205f93e60f3eb02df6b211aebb4d3a2c512cb51055d8ecfb35c1', |
There was a problem hiding this comment.
The root as reported from ethers. Also verified this on my node.
packages/vm/tests/runner.ts
Outdated
| // Ensure we run on the right root | ||
| trie._setRoot( | ||
| Buffer.from('9dceb42e227ee6a244449d1d92cc5e0c17c63b520d0ff050d943baa8055d0ca6', 'hex') | ||
| ) |
There was a problem hiding this comment.
Actually the more official way is doing a stateManager.setStateRoot(Buffer.from('9dceb42e227ee6a244449d1d92cc5e0c17c63b520d0ff050d943baa8055d0ca6', 'hex')) here. 😄
There was a problem hiding this comment.
Ah yes, I realized later on that I should not call methods on the low-level trie, but indeed on the state manager. (This PR is rather dirty ATM - it is a sort of PoC)
packages/vm/tests/runner.ts
Outdated
| }, | ||
| }, | ||
| { common } | ||
| ) |
There was a problem hiding this comment.
Whew. Sophisticated.
We might want to leave some generalized version of this script (with the changing parts as CL arguments or constants at the beginning of the file or something) + some more usages instructions as comments inside either the VM or client test folders.
There was a problem hiding this comment.
As a general comment on the tooling for debugging: it should be built around blocks, not transactions. If we have a block with multiple transactions then if a transaction like the third one fails we should first execute the earlier blocks to get the state right, then execute failing tx, and then the remaining transactions on top. Then finally we check the state root and other things like receipt trie and gas used.
There was a problem hiding this comment.
This is not around something specific, but these will be different debugging modules over time like in the devp2p library (devp2p:rlpx, devp2p:peer and so on), and each module will output the module specific things, so e.g. vm:tx the specifics on the tx run. Of course once can also activate this debug logger when running blocks. I will also add a dedicated one for runBlocks (vm:block), just have started with the tx one. 😄 And doing all at once is just too much, so I will (we can) add over time. Will also give this some documentation mentioning the loggers available.
holgerd77
changed the title
holgerd77
changed the title
holgerd77
changed the title
jochem-brouwer
force-pushed
the
vm-fix-client-consensus-bug
branch
from
2017d54
to
d024e46
Compare
|
Right, the classic "tests fail, but test is wrong" 😅 We might want some dedicated tooling to debug these things, which setup the environment the right way. I forgot to add the timestamp to the block, so when TIMESTAMP is called, it puts 0 on the stack instead of the actual time. |
|
@jochem-brouwer I've just solved this once and for all by adding a new debug option directly to the client which outputs a dynamically generated script out of the box which can be used for debugging. 😄 If you want we can directly merge this in early, then you can rebase here and directly use this. |
|
(so see: #1091 ) |
| } | ||
| const common = new Common({ chain: 'mainnet', hardfork: 'chainstart' }) | ||
| const tx = Transaction.fromTxData(txData, { common }) | ||
| const trie = new Trie(level('/Users/jochem/Library/Ethereum/ethereumjs/mainnet/state/')) |
There was a problem hiding this comment.
We don't have a /Users/jochem/Library/Ethereum/ethereumjs/mainnet/state/ state database on CI so I will remove this test. 😄 Please keep a local copy in case you want to keep.
holgerd77
force-pushed
the
vm-fix-client-consensus-bug
branch
from
52f1e38
to
ab89b59
Compare
|
Yeah, it works!!! 🎉 Great work @jochem-brouwer! 😀 Ready to fill up my (super-limited) SSD again! 😜 |
holgerd77
changed the title
This is related to #1076