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

Option to not mark transactions as local when received over RPC #8820

Closed
tomusdrw opened this issue Jun 6, 2018 · 10 comments
Closed

Option to not mark transactions as local when received over RPC #8820

tomusdrw opened this issue Jun 6, 2018 · 10 comments
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M6-rpcapi 📣 RPC API. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Milestone

Comments

@tomusdrw
Copy link
Collaborator

tomusdrw commented Jun 6, 2018

Currently the node will mark transactions submitted using eth_sendRawTransaction as local ones.

This means that such transactions will have a higher priority and also can go below configured minimal gas price.
Such situation can cause issues for public-facing nodes that expose eth namespace.

Solution:

  1. Either allow disabling RPC methods name-by-name, so that you can choose to expose eth but not eth_sendRawTransaction
  2. Or add a CLI flag to disable special treating of RAW transactions received over RPC
@tomusdrw tomusdrw added F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M6-rpcapi 📣 RPC API. labels Jun 6, 2018
@XertroV
Copy link
Contributor

XertroV commented Jun 6, 2018

While granular control over RPC methods would be nice, what about a CLI flag that can simply turn off local transactions all together? That might be a lot simpler, and for public facing nodes - they don't really matter.

In the worst case, it's not that difficult to write a proxy that filters particular methods, just doesn't feel like a good solution.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jun 6, 2018

What exactly do you mean by

turn off local transactions all together?

Is it the same as with my option (2)? So that the transactions from eth_sendRawTrasnaction are not handled in a special way?

@XertroV
Copy link
Contributor

XertroV commented Jun 7, 2018

What exactly do you mean by

turn off local transactions all together?

FYI I don't know Rust or the Parity architecture (so I invented config below), but I imagined something like this:

// from /miner/src/pool/scoring.rs

	// Always kick out non-local transactions in favour of local ones.
	if new.priority().is_local() && !old.priority().is_local() && config.prioritise_local() {
		return true;
	}
	// And never kick out local transactions in favour of external ones.
	if !new.priority().is_local() && old.priority.is_local() && config.prioritise_local() {
		return false;
	}

Basically a flag that can totally nullify local checks (i.e. parity becomes indiscriminate between local and non-local functions)

@XertroV
Copy link
Contributor

XertroV commented Jun 7, 2018

Also, @tomusdrw - I think I have a temporary fix.

Although our RPC port on the node was set to 38545 for various reasons (initially had an nginx+https proxy running on 8545) our AWS security group was open to the world. I suspect someone's running a bot that finds the IPs of Ethereum nodes, does a port scan, then if RPC is enabled (even on non-std port) they'll send in eth_sendRawTx methods. I'm disabling public access to 38545 because our nodes are behind a load balancer anyway. Hopefully this will solve the issue in the short term.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Jun 7, 2018

@XertroV ok, I see. So that's similar to what I thought, but a bit more extreme (i.e. doesn't even treat transactions that you have private key for as local).

Good that you found a workaround, we should fix it anyway though.

@XertroV
Copy link
Contributor

XertroV commented Jun 8, 2018

@tomusdrw - yeah, I don't mind how extreme the measure is personally, I just thought that might be quicker/safer to implement.

It's sort of a workaround - technically ppl can still submit raw txs. One thing that would be nice to know is if there's a way to log when this is happening. If it keeps happening I'll implement a small shim that sits between the load balancer and the RPC interface and filters out all eth_sendRawTransaction methods.

@artemii235
Copy link

Hi, I think not only me face this issue because of eth_sendRawTransactions are treated as local. My public Parity node (v1.11.3) requires eth_sendRawTransaction API to be enabled. Someone spams my node with zero gas price transactions filling queue out, seems like there is no way to prevent this by Parity config as these transactions are local.
I only get these messages in logs:
2018-06-12 10:52:16 UTC Local tx 0xea4985d130a2ae7e9797ee4d8b13dba7ac37c3e030251187b10b149765cdb623 below minimal gas price accepted
It's not hard to implement a proxy which will parse raw transactions and decline zero gas price ones, but it would be great if these will get rejected automatically.

@tomusdrw tomusdrw added this to the 1.12 milestone Jun 12, 2018
@tomusdrw tomusdrw added the P5-sometimesoon 🌲 Issue is worth doing soon. label Jun 12, 2018
@tomusdrw
Copy link
Collaborator Author

@5chdn might be worth bumping priority on this one, perhaps a 1.11 release blocker as well.

@XertroV
Copy link
Contributor

XertroV commented Jun 13, 2018

So reading the thread this was forked from gave me an idea - the issue is that the node doesn't have the privkey for the transaction; why not just check the address against local known addresses and only mark local if we find it?

Is this how eth_sendTransaction works now? If so then I imagine it'd be easy..

@XertroV
Copy link
Contributor

XertroV commented Jun 13, 2018

I have an idea:

Add a miner config variable: tx_queue_allow_unknown_local: bool - defaults to true to match current behaviour

When a tx is dispatched the dispatcher checks this flag and calls miner.import_own_transaction or miner.import_external_transaction depending on whether the account is found (if the flag is true)

Working on a PR for this now.

Personally, btw, the default should be false

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
F3-annoyance 💩 The client behaves within expectations, however this “expected behaviour” itself is at issue. M6-rpcapi 📣 RPC API. P2-asap 🌊 No need to stop dead in your tracks, however issue should be addressed as soon as possible.
Projects
None yet
Development

No branches or pull requests

4 participants