Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Penalize transactions with gas above gas limit #2271

Merged
merged 11 commits into from
Sep 25, 2016
Merged

Penalize transactions with gas above gas limit #2271

merged 11 commits into from
Sep 25, 2016

Conversation

tomusdrw
Copy link
Collaborator

No description provided.

@tomusdrw tomusdrw added A0-pleasereview 🤓 Pull request needs code review. B0-patch M4-core ⛓ Core client code / Rust. labels Sep 23, 2016
@coveralls
Copy link

coveralls commented Sep 23, 2016

Coverage Status

Changes Unknown when pulling 52169b2 on tx-penalize into * on beta*.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 23, 2016
@gavofyork
Copy link
Contributor

looks alright to me.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 745b8b6 on tx-penalize into * on beta*.

Copy link
Contributor

@rphmeier rphmeier left a comment

Choose a reason for hiding this comment

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

just a couple tiny code quality issues, otherwise LGTM

self.penalties = match self.penalties.overflowing_add(1) {
(_, true) => current,
(val, false) => val,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

saturating_add?

Copy link
Contributor

@rphmeier rphmeier Sep 23, 2016

Choose a reason for hiding this comment

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

self.penalties = self.penalties.saturating_add(1);

return;
}

let transaction = transaction.expect("Early-exit for None is above.");
Copy link
Contributor

Choose a reason for hiding this comment

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

more idiomatic to match instead of .is_none() and expect combo.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6fffede on tx-penalize into * on beta*.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d4312d1 on tx-penalize into * on beta*.

@rphmeier
Copy link
Contributor

Not sure about the gas before gas price change tbh -- what's the incentive under normal operating conditions?

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling d4312d1 on tx-penalize into * on beta*.

@gavofyork
Copy link
Contributor

gavofyork commented Sep 24, 2016

to favour a regular throughput of the queue: if there is an attack underway (which there pretty much will be until we get a hard fork) then favouring small transactions:

  • gives a much greater chance of mining beneficial transactions as they get mixed in with the bad transactions under a spam attack;
  • favours a faster rotation of queue contents, mitigating any exploitable queue ordering logic;
  • dilutes any attack gas - for a 30kgas attack transaction, only ~9kgas can actively be used in the attack (21k is price of transaction), giving an attack efficiency of only 30%; for current 1Mgas transactions, attack efficiency is 98%.

@gavofyork gavofyork added A0-pleasereview 🤓 Pull request needs code review. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Sep 24, 2016
@gavofyork
Copy link
Contributor

merging due to general positive voices and on the back of @rphmeier 's prior review

@gavofyork gavofyork merged commit c374164 into beta Sep 25, 2016
@gavofyork gavofyork deleted the tx-penalize branch September 25, 2016 10:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants