fix(light/response) : handle bad responses #9756

niklasad1 · 2018-10-15T15:53:27Z

This PR adds the following:

Closes Light client does not respond to eth_getTransactionReceipt request #9133
Registers empty responses as a bad
Register bad responses or at least responses that are validated as bad and only takes a decision when the majority of the peers have come to the same conclusion.

This change the assumptions slightly i.e, before we assumed that all requests are valid and only dropped on a timeout or too many request attempts to each peer.

Might be overlapping a bit with the timeouts and I plan the clean it up that code a bit if/after this gets merged! Should be sufficient with this and timeouts only

Issues:

How to handle when peers disconnect, i.e should the response be removed from the bad response set? A malicious node could potentially connect and disconnect repeatedly to get a new node_id and get us to eventually drop the
Is the formula bad_responses > peers/2 sufficient?

/cc @Tbaut @amaurymartiny
Would be good if you could test it, seems to work well when I tested it with
https://gist.github.com/niklasad1/43690f39a07e59f92fd4a8ea9709385b

niklasad1 · 2018-10-15T15:55:04Z

ethcore/light/src/on_demand/mod.rs

+					.cycle()
+					.skip(offset)
+					.take(num_peers)
+				{


I think cycle is clearer in this context and formatted the code to be easier to read

niklasad1 · 2018-10-15T15:55:48Z

ethcore/light/src/on_demand/mod.rs

+						}
+					}
+				}
+


Verbosity, I think it helps to know which errors can occur

niklasad1 · 2018-10-15T15:57:37Z

ethcore/light/src/on_demand/mod.rs

+					match err {
+						// This can be a malformed request or bad response but we can't determine which at the moment!
+						// Thus, register the response as bad and wait for more responses in order to take a decision
+						ValidityError::BadProof | ValidityError::Empty => {


We can get a BadProof for an execution on an account with an insufficient balance so better to be sure when we punish peers!

Not sure I understand this. Why would it happen now that we have #9591 (unless someone is running node, older than 2.0.7-stable/2.1.2-beta)? And how can we distinguish this from invalid proof?

niklasad1 · 2018-10-15T15:58:23Z

ethcore/light/src/on_demand/tests.rs

@@ -18,6 +18,7 @@

 use cache::Cache;
 use ethcore::header::Header;
+use ethcore::encoded::Header as EncodedHeader;


not used remove!!

niklasad1 · 2018-10-15T15:58:35Z

ethcore/light/src/on_demand/tests.rs

@@ -28,7 +29,7 @@ use ::request::{self as basic_request, Response};

 use std::sync::Arc;

-use super::{request, OnDemand, Peer, HeaderRef};
+use super::{request, OnDemand, Peer, HeaderRef, ResponseError, ValidityError};


not used remove!

5chdn · 2018-10-15T18:17:20Z

Please label your PR :)

ordian

I have mixed feelings about this one:

I thought Light client does not respond to eth_getTransactionReceipt request #9133 is about not replying to a valid request? For invalid ones we now may return "Timeout for On-demand query". Or is this about proper error message?
Not sure about this formula |bad_responses| > peers / 2, since number of peers is dynamic and if we disconnect from peers this will change whether a request is considered bad or not. I don't have a better suggestion though.
This policy could potentially be implemented via failsafe (Refactor on-demand queries to use failsafe-rs #9536).

ordian · 2018-10-16T10:21:38Z

ethcore/light/src/on_demand/mod.rs

-	{
+		-> Result<(), basic_request::ResponseError<self::request::Error>> {
+
+		println!("{:?}", response);


Use trace! or debug! here.

ethcore/light/src/on_demand/mod.rs

ordian · 2018-10-16T10:34:21Z

ethcore/light/src/on_demand/tests.rs

+	let mut harness = Harness::create();
+
+	// peer[0]      - requester ("light")
+	// peer[1..9]   - responders ("providers"


nit: unmatched (

ordian · 2018-10-16T10:52:00Z

ethcore/light/src/on_demand/mod.rs

 	fn time_out(self) {
 		trace!(target: "on_demand", "Dropping a pending query (no new peer time out) at query #{}", self.query_id_history.len());
 		let err = self::error::ErrorKind::TimeoutOnNewPeers(self.requests.num_answered(), self.query_id_history.len());
 		if self.sender.send(Err(err.into())).is_err() {
 			debug!(target: "on_demand", "Dropped oneshot channel receiver on time out");
 		}
 	}
+
+	// The given request is determined as faulty and drop it accordingly
+	fn set_as_faulty_request(self, total_peers: usize, req_id: ReqId) {


Consider renaming this to faulty_request or bad_request to follow the same naming convention as time_out and no_response (or rename those).

ordian · 2018-10-16T11:18:38Z

ethcore/light/src/on_demand/mod.rs

+	// The given request is determined as faulty and drop it accordingly
+	fn set_as_faulty_request(self, total_peers: usize, req_id: ReqId) {
+		let bad_peers = self.bad_responses.len();
+		warn!(target: "on_demand", "The request: {} was determined as faulty, {}/{} peer(s) gave bad response", req_id, bad_peers, total_peers);


Do we need a warn! here? A user can see a similar message in the response.

This PR adds the following: * Registers empty responses as a `bad` * Register bad responses and only takes a decision when the majority of the peers have come to `agreement`.

niklasad1 · 2018-10-16T12:43:07Z

@ordian

Great, comments

Basically, timeout/retries are based on user configuration assumptions. This should improve throughput as long the peers and don't spam the network. We can get more accurate error message also but those are unforgently not propagated currently, I will change that but it requires to actually store the reason and select the majority of the bad responses...
...
Yes, that is awesome but I think it's even more beneficial for timeouts because of the exponential backoff i.e, to limit the number of attempts on a request by exponential decaying. Currently, we just wake up every 5 seconds and retry the request (I haven't double checked the actual interval but something like that)

niklasad1 · 2018-10-19T09:27:20Z

Closing for now, I will migrate to failsafe first then revisit this

niklasad1 commented Oct 15, 2018

View reviewed changes

ethcore/light/src/on_demand/mod.rs

}

}

}

Copy link

Collaborator Author

niklasad1 Oct 15, 2018

Choose a reason for hiding this comment

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

Verbosity, I think it helps to know which errors can occur

niklasad1 commented Oct 15, 2018

View reviewed changes

niklasad1 requested review from ordian and rphmeier October 15, 2018 16:03

niklasad1 mentioned this pull request Oct 15, 2018

eth_estimateGas returns "The execution failed due to an exception." #9669

Closed

5chdn added this to the 2.2 milestone Oct 15, 2018

5chdn added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 15, 2018

niklasad1 force-pushed the light/response-handling branch from 61b9c76 to a3ddcb0 Compare October 15, 2018 18:28

niklasad1 added B1-patch-beta 🕷🕷 B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. labels Oct 15, 2018

ordian reviewed Oct 16, 2018

View reviewed changes

niklasad1 added 3 commits October 16, 2018 14:13

fix(light::response) : handle bad responses

096dca9

This PR adds the following: * Registers empty responses as a `bad` * Register bad responses and only takes a decision when the majority of the peers have come to `agreement`.

fix(test) - make sure request don't dropped

3e50f39

fix(nits)

56779ff

niklasad1 force-pushed the light/response-handling branch from a3ddcb0 to 56779ff Compare October 16, 2018 12:14

niklasad1 added the A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. label Oct 17, 2018

niklasad1 closed this Oct 19, 2018

niklasad1 deleted the light/response-handling branch January 17, 2019 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(light/response) : handle bad responses #9756

fix(light/response) : handle bad responses #9756

niklasad1 commented Oct 15, 2018 •

edited

Loading

niklasad1 Oct 15, 2018 •

edited

Loading

niklasad1 Oct 15, 2018

niklasad1 Oct 15, 2018

ordian Oct 16, 2018

niklasad1 Oct 15, 2018

niklasad1 Oct 15, 2018

5chdn commented Oct 15, 2018

ordian left a comment •

edited

Loading

ordian Oct 16, 2018

ordian Oct 16, 2018

ordian Oct 16, 2018

ordian Oct 16, 2018

niklasad1 commented Oct 16, 2018 •

edited

Loading

niklasad1 commented Oct 19, 2018

fix(light/response) : handle bad responses #9756

fix(light/response) : handle bad responses #9756

Conversation

niklasad1 commented Oct 15, 2018 • edited Loading

This PR adds the following:

niklasad1 Oct 15, 2018 • edited Loading

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

5chdn commented Oct 15, 2018

ordian left a comment • edited Loading

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

niklasad1 commented Oct 16, 2018 • edited Loading

niklasad1 commented Oct 19, 2018

niklasad1 commented Oct 15, 2018 •

edited

Loading

niklasad1 Oct 15, 2018 •

edited

Loading

ordian left a comment •

edited

Loading

niklasad1 commented Oct 16, 2018 •

edited

Loading