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

EIP-2583: Penalty for account trie misses #2583

Merged
merged 7 commits into from
Aug 29, 2020
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Apr 3, 2020

This EIP implements a penalty-scheme for operations which are heavy on the trie. The scheme is designed to be as little disruptive as possible to existing contracts.

- A `penalty` of `3000`would lower the number to ~`2700` lookups, `20%` of the original.
- A `penalty` of `4000`would lower the number to ~`2100` lookups, `15%` of the original.

There exists a roofing function for the `penalty`. Since the `penalty` is deducted from `gas`, that means that a malicious contract can always invoke a malicious relay to perform the trie lookup.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is worth considering the idea which was brought up as part of "Stateless Summit". Have a separate counter, this time for the entire transaction frame, for the penalty counting. This same counter could be used for witness size counting.

Wouldn't it eliminate this attack vector?


1. Storage tries are typically small, and there's a high cost to populate a storage trie with sufficient density for it to be in the same league as the account trie.
2. If an attacker wants to use an existing large storage trie, e.g. some popular token, he would typically have to make a `CALL` to cause a lookup in that token -- something like `token.balanceOf(<inexistant-address>)`.
That adds quite a lot of extra gas-impediments, as each `CALL` is another `700` gas, plus gas for arguments to the `CALL`.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this dependent on the assumption that there are no popular contracts (i.e. ones with an existing large storage trie) with a method that reads a large number of (caller-determined) storage entries? If so, it would be nice to state this assumption explicitly here. (e.g. a theoretical popular token contract with a sumBalanceOf(address[]) method or something like that)

Also if I understood you correctly, I would remove "plus gas for arguments to the CALL", since the CALL opcode doesn't charge gas for input data and presumably the DoS-attacker would generate the cache-miss arguments on-chain rather than passing them as tx data. The point about the minimum of 700 gas per cache-miss is still valid though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I guess it could be several different ways to do this. However, it's much eaiser to get a 'random' number as a single stack-op, (e.g. gas), than if you have to pre-fill a memory segment with 'random' data and pass it to another method. Every argument you send is another 32 byte memory expanded on the caller-side.

One could experiment more in-depth about exactly what the costs would be, but I don't think it would be marginal.

Also if I understood you correctly, I would remove "plus gas for arguments to the CALL", since the CALL opcode doesn't charge gas for input data

You are correct that it doesn't, however, I was referring to the fact that you need to get your input first to stack, then load into memory. If you do it over and over (as opposed to a one-off multi-parameter thing), then you need to flip some bits between calls, to not hit the same thing over and over again.

EIPS/eip-draft-trie-penalty.md Outdated Show resolved Hide resolved
@axic
Copy link
Member

axic commented Apr 21, 2020

Spelling typos:

EIPS/eip-2583.md:15: non-existant  ==> non-existent
EIPS/eip-2583.md:28: non-existant  ==> non-existent
EIPS/eip-2583.md:70: inexistant  ==> inexistent
EIPS/eip-2583.md:71: inexistant  ==> inexistent
EIPS/eip-2583.md:249: non-existant  ==> non-existent

@axic axic changed the title eip x: draft trie penalty EIP-2583: Penalty for account trie misses Apr 21, 2020
@axic
Copy link
Member

axic commented Aug 28, 2020

@holiman hope you don't mind the spelling fix. I think this draft should be merged for the record and to have a good reference for discussion. Even if this version may not be pursued further.


## Simple Summary

This EIP propose to introduce a gas penalty for opcodes which access the account for trie non-existent accounts.
Copy link
Contributor

@MicahZoltu MicahZoltu Aug 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EIPs don't propose, they assert. One may propose an EIP be adopted (in some other venue than the EIP document), but the EIP itself is an assertion of some change.

Comment on lines +19 to +20
This EIP proposes to add a gas penalty for accesses to the account trie, where the address being looked up does not exist. Non-existing accounts can be used in
DoS attacks, since they bypass cache mechanisms, thus creating a large discrepancy between 'normal' mode of execution and 'worst-case' execution of an opcode.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this feels like motivation, not abstract. Abstract should be a terse human-friendly version of the specification section.


## Specification

We define the constant `penalty` as `TBD` (suggested `2000` gas).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
We define the constant `penalty` as `TBD` (suggested `2000` gas).
We define the constant `penalty` as `2000`.

Recommended during Draft (not required until Last Call): EIPs are assertions, put the number you want in and you can always change it later (while it is in draft). Alternatively, if you don't want a sticky default just put in TBD and discuss 2000 in the discussion-to link.

Comment on lines +75 to +76


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: should be 1 newline or 0 (depending on personal style, have seen both used in this repository).

Comment on lines +112 to +123
A transaction with `10M` gas can today cause ~`14K` trie lookups.

- A `penalty` of `1000`would lower the number to ~`5800` lookups, `41%` of the original.
- A `penalty` of `2000`would lower the number to ~`3700` lookups, `26%` of the original.
- A `penalty` of `3000`would lower the number to ~`2700` lookups, `20%` of the original.
- A `penalty` of `4000`would lower the number to ~`2100` lookups, `15%` of the original.

There exists a roofing function for the `penalty`. Since the `penalty` is deducted from `gas`, that means that a malicious contract can always invoke a malicious relay to perform the trie lookup. Let's refer to this as the 'shielded relay' attack.

In such a scenario, the `malicious` would spend `~750` gas each call to `relay`, and would need to provide the `relay` with at least `700` gas to do a trie access.

Thus, the effective `cost` would be on the order of `1500`. It can thus be argued that `penalty` above `~800` would not achieve better protection against state-inexistence attacks.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in the Rationale section. The rationale is where you discuss why you chose the constants you chose. The specification section should merely assert that the constant is X and it is applied here, here, and here.


| Opcode | Affected | Comment |
| ----- | ---------| ----------|
| BALANCE| Yes | `balance(inexistent_addr)` would incur `penalty`|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is inexistent a word? I thought only nonexistent was? Spell checker says no, Bing thinks I want to translate from French when I type it in, but there are a couple online dictionaries that have an entry for inexistent.

Recommend switching to nonexistent unless there is a strong belief that this is in fact a word and means something different than nonexistent.

Comment on lines +259 to +291
## Alternative variants

### Alt 1: Insta-refunds

Bump all trie accesses with `penalty`. `EXTCODEHASH` becomes `2700` instead of `700`.
- If a trie access hit an existing item, immediately refund penalty (`2K` )

Upside:

- This eliminates the 'shielded relay' attack

Downside:

- This increases the up-front cost of many ops (CALL/EXTCODEHASH/EXTCODESIZE/STATICCALL/EXTCODESIZE etc)
- Which may break many contracts.

### Alt 2: Parent bail

Use `penalty` as described, but if a child context goes OOG on the `penalty`, then the remainder is subtracted from the
parent context (recursively).

Upside:

- This eliminates the 'shielded relay' attack

Downside:

- This breaks the current invariant that a child context is limited by whatever `gas` was allocated for it.
- However, the invariant is not _totally_ thrown out, the new invariant becomes that it is limited to `gas + penalty`.
- This can be seen as 'messy' -- since only _some_ types of OOG (penalties) becomes passed up the call chain, but not others, e.g. OOG due to trying
to allocate too much memory. There is a distinction, however:
- Gas-costs which arise due to not-yet-consumed resources do not get passed to parent. For example: a huge allocation is not actually performed if there is insufficient gas.
- Whereas gas-costs which arise due to already-consumed resources _do_ get passed to parent; in this case the penalty is paid post-facto for a trie iteration.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this should either move to discussion-to link or be restructured to fit into the Rationale section (the section for "why did we do X instead of Y"). EIPs are technical documents that define a specification, they aren't meant to be discussion platforms or a log of discussions on a topic.

@MicahZoltu
Copy link
Contributor

I think this draft should be merged for the record and to have a good reference for discussion. Even if this version may not be pursued further.

I'm generally not a fan of merging EIPs that the author is going to immediately abandon, however I don't think we have a policy against that at the moment so I'll move forward with merging this and just say that I would prefer we don't do that in the future.

@MicahZoltu MicahZoltu merged commit 93072ce into ethereum:master Aug 29, 2020
@axic
Copy link
Member

axic commented Aug 29, 2020

This was discussed on ACD a few times and I think an upcoming EIP may or may not deprecate it. For properly referencing it makes sense, but I'm not 100% sure what is the best option.

@lightclient
Copy link
Member

lightclient commented Aug 29, 2020

It would be interesting if we had a "draft track" akin to the RFC "Internet draft". In that case we could be much more lenient on the draft track and then be more strict when it comes time for an EIP. That might also help people avoid grabbing EIP numbers to grandstand an issue.

@holiman
Copy link
Contributor Author

holiman commented Aug 29, 2020 via email

tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
* eip x: draft trie penalty

* Update EIPS/eip-draft-trie-penalty.md

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

* EIP-2583: rename file

* 2583: discussion-to + two alternative variants

* 2583: formatting

* Fix wording: *existant -> *existent

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
* eip x: draft trie penalty

* Update EIPS/eip-draft-trie-penalty.md

Co-Authored-By: Alex Beregszaszi <alex@rtfs.hu>

* EIP-2583: rename file

* 2583: discussion-to + two alternative variants

* 2583: formatting

* Fix wording: *existant -> *existent

Co-authored-by: Alex Beregszaszi <alex@rtfs.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants