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

feat(light client): fetch block body from remote #2527

Merged
merged 13 commits into from
May 18, 2019
Merged

feat(light client): fetch block body from remote #2527

merged 13 commits into from
May 18, 2019

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented May 9, 2019 •
edited
Loading

Attempt to close #1445

@niklasad1 niklasad1 requested a review from svyatonik May 9, 2019 16:39
@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

1 similar comment
@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@niklasad1 niklasad1 added the A0-please_review Pull request needs code review. label May 9, 2019
andresilva
andresilva reviewed May 9, 2019
View reviewed changes
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

In order to avoid creating new network messages, when we receive a BlockResponse we check whether the request is for on_demand (i.e. there's a pending request there for that id), and otherwise fall through and pass the request to sync.

core/client/src/light/fetcher.rs Show resolved Hide resolved
Demi-Marie
Demi-Marie previously requested changes May 9, 2019
View reviewed changes
Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

This patch needs a minor amount of refactoring.

core/client/src/light/blockchain.rs
header,
retry_count: None,
})
.into_future().wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really return a Future instead of blocking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I agree, but this is a part of the trait Backend impl which doesn't support futures. I guess this relies on http timeouts instead?!

However, this is out-of-scope of this PR and could probably refactored to be async instead.

core/client/src/light/fetcher.rs Outdated Show resolved Hide resolved
core/network/src/on_demand.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 force-pushed the na-on-demand-block-body branch 2 times, most recently from 4f25cef to ef6569f Compare May 14, 2019 14:27
@niklasad1 niklasad1 force-pushed the na-on-demand-block-body branch from ef6569f to 13e4957 Compare May 15, 2019 08:40
niklasad1
niklasad1 commented May 15, 2019
View reviewed changes
core/network/src/protocol.rs
self.update_peer_info(&who);
return outcome
// Note, this is safe because only `ordinary bodies` and `remote bodies` are received in this matter.
if self.is_on_demand_response(&who, r.id) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Perform this check first because otherwise the peer might be disconnected/punished.

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @andresilva something like you had in mind?

It is kind of ugly to introduce is_on_demand_response but because of encapsulation/trait object I don't see any other options!

@niklasad1 niklasad1 force-pushed the na-on-demand-block-body branch from 13e4957 to eef6baa Compare May 15, 2019 08:47
svyatonik
svyatonik approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Looks good. Probably add couple of tests (positive, when response is accepted and negative, when that's extrinsic root mismatch) to client/src/light/fetcher.rs (could be done in follow up PR)?

@niklasad1 niklasad1 force-pushed the na-on-demand-block-body branch 2 times, most recently from 0085eef to d65b5a2 Compare May 16, 2019 08:13
@niklasad1
Copy link
Member Author

niklasad1 commented May 16, 2019 •
edited
Loading

Probably add couple of tests (positive, when response is accepted and negative, when that's extrinsic root mismatch) to client/src/light/fetcher.rs (could be done in follow up PR)?

Yeah, I will take care of that in this PR

@niklasad1 niklasad1 force-pushed the na-on-demand-block-body branch from d65b5a2 to d1aa286 Compare May 17, 2019 13:58
niklasad1
niklasad1 commented May 17, 2019
View reviewed changes
core/client/src/light/fetcher.rs
@@ -191,6 +205,12 @@ pub trait FetchChecker<Block: BlockT>: Send + Sync {
request: &RemoteChangesRequest<Block::Header>,
proof: ChangesProof<Block::Header>
) -> ClientResult<Vec<(NumberFor<Block>, u32)>>;
/// Check remote body proof.
fn check_body_proof(
Copy link
Member Author

Choose a reason for hiding this comment

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

moved the extrinsic_root verification here to test it!

@Demi-Marie Demi-Marie merged commit 8ac5cad into master May 18, 2019
@Demi-Marie Demi-Marie deleted the na-on-demand-block-body branch May 18, 2019 00:05
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Jul 10, 2019
@niklasad1 @MTDK1
feat(light client): fetch block body from remote (paritytech#2527)
0e97c3c 
* feat(on_demand): block body request

* fix(light block req): no justific + one block

* fix(bad rebase)

* feat(protocol): add messages for `remote_body`

* fix(on demand body): remove needless `take()`

* fix(network): remove messages for `on_demand_body`

* fix(grumbles): use `hash` in `remote_body_requests`

As long as we can't compute `ordered_trie_root(body)` just compare that request.header.hash() == response.header.hash()

* fix(grumbles): `hdr.ext_root == trie_root(body)`

* fix(grumbles): propogate `Err` in `fn body()`

* fix(grumbles): Vec<Block::Extrinsic>

* fix(grumbles): util_fn for `not_impl` in tests

* fix(on remote body): tests `fetch` and `on_demand`

* docs(resolve todos)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DemiMarie-temp DemiMarie-temp DemiMarie-temp left review comments

@andresilva andresilva andresilva left review comments

@svyatonik svyatonik svyatonik approved these changes

@Demi-Marie Demi-Marie Demi-Marie left review comments

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Light clients should fetch block body from remote.
6 participants
@niklasad1 @parity-cla-bot @andresilva @svyatonik @Demi-Marie @DemiMarie-temp