Transaction Queue banning #2524

tomusdrw · 2016-10-07T15:17:17Z

No description provided.

taoeffect · 2016-10-07T22:32:30Z

What is this?

There is no reason for a blacklisting mechanism to be included in a public blockchain node. That is what the gas/fee mechanism is for. Introducing a generic blacklisting mechanism is:

Unnecessary, you can always ban an IP using ufw / iptables / your-favorite-firewall.
Guaranteed to increase censorship in the network.
Incorrect, fees are how you solve spam issues.

rphmeier · 2016-10-07T23:02:41Z

@taoeffect Unfortunately, the gas mechanism is fundamentally mis-priced. In the period before the instruction costs are adjusted, I hope you can forgive some interim measures intended to smooth over the corner cases of the current pricing scheme.

It's not truly a blacklist but a (local) timeout for senders who appear to be producing transactions exploiting those weaknesses.

taoeffect · 2016-10-07T23:06:43Z

Thanks @rphmeier, I just reached a similar conclusion after finishing reviewing the code. It's (thankfully) not user-specified but automatic based on txn processing time.

I still get nervous anytime I see blacklisting mechanisms, even if they seem fairly benign like this, simply because once you introduce a blacklisting mechanism it becomes trivial to expand it. Every now and then you'll get someone else adding another "minor" change to it and after a while you're left with the full censorship-experience shebang.

Therefore, if there is really no option but to do this, then I hope this code can be removed after a proper fix is introduced, e.g.: ethereum/EIPs#150

taoeffect · 2016-10-07T23:12:04Z

In the period before the instruction costs are adjusted, I hope you can forgive some interim measures intended to smooth over the corner cases of the current pricing scheme.

Once the pricing scheme is fixed, this should no longer be necessary, correct? Therefore, can we get a commitment that this code will be removed in a future release?

And if no such commitment can be given, can it please be explained why that is, and also why the extreme divergence from the Yellow Paper?

rphmeier · 2016-10-07T23:17:29Z

@taoeffect I don't see any problem in removing it once EIP 150 is in. If the costs are adjusted correctly we shouldn't see any transactions hitting the time limit here, making the mechanism effectively dead code.

taoeffect · 2016-10-07T23:42:30Z

@taoeffect I don't see any problem in removing it once EIP 150 is in. If the costs are adjusted correctly we shouldn't see any transactions hitting the time limit here, making the mechanism effectively dead code.

@rphmeier Awesome, thank you. 🙇

taoeffect · 2016-10-08T00:32:41Z

Every now and then you'll get someone else adding another "minor" change to it and after a while you're left with the full censorship-experience shebang.

Err, what am I saying, that's not how it usually works.

Usually it works more like we're seeing here right now.

Attacker wants to achieve something (for Ethereum it's usually censorship, brought about by various means).
So the attacker creates a problem, in this case it's a spam attack.
Then, while everybody's freaking about whatever problem, the attacker proposes, or gets somebody else to suggest, a "solution", that happens to be an actual solution to the problem.
Undoubtedly this solution will always have a caveat, sometimes it will be obvious, other times it might appear innocuous because it's part of a more subtle long-term plan. In this case, if such an attack were taking place, it would be the beginnings of a generic censorship mechanism.

So, this stuff is real. This is how sophisticated sabotage attempts actually work, and we have plenty of case studies of existing attempts on Bitcoin.

That is why it is very important to take this seriously and to really remove the code ASAP, or better yet, don't merge it at all.

gavofyork · 2016-10-19T16:40:38Z

@tomusdrw i'm generally happy to merge this, but i think it'd be better we HEAVY_TRANSACTION_THRESHOLD_MS and generally activating it---to be configurable. should probably start either with a fairly conservative threshold (e.g. 1s) or disabled.

tomusdrw · 2016-10-21T08:04:52Z

Sure, will have a look at this.

Conflicts: ethcore/Cargo.toml ethcore/src/miner/miner.rs

rphmeier · 2016-10-25T16:34:43Z

ethcore/Cargo.toml

 byteorder = "0.5"
 transient-hashmap = "0.1"
+lru-cache = { git = "https://github.com/contain-rs/lru-cache" }


why move to the direct version? i've found that they'll usually publish a new version if you ask :)

