EIP-168, 169: Dust protection #4757

arkpar · 2017-03-04T00:14:08Z

This implements
ethereum/EIPs#168 Variant C
ethereum/EIPs#169 Variant C

gavofyork · 2017-03-05T09:42:34Z

added a variant C to 168, which might be worth considering

gavofyork · 2017-03-05T09:57:00Z

169.c.c.2 is definitely not redundant.

gavofyork · 2017-03-08T13:54:04Z

will need resolving

arkpar · 2017-03-08T14:17:46Z

resolved

gavofyork · 2017-03-08T18:03:19Z

ethcore/src/state/account.rs

@@ -315,6 +315,11 @@ impl Account {
 		self.code_hash == SHA3_EMPTY
 	}

+	/// Check if account is basic (Has no code).
+	pub fn is_basic(&self) -> bool {
+		self.code_hash == SHA3_EMPTY


don't think this will work for newly created accounts

If it's newly created account that does not have code it is a basic account, right? It stops being basic only after it gets the code.

i guess it's fine - this particular function should work at least (though at one point new_contract actually gave a non-basic account which just happened to have no code yet, but looks like it's now just an uncached basic account).

gavofyork · 2017-03-08T18:03:32Z

ethcore/src/state/mod.rs

@@ -95,6 +97,7 @@ enum AccountState {
 /// account (`None`)
 struct AccountEntry {
 	account: Option<Account>,
+	clean_balance: Option<U256>,


probably worth documenting these members now - clean_balance isn't immediately obvious in meaning. would probably rename to old_balance or something similar to emphasise its underlying semantics.

gavofyork · 2017-03-08T18:03:42Z

ethcore/src/state/mod.rs

@@ -121,12 +128,14 @@ impl AccountEntry {
 		AccountEntry {
 			account: self.account.as_ref().map(Account::clone_dirty),
 			state: self.state,
+			clean_balance: self.clean_balance,


appears after other fields (others constructors appear prior)

This is in line with the declaration order. Other constructors are exceptions because they use account before it is moved into self.account. Will fix here for consistency.

gavofyork · 2017-03-08T18:03:54Z

ethcore/src/state/mod.rs

-				_ => {}
-			}
+		} else if cleanup_mode == CleanupMode::KillEmpty && self.exists(a)? {
+			self.require(a, false)?;


consider introducing a function fn touch(&self, a: ...) -> ... { self.require(a, false) } and using that in order to make clear the intent here.
this doesn't make use of is_value_transfer; i am skeptical that precise semantics have been preserved.

This branch only executes when is_value_transfer == false

gavofyork · 2017-03-08T18:04:16Z

ethcore/src/state/mod.rs

+	pub fn kill_garbage(&mut self, remove_empty_touched: bool, min_balance: &Option<U256>, kill_contracts: bool) -> trie::Result<()> {
+		let to_kill: HashSet<_> = {
+			self.cache.borrow().iter().filter_map(|(address, ref entry)|
+			if (entry.is_dirty() || address == &RIPEMD_BUILTIN) &&


RIPEMD is very ugly - can we avoid having it here?

also, not entirely sure what these conditions are supposed to achieve.

I've added this because this new implementation revealed a subtle consensus flaw in the previous one. If a built-in runs out of gas it's account still gets deleted. This is now in sync with the go-ethereum and other clients which also have a special case for this particular built-in account.

that said, this special-casing is only for the mainnet; our code should reflect that.

gavofyork · 2017-03-08T18:04:26Z

ethcore/src/state/mod.rs

+			self.cache.borrow().iter().filter_map(|(address, ref entry)|
+			if (entry.is_dirty() || address == &RIPEMD_BUILTIN) &&
+			((remove_empty_touched && entry.is_null())
+			|| min_balance.map_or(false, |ref balance| entry.account.as_ref().map_or(false, |account|


indentation off by one (still have an open paren here from previous line)

gavofyork · 2017-03-08T18:04:34Z

ethcore/src/state/mod.rs

+			((remove_empty_touched && entry.is_null())
+			|| min_balance.map_or(false, |ref balance| entry.account.as_ref().map_or(false, |account|
+				(account.is_basic() || kill_contracts)
+				&& account.balance() < balance && entry.clean_balance.as_ref().map_or(false, |b| account.balance() < b)))) {


could do with some commentary.

gavofyork · 2017-03-08T18:04:46Z

ethcore/src/state/substate.rs

@@ -28,9 +28,6 @@ pub struct Substate {
 	/// Any accounts that have suicided.
 	pub suicides: HashSet<Address>,

-	/// Any accounts that are tagged for garbage collection.
-	pub garbage: HashSet<Address>,


i'm a bit concerned that the alteration of semantics (from explicit collection of gardbage-collected accounts to the implicit reliance on touching the cache) here will lead to subtle consensus flaws. i don't remember exactly, but when i coded this originally i chose this means of doing it for a reason.

gavofyork · 2017-03-08T18:04:58Z

ethcore/src/verification/verification.rs

@@ -392,6 +400,12 @@ mod tests {
 		verify_block_family(&header, bytes, engine, bc)
 	}

+	fn unordered_test(bytes: &[u8], engine: &Engine) -> Result<(), Error> {


mark with #[test] if only used in the test?

It is part of the tests mod already, but not a test itself.

gavofyork · 2017-03-08T18:06:08Z

not at all keen on the removal of garbage from substate - it departs significantly from the structure of the YP spec and relies on largely undocumented and unspecified behaviour of the account cache.

arkpar · 2017-03-08T18:30:39Z

Current implementation does not handle state reverts properly. This is what caused the original RIPEMD built-in bug, for which there's going to be an exception added to the YP. State already tracks touched/dirty states for entries, why repeat that logic elsewhere? Also, how's State is less documented and specified than Substate/Executive? As far as I can see it has even better documentation.

gavofyork · 2017-03-08T19:28:03Z

Well I guess the point is that while the notion of "in the cache" may be similar to the definition of "touched" within the strict context of EIP/YP, because it was not explicitly implemented or specified to be exactly that concept, we run the risk that what might reasonably be considered an internal change or optimisation to our state-caching code will accidentally and subtly break consensus.

gavofyork · 2017-03-30T18:11:48Z

i now remember the reason we don't want to do it this way: reversion of an EVM call-path should also revert the touchedness of an account, in much the same way it reverts other aspects of substate.

arkpar · 2017-04-11T07:27:41Z

This has been updated to track touched accounts in the substate

rphmeier · 2017-05-02T09:28:38Z

Could use a merge

gavofyork · 2017-05-03T13:33:21Z

Variant C of 168 purposefully doesn't mention "touched" accounts:

At the end of the transaction, any account created or whose balance is decreased by the execution of that transaction which is now dust SHALL instead become non-existent (i.e. deleted).

Yet this PR seems to operate solely on them.

arkpar · 2017-05-03T15:39:37Z

@rphmeier I'll merge it once @gavofyork is happy with the implementation.

arkpar · 2017-05-03T15:52:00Z

@gavofyork Quoting EIP-161:

An account is considered to be touched when it is involved in any potentially state-changing operation

Clearly this includes all EIP-168C accounts as well. A check for "Created or whose balance is decreased by the execution of that transaction which is now dust" is performed "at the end of the transaction" in kill_garbage as specified.

Could you point out what's exactly wrong with that?

I don't think there's any sense in maintaining yet another additional list of accounts that were "Created or whose balance is decreased by any non-zero value" as this is, again, just wasting memory.

replied in comments

gavofyork · 2017-05-04T10:20:23Z

ethcore/src/state/mod.rs

 		trace!(target: "state", "sub_balance({}, {}): {}", a, decr, self.balance(a)?);
-		if !decr.is_zero() || !self.exists(a)? {


reason for removal?

This function is never called for non-existing accounts. However it is indeed incorrect to assume it here. I'll restore it.

gavofyork · 2017-05-04T18:06:32Z

Could you point out what's exactly wrong with that?

the concern was with the prefilter of "touched" being applied to to EIP168's "any account created or whose balance is decreased". just need to prove that EIP168's condition strictly implies the property of being "touched". i suspect it does, but it does need a reasonably formal proof before we can be sure this properly implements the EIP.

arkpar · 2017-06-23T13:38:50Z

ethcore/src/engines/mod.rs

+	/// The nonce with which accounts begin at given block.
+	fn account_start_nonce(&self, block: u64) -> U256 {
+		if block >= self.params().dust_protection_transition {
+			U256::from(self.params().nonce_cap_increment) * U256::from(block)


Could just multiply in u64

arkpar · 2017-06-23T14:09:18Z

ethcore/src/state/mod.rs

@@ -106,6 +110,10 @@ impl AccountEntry {
 		self.state == AccountState::Dirty
 	}

+	fn is_null(&self) -> bool {


better name: exists_and_is_null

arkpar force-pushed the dust branch from 91e9d51 to 60b12c7 Compare March 8, 2017 14:17

gavofyork previously requested changes Mar 8, 2017

View reviewed changes

gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 8, 2017

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Mar 8, 2017

This was referenced Mar 10, 2017

EIP-86 #4697

Merged

EIP-116 (214), #4833 #4851

Merged

gavofyork changed the title ~~Dust protection~~ EIP-168, 168: Dust protection Mar 10, 2017

gavofyork changed the title ~~EIP-168, 168: Dust protection~~ EIP-168, 169: Dust protection Mar 13, 2017

arkpar force-pushed the dust branch from d5cd1e4 to 0c96f99 Compare March 15, 2017 16:21

gavofyork added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Mar 30, 2017

arkpar force-pushed the dust branch 2 times, most recently from 6249981 to 52c4732 Compare April 18, 2017 10:33

gavofyork added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 3, 2017

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels May 3, 2017

gavofyork reviewed May 4, 2017

View reviewed changes

arkpar added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 4, 2017

arkpar commented Jun 23, 2017

View reviewed changes

gavofyork added A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 23, 2017

arkpar added 3 commits June 23, 2017 17:15

Dust protection

5e7955f

Track touched accounts in the substate

0399550

Minor alterations

d80bc2f

arkpar force-pushed the dust branch from 52c4732 to d80bc2f Compare June 23, 2017 15:16

arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Jun 23, 2017

arkpar merged commit 57626b6 into master Jun 28, 2017

arkpar deleted the dust branch June 28, 2017 07:11

vyorkin mentioned this pull request Jun 1, 2018

Fix refund on selfdestruct mana-ethereum/mana#126

Closed

meowsbits mentioned this pull request May 28, 2019

Final Call: ECIP-1054 Atlantis Upgrade ethereumclassic/ECIPs#78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP-168, 169: Dust protection #4757

EIP-168, 169: Dust protection #4757

arkpar commented Mar 4, 2017 •

edited

Loading

gavofyork commented Mar 5, 2017

gavofyork commented Mar 5, 2017 •

edited

Loading

gavofyork commented Mar 8, 2017

arkpar commented Mar 8, 2017

gavofyork Mar 8, 2017

arkpar Mar 8, 2017

gavofyork May 3, 2017

gavofyork Mar 8, 2017

gavofyork Mar 8, 2017

arkpar Mar 8, 2017

gavofyork Mar 8, 2017

arkpar Mar 8, 2017

gavofyork Mar 8, 2017

arkpar Mar 8, 2017

rphmeier Mar 8, 2017

gavofyork Mar 8, 2017

gavofyork Mar 8, 2017

gavofyork Mar 8, 2017

gavofyork Mar 8, 2017

arkpar Mar 8, 2017

gavofyork commented Mar 8, 2017

arkpar commented Mar 8, 2017

gavofyork commented Mar 8, 2017 •

edited

Loading

gavofyork commented Mar 30, 2017

arkpar commented Apr 11, 2017

rphmeier commented May 2, 2017

gavofyork commented May 3, 2017

arkpar commented May 3, 2017

arkpar commented May 3, 2017

gavofyork May 4, 2017

arkpar May 4, 2017

gavofyork commented May 4, 2017

arkpar Jun 23, 2017

arkpar Jun 23, 2017

		trace!(target: "state", "sub_balance({}, {}): {}", a, decr, self.balance(a)?);
		if !decr.is_zero() \|\| !self.exists(a)? {

EIP-168, 169: Dust protection #4757

EIP-168, 169: Dust protection #4757

Conversation

arkpar commented Mar 4, 2017 • edited Loading

gavofyork commented Mar 5, 2017

gavofyork commented Mar 5, 2017 • edited Loading

gavofyork commented Mar 8, 2017

arkpar commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Mar 8, 2017

arkpar commented Mar 8, 2017

gavofyork commented Mar 8, 2017 • edited Loading

gavofyork commented Mar 30, 2017

arkpar commented Apr 11, 2017

rphmeier commented May 2, 2017

gavofyork commented May 3, 2017

arkpar commented May 3, 2017

arkpar commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arkpar commented Mar 4, 2017 •

edited

Loading

gavofyork commented Mar 5, 2017 •

edited

Loading

gavofyork commented Mar 8, 2017 •

edited

Loading