From 5a34f86e770dedae20d4c383293bdb8cce722000 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 22 Aug 2019 16:14:51 +1000 Subject: [PATCH] Address some review comments --- beacon_node/client/src/bootstrapper.rs | 16 ++++++++-------- beacon_node/rest_api/src/beacon.rs | 22 ++++++---------------- beacon_node/rest_api/src/helpers.rs | 15 +++++++++++++++ 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/beacon_node/client/src/bootstrapper.rs b/beacon_node/client/src/bootstrapper.rs index 9843ceec7..19f13e2da 100644 --- a/beacon_node/client/src/bootstrapper.rs +++ b/beacon_node/client/src/bootstrapper.rs @@ -10,7 +10,7 @@ use url::Host; #[derive(Debug)] enum Error { - UrlCannotBeBase, + InvalidUrl, HttpError(HttpError), } @@ -38,7 +38,7 @@ impl Bootstrapper { /// Build a multiaddr using the HTTP server URL that is not guaranteed to be correct. /// - /// The address is created by querying the HTTP server for it's listening libp2p addresses. + /// The address is created by querying the HTTP server for its listening libp2p addresses. /// Then, we find the first TCP port in those addresses and combine the port with the URL of /// the server. /// @@ -124,7 +124,7 @@ fn get_slots_per_epoch(mut url: Url) -> Result { .map(|mut url| { url.push("spec").push("slots_per_epoch"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; reqwest::get(url)? .error_for_status()? @@ -137,7 +137,7 @@ fn get_finalized_slot(mut url: Url, slots_per_epoch: u64) -> Result .map(|mut url| { url.push("beacon").push("latest_finalized_checkpoint"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; let checkpoint: Checkpoint = reqwest::get(url)?.error_for_status()?.json()?; @@ -149,7 +149,7 @@ fn get_state(mut url: Url, slot: Slot) -> Result, Err .map(|mut url| { url.push("beacon").push("state"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; url.query_pairs_mut() .append_pair("slot", &format!("{}", slot.as_u64())); @@ -165,7 +165,7 @@ fn get_block(mut url: Url, slot: Slot) -> Result, Err .map(|mut url| { url.push("beacon").push("block"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; url.query_pairs_mut() .append_pair("slot", &format!("{}", slot.as_u64())); @@ -181,7 +181,7 @@ fn get_enr(mut url: Url) -> Result { .map(|mut url| { url.push("node").push("network").push("enr"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; reqwest::get(url)? .error_for_status()? @@ -194,7 +194,7 @@ fn get_listen_addresses(mut url: Url) -> Result, Error> { .map(|mut url| { url.push("node").push("network").push("listen_addresses"); }) - .map_err(|_| Error::UrlCannotBeBase)?; + .map_err(|_| Error::InvalidUrl)?; reqwest::get(url)? .error_for_status()? diff --git a/beacon_node/rest_api/src/beacon.rs b/beacon_node/rest_api/src/beacon.rs index 88427c9a4..4e3cc02fd 100644 --- a/beacon_node/rest_api/src/beacon.rs +++ b/beacon_node/rest_api/src/beacon.rs @@ -54,14 +54,9 @@ pub fn get_block(req: Request) -> ApiResult ("slot", value) => { let target = parse_slot(&value)?; - beacon_chain - .rev_iter_block_roots() - .take_while(|(_root, slot)| *slot >= target) - .find(|(_root, slot)| *slot == target) - .map(|(root, _slot)| root) - .ok_or_else(|| { - ApiError::NotFound(format!("Unable to find BeaconBlock for slot {}", target)) - })? + block_root_at_slot(&beacon_chain, target).ok_or_else(|| { + ApiError::NotFound(format!("Unable to find BeaconBlock for slot {}", target)) + })? } ("root", value) => parse_root(&value)?, _ => return Err(ApiError::ServerError("Unexpected query parameter".into())), @@ -99,14 +94,9 @@ pub fn get_block_root(req: Request) -> ApiR let slot_string = UrlQuery::from_request(&req)?.only_one("slot")?; let target = parse_slot(&slot_string)?; - let root = beacon_chain - .rev_iter_block_roots() - .take_while(|(_root, slot)| *slot >= target) - .find(|(_root, slot)| *slot == target) - .map(|(root, _slot)| root) - .ok_or_else(|| { - ApiError::NotFound(format!("Unable to find BeaconBlock for slot {}", target)) - })?; + let root = block_root_at_slot(&beacon_chain, target).ok_or_else(|| { + ApiError::NotFound(format!("Unable to find BeaconBlock for slot {}", target)) + })?; let json: String = serde_json::to_string(&root) .map_err(|e| ApiError::ServerError(format!("Unable to serialize root: {:?}", e)))?; diff --git a/beacon_node/rest_api/src/helpers.rs b/beacon_node/rest_api/src/helpers.rs index a65c7c1ac..5365086df 100644 --- a/beacon_node/rest_api/src/helpers.rs +++ b/beacon_node/rest_api/src/helpers.rs @@ -31,6 +31,21 @@ pub fn parse_root(string: &str) -> Result { } } +/// Returns the root of the `BeaconBlock` in the canonical chain of `beacon_chain` at the given +/// `slot`, if possible. +/// +/// May return a root for a previous slot, in the case of skip slots. +pub fn block_root_at_slot( + beacon_chain: &BeaconChain, + target: Slot, +) -> Option { + beacon_chain + .rev_iter_block_roots() + .take_while(|(_root, slot)| *slot >= target) + .find(|(_root, slot)| *slot == target) + .map(|(root, _slot)| root) +} + /// Returns a `BeaconState` and it's root in the canonical chain of `beacon_chain` at the given /// `slot`, if possible. ///