Either I broke something during merge or it was already like this on master:
https://github.com/ethcore/parity/blame/master/ethcore/Cargo.toml

[ci:skip]

gavofyork · 2016-10-26T14:55:38Z

would prefer to see the "180 seconds" parameterised, too. looks good otherwise.

tomusdrw · 2016-10-27T11:59:31Z

Updated.

rphmeier · 2016-10-27T13:09:56Z

parity/cli/mod.rs

 			flag_remove_solved: false,
 			flag_notify_work: Some("http://localhost:3001".into()),

 			// -- Footprint Options
 			flag_tracing: "auto".into(),
 			flag_pruning: "auto".into(),
-			flag_pruning_history: 64u64,
+			flag_pruning_history: 128u64,


why change pruning history default?

Sorry, will move it to separate PR - I haven't changed the default, but rather fixed a bug where config value would not be used (usage.txt had [default: instead of (default:)
This change was only to verify that it's not working.

Tomasz Drwięga added 2 commits October 7, 2016 17:00

Blacklisting transaction queue

3eb8ef8

Using blacklisting queue in miner

eb9d58b

tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Oct 7, 2016

Restoring todo [ci:skip]

1810e44

openethereum locked and limited conversation to collaborators Oct 19, 2016

Tomasz Drwięga added 4 commits October 25, 2016 10:20

Merge branch 'master' into queue-blacklist

133c899

Conflicts: ethcore/Cargo.toml ethcore/src/miner/miner.rs

Blacklisting recipients and code

ad06276

Renaming blacklisting->banning

48a9e71

CLI option for banning.

77e4ac5

tomusdrw changed the title ~~Transaction Queue blacklists~~ Transaction Queue banning Oct 25, 2016

rphmeier reviewed Oct 25, 2016

View reviewed changes

Tomasz Drwięga added 3 commits October 25, 2016 18:40

Fixing submodule commit [ci:skip]

e4fec05

Fixing RPC tests

58b4a62

Additional logging when dropping transactions

eb6c863

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 26, 2016

whitespace

9dbe3c0

[ci:skip]

gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 26, 2016

Configurable ban duration

068b759

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Oct 27, 2016

rphmeier reviewed Oct 27, 2016

View reviewed changes

tomusdrw added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 27, 2016

Reverting fix for pruning history from config file

9c96e55

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 27, 2016

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 27, 2016

gavofyork merged commit 152a551 into master Oct 27, 2016

gavofyork deleted the queue-blacklist branch October 27, 2016 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction Queue banning #2524

Transaction Queue banning #2524

tomusdrw commented Oct 7, 2016

taoeffect commented Oct 7, 2016

rphmeier commented Oct 7, 2016

taoeffect commented Oct 7, 2016 •

edited

Loading

taoeffect commented Oct 7, 2016

rphmeier commented Oct 7, 2016

taoeffect commented Oct 7, 2016

taoeffect commented Oct 8, 2016 •

edited

Loading

gavofyork commented Oct 19, 2016

tomusdrw commented Oct 21, 2016

rphmeier Oct 25, 2016

tomusdrw Oct 25, 2016

gavofyork commented Oct 26, 2016

tomusdrw commented Oct 27, 2016

rphmeier Oct 27, 2016

tomusdrw Oct 27, 2016

Transaction Queue banning #2524

Transaction Queue banning #2524

Conversation

tomusdrw commented Oct 7, 2016

taoeffect commented Oct 7, 2016

rphmeier commented Oct 7, 2016

taoeffect commented Oct 7, 2016 • edited Loading

taoeffect commented Oct 7, 2016

rphmeier commented Oct 7, 2016

taoeffect commented Oct 7, 2016

taoeffect commented Oct 8, 2016 • edited Loading

gavofyork commented Oct 19, 2016

tomusdrw commented Oct 21, 2016

rphmeier Oct 25, 2016

Choose a reason for hiding this comment

tomusdrw Oct 25, 2016

Choose a reason for hiding this comment

gavofyork commented Oct 26, 2016

tomusdrw commented Oct 27, 2016

rphmeier Oct 27, 2016

Choose a reason for hiding this comment

tomusdrw Oct 27, 2016

Choose a reason for hiding this comment

taoeffect commented Oct 7, 2016 •

edited

Loading

taoeffect commented Oct 8, 2016 •

edited

Loading