core: check if sender is EOA

[MariusVanDerWijden]

Draft PR after a recent discussion about what would happen if a contract would send a transaction (which is very unlikely, but not impossible)

[holiman]

LGTM. But please also add a test -- we have tests for all core errors in state_transition_test.go, I think. IMO we should discuss this on an ACD and then we can merge it.

[holiman]

Interesting:

--- FAIL: TestBlockchain (0.07s)

    --- FAIL: TestBlockchain/ValidBlocks/VMTests/vmSystemOperations/suicide0.json (0.01s)

        --- FAIL: TestBlockchain/ValidBlocks/VMTests/vmSystemOperations/suicide0.json/suicide0_Berlin (0.00s)

            block_test.go:52: test without snapshotter failed: block #1 insertion into chain failed: could not apply tx 0 [0x4b6bd78c49dbdee90035462999de38c6078f2d3587d902f5970d22773b7da185]: sender not an eoa: address 0xa94f5374Fce5edBC8E2a8697C15331677e6EbF0B, codesize: 17

• quite a lot of more failures. It appears this trick has been used in the tests.

[holiman]

@winsvega it appears that a lot of tests are using this 'feature'. Do you have any idea, off the top of your head, how hard it would be to rewrite the tests that rely on this?

[winsvega]

Not so hard. There are a few tests having sender with code. Many times its not critical for the test logic.

We add new tests that check that it is disallowed

[holiman]

Many times its not critical for the test logic.

I think this change will go forward, so it would be great if, for those cases, we could get started on rewriting them.

[holiman]

Doing a full-sync from 0 on bench04, and a full-sync from 6M on bench03 with this PR.

[holiman]

Doing a full-sync from 0 on bench04, and a full-sync from 6M on bench03 with this PR.

So far, all blocks up to 11M are fine

[holiman]

All good up until 12.36M so far

[holiman]

Confirmed up until current head (12.716M)

[holiman]

I updated to the most recent tests and pushed to this PR, to see if it works now

[holiman]

A lot of errors on old forks -- I suspect the legacy tests needs to be refilled aswell? cc @winsvega

[holiman]

Interesting!
The change from checking code size to comparing against the emptyHash actually caused some new errors internally, in the tests.

    api_test.go:428: test 1: error mismatch, want insufficient funds for transfer, have tracing failed: sender not an eoa: address 0x4b8Ce98202e0f3Ca06F9bA6A1434f9Ab8ad835d3, codehash: 0x0000000000000000000000000000000000000000000000000000000000000000
    api_test.go:432: test 2: want no error, have tracing failed: sender not an eoa: address 0x4b8Ce98202e0f3Ca06F9bA6A1434f9Ab8ad835d3, codehash: 0x0000000000000000000000000000000000000000000000000000000000000000

The codehash, for some reason, isn't set.

[holiman]

Actually, the check for emptyCodeHash is slightly wrong. It uses the same method as EXTCODEHASH uses, however, which has a quirk:

• If the account exists and has no code, the emptyCodeHash is returned
• If the account does not exist, then common.Hash (all zeroes) is returned

Therefore, this PR cannot use that same check. If we process an historical block with a transaction usign 0-gasPrice, it's fully possible that the sender does not previously exist in the state. Thus would have 0x000... codehash, and the block would be rejected.

So the PR as is right now probably doesn't pass a full-sync from geneiss. Since we did a full-sync just to validate this PR, I'd feel safer to revert to the PR as it was when we performed the full-sync.