Improve on-demand dispatch and add support for batch requests by rphmeier · Pull Request #5419 · openethereum/parity-ethereum

rphmeier · 2017-04-06T16:05:39Z

After #5383, #5320
Closes #4593
Partially addresses #5311

This PR adds batch-request support to the OnDemand service. Batch responses are handled up to the point where they are incomplete or incorrect, and the remainder will be reassigned to another peer.

There's still a lot of room for improvement in terms of multiplexing different logical batches into different packets, along with submitting portions of batches at a time instead of submitting the whole thing (which will cause heavy requests to potentially never be served) but this come later.

Requests are now prioritized by submission order.

A strongly-typed API for OnDemand has been added; for tuples up to a certain size, you can submit

(request_type_a, request_type_b, request_type_c)

and get back a

(response_type_a, response_type_b, response_type_c)

The RPCs haven't been ported to use the new system yet, so they should be as inefficient as ever. The next PR will do this, along with implementing back-referencing for on-demand requests, where verification data can be extracted from prior responses.

rphmeier · 2017-04-06T18:06:09Z

Strongly-typed API in place:

let (header, code, account) = on_demand.request(&ctx, (hdr_req, code_req, account_req))
    .expect("all back-references ok").wait();

tomusdrw

Couple of nitpicks, will need to dive into the logic deeper.

