Skip to content
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

Tests for EIP2929 operations on precompiles #829

Merged
merged 3 commits into from
Apr 21, 2021
Merged

Conversation

qbzzt
Copy link
Collaborator

@qbzzt qbzzt commented Apr 20, 2021

Check which low address contracts (precompiles) are part of the EIP2929 access lists. This was the Berlin fork OpenEthereum bug (see https://twitter.com/ralexstokes/status/1382750525070868484 and https://twitter.com/ralexstokes/status/1382750938142629889).

According to EIP2929 (https://eips.ethereum.org/EIPS/eip-2929#abstract) precompiles are exempt from the opcode cost increases for the first (cold) access, they are always hot. However, this only happens with some operations in geth:

  1. If you send WEI to a precomp with a CALL, it acts as if the precomp is cold. This is true also if you send or receive data in the call. However, if you send the WEI with CALLCODE, it works normally.
  2. Accounts 1-9 act cold for EXTCODECOPY

The expected results in this test are based on observed geth behavior.

Fixed by using a constant amount to gas as @holiman suggested:

  1. If you send data, 1-7 act hot, 8 acts cold. This is true for CALL, CALLLCODE, DELEGATECALL, and STATICCALL
  2. Account 9 acts cold for calls (all four types, all variants I tested), but acts hot for BALANCE, EXTCODEHASH, and EXTCODESIZE

@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label Apr 20, 2021
@qbzzt qbzzt removed the ready-to-merge PR is ready to merge and not WIP label Apr 20, 2021
@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

If you send WEI to a precomp with a CALL, it acts as if the precomp is cold. This is true also if you send or receive data in the call. However, if you send the WEI with CALLCODE, it works normally.

That can't be right. I'll look into it

If you send data, 1-7 act hot, 8 acts cold. This is true for CALL, CALLLCODE, DELEGATECALL, and STATICCALL

I don't think this is rirght. I assume that 8 simply rejects your input.

Account 9 acts cold for calls (all four types, all variants I tested), but acts hot for BALANCE, EXTCODEHASH, and EXTCODESIZE

I think it rejects your input. It's blake2f, it requires exactly 213 bytes of input to not error.

1-7 have always been precompiles

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

So it's better to use e.g. EXTBALANCE for the coldness-check, then you don't get it mixed up with the per-precompile unique cost for the precompile itself.

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

Also, we already have tests which touch the low accounts, at least that was my impression?

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

Also, we already have tests which touch the low accounts, at least that was my impression?

Yes, but we didn't have tests for every action. For example, we didn't have tests for calling a precomp while sending it wei (the thing that broke openethereum)

If you send data, 1-7 act hot, 8 acts cold. This is true for CALL, CALLLCODE, DELEGATECALL, and STATICCALL
I don't think this is rirght. I assume that 8 simply rejects your input.

The issue isn't that my call is rejected, I completely ignore the result. The issue is that first call to account 8 costs more gas than the second call.

Account 9 acts cold for calls (all four types, all variants I tested), but acts hot for BALANCE, EXTCODEHASH, and EXTCODESIZE

I think it rejects your input. It's blake2f, it requires exactly 213 bytes of input to not error.

Again, the issue isn't that my input is invalid. It is that a first call and a second call that should cost the same don't.

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

Yes, but we didn't have tests for every action. For example, we didn't have tests for calling a precomp while sending it wei (the thing that broke openethereum)

That wasn't what broke it. The issue was that 0x10 was treated as hot, because it was considered part of the 'precompiles'. This was due to it being defined (but not yet active) in the particular chain config for mainnet. This was not the case of the Berlin-specific test-config.

The issue isn't that my call is rejected, I completely ignore the result. The issue is that first call to account 8 costs more gas than the second call.

I'll look into it. I think there's some mistake here

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

The issue isn't that my call is rejected, I completely ignore the result. The issue is that first call to account 8 costs more gas than the second call.

If you know which indexes are relevant here, it'd be appreciated..

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

Possible source of error, AFAICT, you send all gas: pop(call(gas()). This means that you send along everything. Normally, that's fine, and it sends along only 63/64ths of the available gas. Now, if you do a call to blake, and have erroneous input, it will eat up all that gas, essentially almost all your gas. If you call it a second time, you won't have a much gas left, and this time, you'll only "lose" 63/64ths of what you have now.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

Possible source of error, AFAICT, you send all gas: pop(call(gas()). This means that you send along everything. Normally, that's fine, and it sends along only 63/64ths of the available gas. Now, if you do a call to blake, and have erroneous input, it will eat up all that gas, essentially almost all your gas. If you call it a second time, you won't have a much gas left, and this time, you'll only "lose" 63/64ths of what you have now.

Thank you. I'll redo the test with fixed amounts to check

Edit: Using a fixed amount of gas did fix the issues with 8 and 9. So two issues down, two to go.

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

So two issues down, two to go.

Which are still 'strange" ?

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

So two issues down, two to go.

Which are still 'strange" ?

  1. The first time we send wei to a precompile (with CALL, not CALLCODE) is a different cost than the second time
  2. The first time we EXTCODECOPY from a precompile is a different cost than the second time

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

The first time we send wei to a precompile (with CALL, not CALLCODE) is a different cost than the second time

It's the 25K cost to create an account in the trie. CALLCODE doesn't actually create a new account, so it's just a transfer to self, hence no 25K addon. If you were to "create" the precompiles in the 'state', then you wouldn't see this.

The first time we EXTCODECOPY from a precompile is a different cost than the second time

That sounds a bit odd. I'll look into it (any idea about what index I should look at?)

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

The first time we send wei to a precompile (with CALL, not CALLCODE) is a different cost than the second time

It's the 25K cost to create an account in the trie. CALLCODE doesn't actually create a new account, so it's just a transfer to self, hence no 25K addon. If you were to "create" the precompiles in the 'state', then you wouldn't see this.

Are precompiles "created" on mainnet's state? Or is this behavior consistent across clients so it doesn't matter?

The first time we EXTCODECOPY from a precompile is a different cost than the second time

That sounds a bit odd. I'll look into it (any idea about what index I should look at?)

In the latest version I committed, -d 348

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

Are precompiles "created" on mainnet's state? Or is this behavior consistent across clients so it doesn't matter?

Hah, no, well, yes. They were incrementally created, as people sent money to their addresses. However, when Byzantium rolled in, and we were doing state cleanup by touching empty accounts, the ripemd precompile was touched, and the tx went OOG. It was a shitshow, and caused a still-remaining issue on how to treat empty precompile touches.

If we ever get a new empty precompile, that would be a problem, but no, we never will, so it's theoretical. A lot of them were inited by this account (https://etherscan.io/address/0x8db98f07d8bec0038da3d13ed93adcf09316ef66) at least up to 0x00..0f: https://etherscan.io/address/0x000000000000000000000000000000000000000f. At some point, we need to 'create' https://etherscan.io/address/0x0000000000000000000000000000000000000017 (0x00..17). Damn, looks like we need it before bls!

@holiman
Copy link
Contributor

holiman commented Apr 20, 2021

I trimmed away all but the d=348, and ran it as ./build/bin/evm --json statetest /tmp/precompsEIP2929.json 2> trace.json, to collect the trace.
Then use traceview from https://github.com/holiman/goevmlab .Can be installe via go install github.com/holiman/goevmlab/cmd/traceview, and do traceview trace.json.

Here's the trace right before the first EXTCODECOPY. Note the memory size:
extcodecopy_1

As we execute the EXTCODECOPY, memory expansion is required:
extcodecopy_2

But for the last EXTCODECOPY, no further expansion is needed, so it's cheaper:
extcodecopy_3

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

But for the last EXTCODECOPY, no further expansion is needed, so it's cheaper:

Thank you. Sorry, I thought I dealt with it by touching the memory, but then I made the mistake of reading too much data.

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 20, 2021

Are precompiles "created" on mainnet's state? Or is this behavior consistent across clients so it doesn't matter?

Hah, no, well, yes. They were incrementally created, as people sent money to their addresses. However, when Byzantium rolled in, and we were doing state cleanup by touching empty accounts, the ripemd precompile was touched, and the tx went OOG. It was a shitshow, and caused a still-remaining issue on how to treat empty precompile touches.

If we ever get a new empty precompile, that would be a problem, but no, we never will, so it's theoretical. A lot of them were inited by this account (https://etherscan.io/address/0x8db98f07d8bec0038da3d13ed93adcf09316ef66) at least up to 0x00..0f: https://etherscan.io/address/0x000000000000000000000000000000000000000f. At some point, we need to 'create' https://etherscan.io/address/0x0000000000000000000000000000000000000017 (0x00..17). Damn, looks like we need it before bls!

OK, so there's no chance of this issue being a problem in the real world. I'll keep the tests, but add a note about the reason.

@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label Apr 20, 2021
@holiman
Copy link
Contributor

holiman commented Apr 21, 2021

I'll keep the tests, but add a note about the reason.

SGTM! Glad we cleared all the questions out!

@qbzzt
Copy link
Collaborator Author

qbzzt commented Apr 21, 2021

I'll keep the tests, but add a note about the reason.

SGTM! Glad we cleared all the questions out!

Thank you for answering them.

@winsvega winsvega merged commit 156a4af into develop Apr 21, 2021
@winsvega winsvega deleted the precompEIP2929 branch May 7, 2021 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PR is ready to merge and not WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants