-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core execution refactor, EIP 684, edge case fixes, starting nonce for morden #19
Conversation
how much is required to finalize this? |
Just debugging the gas usage discrepancy, I can give context on how this is very convoluted in the codebase but it shouldn't be a difficult fix. I was waiting to merge in this PR once I solved the consensus bug, and most (I can't say all yet) of the classic failing tests currently are outdated by EIP684 impl |
…output to be consistent
Removed failing tests since they are both cases where it as assumed a contract collision will overwrite a contract. PR is ready for review now. Solves all state test discrepancies and the post state root hash was matching on the Morden tx now and was just giving a gas usage error for the tx (and tx receipt because of this) so that issue can be assumed to be seperate from this PR, unless the issue is found before this gets merged in. Closes #13 by implementing final difference, EIP684 |
I will look into it early tomorrow but I don't see how this is possible, I have synced up many times with this branch and had just done so before opening PR for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can confirm gets stuck at block 323.
opcodes from the contract creation tx in the next block (pulled from parity) here
https://pastebin.com/Xpa4pMtu
@austinabell @soc1c we're stuck on block 323 because block 324 contains the first contract creation transaction and our EIP684 explicitly checks for a nonce of 0 but accounts on morden start with a nonce of 2^20 |
…or contract collision and empty objects
Good find, I'm resyncing now. |
resolves #22, if anyone else can double check |
4267592
to
04716ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works finally.
Syncing... code checks out |
Maybe rename it to include the starting nonce details/have it in the description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I found only some small "stylistic" issues.
@@ -200,7 +200,7 @@ func runStateTest(ruleSet RuleSet, test VmTest) error { | |||
if balance, ok := new(big.Int).SetString(account.Balance, 0); !ok { | |||
panic("malformed test account balance") | |||
} else if balance.Cmp(obj.Balance()) != 0 { | |||
return fmt.Errorf("(%x) balance failed. Expected: %v have: %v\n", obj.Address().Bytes()[:4], account.Balance, obj.Balance()) | |||
return fmt.Errorf("(%x) balance failed. Expected: %v have: %v\n", obj.Address().Bytes()[:4], account.Balance, "0x"+obj.Balance().Text(16)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can easily add "0x" inside error format string, not as a concatenation in argument.
callerAddr := caller.Address() | ||
ret, _, err = exec(env, caller, &callerAddr, &addr, env.Db().GetCodeHash(addr), input, env.Db().GetCode(addr), gas, gasPrice, value, false) | ||
evm := env.Vm() | ||
// Depth check execution. Fail if we're trying to execute above the limit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This few lines look like a code duplicate, function could be extracted.
ret, _, err = exec(env, caller, &addr, &addr, env.Db().GetCodeHash(addr), input, env.Db().GetCode(addr), gas, gasPrice, new(big.Int), true) | ||
evm := env.Vm() | ||
// Depth check execution. Fail if we're trying to execute above the limit. | ||
if env.Depth() > callCreateDepthMax { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another code duplicate
… morden (#19) * Refactored Call * Refactored CallCode * Refactored DelegateCall * Refactored StaticCall * Added create before contract collision check * Refactored Call * Refactored CallCode * Refactored DelegateCall * Refactored StaticCall * Added create before contract collision check * Fixed state transition logic for edge case * Added create collision logic * Fix for edge case tests * Removed outdated contract collision tests and changed format of test output to be consistent * Rename exec function * utilize StartingNonce from state instead of explicit 0 nonce checks for contract collision and empty objects * Fixes morden seg fault edge case * adding starting nonce to created contracts
* Fixed state transition logic for edge case (#17) * Update morden bootnodes (#16) * Updates bootnodes * what a fantastic test :) * Core execution refactor, EIP 684, edge case fixes, starting nonce for morden (#19) * Refactored Call * Refactored CallCode * Refactored DelegateCall * Refactored StaticCall * Added create before contract collision check * Refactored Call * Refactored CallCode * Refactored DelegateCall * Refactored StaticCall * Added create before contract collision check * Fixed state transition logic for edge case * Added create collision logic * Fix for edge case tests * Removed outdated contract collision tests and changed format of test output to be consistent * Rename exec function * utilize StartingNonce from state instead of explicit 0 nonce checks for contract collision and empty objects * Fixes morden seg fault edge case * adding starting nonce to created contracts
This does not completely solve the issue from #12 but now the discrepancy is just a gas usage differential and the post state root hashes are matching up with the tx. These changes pass all state tests and I will remove the invalid state tests existing in the repository before that expect the incorrect behaviour and investigate the test failures to try to finish off these tests before opening PR for review.