tomusdrw · 2017-05-12T08:15:13Z

 			cache.block_hash(&req.num()).and_then(|hash| cache.chain_score(&hash))
 		};

 		match cached {


Seems like a lot of repetition with getting stuff from cache, maybe use something like this?

fn from_cache<T, F: FnOnce(&mut Cache) -> T>(f: F) -> Option<BoxFuture<T, Canceled>> { //.. } self.from_cache(|cache| cache.block_hash(&req.num).and_then(|hash| cache.chain_score(hash))) .unwrap_or_else(|| self.request(ctx, req)...)

All these methods are removed and replaced with a more general answer_from_cache in #5573, would prefer to leave this refactor specifically to that PR.

tomusdrw · 2017-05-12T08:27:06Z

 impl LightFetch {
 	/// Get a block header from the on demand service or client, or error.
-	pub fn header(&self, id: BlockId) -> BoxFuture<Option<encoded::Header>, Error> {
+	pub fn header(&self, id: BlockId) -> BoxFuture<encoded::Header, Error> {


Could be futures::Either with a typedef instead of Box everywhere.

yep, although I think optimizing this stuff is beyond the scope of the PR. After #5573 , most of these RPC helpers won't need to dispatch futures, and will rather just push onto a request builder.

tomusdrw · 2017-05-12T08:28:14Z

+					// three possible outcomes:
+					//   - network is down.
+					//   - we get a score, but our hash is non-canonical.
+					//   - we get ascore, and our hash is canonical.


s/ascore/a score

tomusdrw · 2017-05-12T08:31:38Z

 	}
+
+	fn adjust_refs<F>(&mut self, mapping: F) where F: FnMut(usize) -> usize {
+		match *self {


Wouldn't it make sense to implement Request::request() method on the enum itself, seems that pattern matching every kind is quite common in this file. (Probably same with Response)

Yes, although there are currently tests for behavior of the individual types as well as their enum-wrapped counterparts, and implementing just for the enum would make these tests impossible. It's kind of a drag to do all the matching, but I think this code could probably be refactored better with macros

tomusdrw · 2017-05-12T08:38:08Z

-		// fill as much of the next request as we can.
-		if let Some(ref mut req) = self.requests.get_mut(self.answered) {
+		// fill as much of each remaining request as we can.
+		for req in self.requests.iter_mut().skip(self.answered) {


Aren't we filling up the same requests couple of times? Seems that supply_response is called on every iteration of the loop in respond_to_all. Is req::fill only requesting the outputs that it doesn't already have?

We do iterate over all remaining requests upon each supplied response, but as you guessed it will only request outputs that haven't already been filled.

An optimization would be to just fill the next request upon each supplied response and then do a sweep of all unanswered requests after supplying all responses.

tomusdrw · 2017-05-12T08:41:18Z

 /// Requests pending responses.
 #[derive(Debug, Clone, PartialEq, Eq)]
-pub struct Requests {
+pub struct Requests<T: IncompleteRequest> {


Why put constraints here?

I'll fix that, it's definitely unidiomatic

tomusdrw · 2017-05-12T08:44:36Z

+}
+
+#[test]
+fn detects_hangup() {


Loving this file, the tests looks so clean 👍

tomusdrw · 2017-05-12T09:06:38Z

+		// update pending fields and re-queue.
+		let capabilities = guess_capabilities(&pending.requests.requests()[num_answered..]);
+		pending.net_requests = builder.build();
+		pending.required_capabilities = capabilities;


Shoudn't you update pending.requests here as well (i.e. remove the answered ones?)

no, pending.requests is intentionally not shortened. although that approach would work, I thought it made more sense to keep the original requests batch around and only adjust back-references when building the net_requests, which should lead to slightly less work over-all.

tomusdrw · 2017-05-12T09:07:24Z

 use util::sha3::Hashable;
 use util::trie::{Trie, TrieDB, TrieError};

+const SUPPLIED_MATCHES: &'static str = "supplied responses always match produced requests; qed";


https://github.com/paritytech/parity/pull/5419/files/0fd3e36c23123ddd7f33b3891979214e3f23ceb0#diff-abb081d3b17a1ecf465e845b3fef0728R354

this match ensures that a response is invalid if it doesn't have the correct type for the request.

Might be worth including the function name in the proof then. Something like:
check_request called earlier so supplied response matches produced request; qed

tomusdrw · 2017-05-12T09:08:24Z

+				prover.check_response(cache, &res.code).map(Response::Code),
+			(&CheckedRequest::Execution(ref prover, _), &NetResponse::Execution(ref res)) =>
+				prover.check_response(cache, &res.items).map(Response::Execution),
+			_ => Err(Error::WrongKind),


I think it would be more future proof to enumerate all CheckedRequest (so that when the protocol is extended you immediatelly see all places requiring implementation), but I know it would make the code here a bit more ugly (and macro would be required):

match *self { CheckedRequest::Account(..) => match!(NetResponse::Account(), response) { } }

honestly, that looks cleaner and I never even considered it :)

rphmeier added 16 commits March 28, 2017 11:23

block_hash method for LightChainClient

da3dd65

abstraction and futures-based eth_filter

a6b6c31

log fetching for light client

f409168

add eth-filter delegate

d8893b9

eth_block fetching RPCs

73fa0cd

return default accounts from on_demand

1b0a369

Merge branch 'lightcli' into light-filters

8c715e0

Merge branch 'lightcli' into light-filters

60ce0ae

generalize RequestBuilder

3574045

checked request for OnDemand

08d8709

convert Request to CheckedRequest

3eea777

dispatch batched requests

574cfae

use cache in on-demand again

d19232a

fix RPC fallout

528dbf9

Merge branch 'block_header_rpc' into on-demand-priority

cf75a19

typestrong API

5793bb8

rphmeier added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Apr 6, 2017

tests for on_demand

68ec7ae

rphmeier added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Apr 10, 2017

rphmeier added 2 commits April 10, 2017 19:53

Merge branch 'master' into on-demand-priority

bbe0eb9

Merge branch 'master' into on-demand-priority

a98052f

rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Apr 13, 2017

correct state test checkout

fd4d7c4

rphmeier mentioned this pull request May 7, 2017

Back-references for the on-demand service #5573

Merged

rphmeier added 2 commits May 9, 2017 14:24

fix indentation

9358f81

Merge branch 'master' into on-demand-priority

0fd3e36

rphmeier mentioned this pull request May 10, 2017

OnDemand dispatch improvements #5311

Closed

tomusdrw reviewed May 12, 2017

View reviewed changes

rphmeier added 5 commits May 12, 2017 17:16

address grumbles

2d87f56

optimize back-reference filling

909f3d7

removing slienced warning

2a6f38c

code cleanup with macro

f8279bb

improve assertion

c7cf43d

tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels May 16, 2017

rphmeier merged commit 5d973f8 into master May 17, 2017

rphmeier deleted the on-demand-priority branch May 17, 2017 10:28

Conversation

rphmeier commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rphmeier commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rphmeier commented Apr 6, 2017 •

edited

Loading

rphmeier commented Apr 6, 2017 •

edited

Loading

rphmeier May 12, 2017 •

edited

Loading

rphmeier May 12, 2017 •

edited

Loading