From 94941a892e9c625124afa8f3b28aedc5a6274059 Mon Sep 17 00:00:00 2001 From: Dmitrii Markin Date: Thu, 13 Oct 2022 12:24:31 +0300 Subject: [PATCH 1/7] Punish peers for duplicate GRANDPA neighbor messages (#12462) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Decrease peer reputation for duplicate GRANDPA neighbor messages. * Fix comparison * Fix update_peer_state() validity condition * Add negative test * Rework update_peer_state() validity condition, add tests * update_peer_state() validity condition: invert comparison * Split InvalidViewChange and DuplicateNeighborMessage misbehaviors * Enforce rate-limiting of duplicate GRANDPA neighbor packets * Update client/finality-grandpa/src/communication/gossip.rs Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> * Make rolling clock back in a test safer Co-authored-by: André Silva <123550+andresilva@users.noreply.github.com> --- .../src/communication/gossip.rs | 151 +++++++++++++----- .../finality-grandpa/src/communication/mod.rs | 8 +- .../src/communication/periodic.rs | 17 +- 3 files changed, 124 insertions(+), 52 deletions(-) diff --git a/client/finality-grandpa/src/communication/gossip.rs b/client/finality-grandpa/src/communication/gossip.rs index 1ba5e0da33c96..218b4b668c10f 100644 --- a/client/finality-grandpa/src/communication/gossip.rs +++ b/client/finality-grandpa/src/communication/gossip.rs @@ -35,7 +35,8 @@ //! impolite to send messages about r+1 or later. "future-round" messages can //! be dropped and ignored. //! -//! It is impolite to send a neighbor packet which moves backwards in protocol state. +//! It is impolite to send a neighbor packet which moves backwards or does not progress +//! protocol state. //! //! This is beneficial if it conveys some progress in the protocol state of the peer. //! @@ -97,7 +98,7 @@ use sc_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnbound use sp_finality_grandpa::AuthorityId; use sp_runtime::traits::{Block as BlockT, NumberFor, Zero}; -use super::{benefit, cost, Round, SetId}; +use super::{benefit, cost, Round, SetId, NEIGHBOR_REBROADCAST_PERIOD}; use crate::{environment, CatchUp, CompactCommit, SignedMessage}; use std::{ @@ -148,14 +149,15 @@ enum Consider { /// A view of protocol state. #[derive(Debug)] struct View { - round: Round, // the current round we are at. - set_id: SetId, // the current voter set id. - last_commit: Option, // commit-finalized block height, if any. + round: Round, // the current round we are at. + set_id: SetId, // the current voter set id. + last_commit: Option, // commit-finalized block height, if any. + last_update: Option, // last time we heard from peer, used for spamming detection. } impl Default for View { fn default() -> Self { - View { round: Round(1), set_id: SetId(0), last_commit: None } + View { round: Round(1), set_id: SetId(0), last_commit: None, last_update: None } } } @@ -225,7 +227,12 @@ impl LocalView { /// Converts the local view to a `View` discarding round and set id /// information about the last commit. fn as_view(&self) -> View<&N> { - View { round: self.round, set_id: self.set_id, last_commit: self.last_commit_height() } + View { + round: self.round, + set_id: self.set_id, + last_commit: self.last_commit_height(), + last_update: None, + } } /// Update the set ID. implies a reset to round 1. @@ -417,6 +424,8 @@ pub(super) struct FullCatchUpMessage { pub(super) enum Misbehavior { // invalid neighbor message, considering the last one. InvalidViewChange, + // duplicate neighbor message. + DuplicateNeighborMessage, // could not decode neighbor message. bytes-length of the packet. UndecodablePacket(i32), // Bad catch up message (invalid signatures). @@ -438,6 +447,7 @@ impl Misbehavior { match *self { InvalidViewChange => cost::INVALID_VIEW_CHANGE, + DuplicateNeighborMessage => cost::DUPLICATE_NEIGHBOR_MESSAGE, UndecodablePacket(bytes) => ReputationChange::new( bytes.saturating_mul(cost::PER_UNDECODABLE_BYTE), "Grandpa: Bad packet", @@ -488,20 +498,22 @@ struct Peers { second_stage_peers: HashSet, /// The randomly picked set of `LUCKY_PEERS` light clients we'll gossip commit messages to. lucky_light_peers: HashSet, + /// Neighbor packet rebroadcast period --- we reduce the reputation of peers sending duplicate + /// packets too often. + neighbor_rebroadcast_period: Duration, } -impl Default for Peers { - fn default() -> Self { +impl Peers { + fn new(neighbor_rebroadcast_period: Duration) -> Self { Peers { inner: Default::default(), first_stage_peers: Default::default(), second_stage_peers: Default::default(), lucky_light_peers: Default::default(), + neighbor_rebroadcast_period, } } -} -impl Peers { fn new_peer(&mut self, who: PeerId, role: ObservedRole) { match role { ObservedRole::Authority if self.first_stage_peers.len() < LUCKY_PEERS => { @@ -547,10 +559,23 @@ impl Peers { return Err(Misbehavior::InvalidViewChange) } + let now = Instant::now(); + let duplicate_packet = (update.set_id, update.round, Some(&update.commit_finalized_height)) == + (peer.view.set_id, peer.view.round, peer.view.last_commit.as_ref()); + + if duplicate_packet { + if let Some(last_update) = peer.view.last_update { + if now < last_update + self.neighbor_rebroadcast_period / 2 { + return Err(Misbehavior::DuplicateNeighborMessage) + } + } + } + peer.view = View { round: update.round, set_id: update.set_id, last_commit: Some(update.commit_finalized_height), + last_update: Some(now), }; trace!(target: "afg", "Peer {} updated view. Now at {:?}, {:?}", @@ -748,7 +773,7 @@ impl Inner { Inner { local_view: None, - peers: Peers::default(), + peers: Peers::new(NEIGHBOR_REBROADCAST_PERIOD), live_topics: KeepTopics::new(), next_rebroadcast: Instant::now() + REBROADCAST_AFTER, authorities: Vec::new(), @@ -758,13 +783,16 @@ impl Inner { } } - /// Note a round in the current set has started. + /// Note a round in the current set has started. Does nothing if the last + /// call to the function was with the same `round`. fn note_round(&mut self, round: Round) -> MaybeMessage { { let local_view = match self.local_view { None => return None, Some(ref mut v) => if v.round == round { + // Do not send neighbor packets out if `round` has not changed --- + // such behavior is punishable. return None } else { v @@ -803,6 +831,8 @@ impl Inner { ); self.authorities = authorities; } + // Do not send neighbor packets out if the `set_id` has not changed --- + // such behavior is punishable. return None } else { v @@ -816,7 +846,9 @@ impl Inner { self.multicast_neighbor_packet() } - /// Note that we've imported a commit finalizing a given block. + /// Note that we've imported a commit finalizing a given block. Does nothing if the last + /// call to the function was with the same or higher `finalized` number. + /// `set_id` & `round` are the ones the commit message is from. fn note_commit_finalized( &mut self, round: Round, @@ -1357,6 +1389,8 @@ impl GossipValidator { } /// Note that we've imported a commit finalizing a given block. + /// `set_id` & `round` are the ones the commit message is from and not necessarily + /// the latest set ID & round started. pub(super) fn note_commit_finalized( &self, round: Round, @@ -1647,11 +1681,12 @@ pub(super) struct PeerReport { #[cfg(test)] mod tests { - use super::{environment::SharedVoterSetState, *}; + use super::{super::NEIGHBOR_REBROADCAST_PERIOD, environment::SharedVoterSetState, *}; use crate::communication; use sc_network::config::Role; use sc_network_gossip::Validator as GossipValidatorT; use sp_core::{crypto::UncheckedFrom, H256}; + use std::time::Instant; use substrate_test_runtime_client::runtime::{Block, Header}; // some random config (not really needed) @@ -1684,7 +1719,12 @@ mod tests { #[test] fn view_vote_rules() { - let view = View { round: Round(100), set_id: SetId(1), last_commit: Some(1000u64) }; + let view = View { + round: Round(100), + set_id: SetId(1), + last_commit: Some(1000u64), + last_update: None, + }; assert_eq!(view.consider_vote(Round(98), SetId(1)), Consider::RejectPast); assert_eq!(view.consider_vote(Round(1), SetId(0)), Consider::RejectPast); @@ -1701,7 +1741,12 @@ mod tests { #[test] fn view_global_message_rules() { - let view = View { round: Round(100), set_id: SetId(2), last_commit: Some(1000u64) }; + let view = View { + round: Round(100), + set_id: SetId(2), + last_commit: Some(1000u64), + last_update: None, + }; assert_eq!(view.consider_global(SetId(3), 1), Consider::RejectFuture); assert_eq!(view.consider_global(SetId(3), 1000), Consider::RejectFuture); @@ -1719,7 +1764,7 @@ mod tests { #[test] fn unknown_peer_cannot_be_updated() { - let mut peers = Peers::default(); + let mut peers = Peers::new(NEIGHBOR_REBROADCAST_PERIOD); let id = PeerId::random(); let update = @@ -1750,27 +1795,35 @@ mod tests { let update4 = NeighborPacket { round: Round(3), set_id: SetId(11), commit_finalized_height: 80 }; - let mut peers = Peers::default(); + // Use shorter rebroadcast period to safely roll the clock back in the last test + // and don't hit the system boot time on systems with unsigned time. + const SHORT_NEIGHBOR_REBROADCAST_PERIOD: Duration = Duration::from_secs(1); + let mut peers = Peers::new(SHORT_NEIGHBOR_REBROADCAST_PERIOD); let id = PeerId::random(); peers.new_peer(id, ObservedRole::Authority); - let mut check_update = move |update: NeighborPacket<_>| { + let check_update = |peers: &mut Peers<_>, update: NeighborPacket<_>| { let view = peers.update_peer_state(&id, update.clone()).unwrap().unwrap(); assert_eq!(view.round, update.round); assert_eq!(view.set_id, update.set_id); assert_eq!(view.last_commit, Some(update.commit_finalized_height)); }; - check_update(update1); - check_update(update2); - check_update(update3); - check_update(update4); + check_update(&mut peers, update1); + check_update(&mut peers, update2); + check_update(&mut peers, update3); + check_update(&mut peers, update4.clone()); + + // Allow duplicate neighbor packets if enough time has passed. + peers.inner.get_mut(&id).unwrap().view.last_update = + Some(Instant::now() - SHORT_NEIGHBOR_REBROADCAST_PERIOD); + check_update(&mut peers, update4); } #[test] fn invalid_view_change() { - let mut peers = Peers::default(); + let mut peers = Peers::new(NEIGHBOR_REBROADCAST_PERIOD); let id = PeerId::random(); peers.new_peer(id, ObservedRole::Authority); @@ -1783,29 +1836,41 @@ mod tests { .unwrap() .unwrap(); - let mut check_update = move |update: NeighborPacket<_>| { + let mut check_update = move |update: NeighborPacket<_>, misbehavior| { let err = peers.update_peer_state(&id, update.clone()).unwrap_err(); - assert_eq!(err, Misbehavior::InvalidViewChange); + assert_eq!(err, misbehavior); }; // round moves backwards. - check_update(NeighborPacket { - round: Round(9), - set_id: SetId(10), - commit_finalized_height: 10, - }); - // commit finalized height moves backwards. - check_update(NeighborPacket { - round: Round(10), - set_id: SetId(10), - commit_finalized_height: 9, - }); + check_update( + NeighborPacket { round: Round(9), set_id: SetId(10), commit_finalized_height: 10 }, + Misbehavior::InvalidViewChange, + ); // set ID moves backwards. - check_update(NeighborPacket { - round: Round(10), - set_id: SetId(9), - commit_finalized_height: 10, - }); + check_update( + NeighborPacket { round: Round(10), set_id: SetId(9), commit_finalized_height: 10 }, + Misbehavior::InvalidViewChange, + ); + // commit finalized height moves backwards. + check_update( + NeighborPacket { round: Round(10), set_id: SetId(10), commit_finalized_height: 9 }, + Misbehavior::InvalidViewChange, + ); + // duplicate packet without grace period. + check_update( + NeighborPacket { round: Round(10), set_id: SetId(10), commit_finalized_height: 10 }, + Misbehavior::DuplicateNeighborMessage, + ); + // commit finalized height moves backwards while round moves forward. + check_update( + NeighborPacket { round: Round(11), set_id: SetId(10), commit_finalized_height: 9 }, + Misbehavior::InvalidViewChange, + ); + // commit finalized height moves backwards while set ID moves forward. + check_update( + NeighborPacket { round: Round(10), set_id: SetId(11), commit_finalized_height: 9 }, + Misbehavior::InvalidViewChange, + ); } #[test] diff --git a/client/finality-grandpa/src/communication/mod.rs b/client/finality-grandpa/src/communication/mod.rs index 12cb2601f4c26..75a7697812c6c 100644 --- a/client/finality-grandpa/src/communication/mod.rs +++ b/client/finality-grandpa/src/communication/mod.rs @@ -37,6 +37,7 @@ use std::{ pin::Pin, sync::Arc, task::{Context, Poll}, + time::Duration, }; use finality_grandpa::{ @@ -68,6 +69,9 @@ mod periodic; #[cfg(test)] pub(crate) mod tests; +// How often to rebroadcast neighbor packets, in cases where no new packets are created. +pub(crate) const NEIGHBOR_REBROADCAST_PERIOD: Duration = Duration::from_secs(2 * 60); + pub mod grandpa_protocol_name { use sc_chain_spec::ChainSpec; use sc_network_common::protocol::ProtocolName; @@ -103,6 +107,8 @@ mod cost { pub(super) const UNKNOWN_VOTER: Rep = Rep::new(-150, "Grandpa: Unknown voter"); pub(super) const INVALID_VIEW_CHANGE: Rep = Rep::new(-500, "Grandpa: Invalid view change"); + pub(super) const DUPLICATE_NEIGHBOR_MESSAGE: Rep = + Rep::new(-500, "Grandpa: Duplicate neighbor message without grace period"); pub(super) const PER_UNDECODABLE_BYTE: i32 = -5; pub(super) const PER_SIGNATURE_CHECKED: i32 = -25; pub(super) const PER_BLOCK_LOADED: i32 = -10; @@ -279,7 +285,7 @@ impl> NetworkBridge { } let (neighbor_packet_worker, neighbor_packet_sender) = - periodic::NeighborPacketWorker::new(); + periodic::NeighborPacketWorker::new(NEIGHBOR_REBROADCAST_PERIOD); NetworkBridge { service, diff --git a/client/finality-grandpa/src/communication/periodic.rs b/client/finality-grandpa/src/communication/periodic.rs index e6d63beafc362..c001796b5ca5d 100644 --- a/client/finality-grandpa/src/communication/periodic.rs +++ b/client/finality-grandpa/src/communication/periodic.rs @@ -32,9 +32,6 @@ use super::gossip::{GossipMessage, NeighborPacket}; use sc_network::PeerId; use sp_runtime::traits::{Block as BlockT, NumberFor}; -// How often to rebroadcast, in cases where no new packets are created. -const REBROADCAST_AFTER: Duration = Duration::from_secs(2 * 60); - /// A sender used to send neighbor packets to a background job. #[derive(Clone)] pub(super) struct NeighborPacketSender( @@ -60,6 +57,7 @@ impl NeighborPacketSender { /// implementation). Periodically it sends out the last packet in cases where no new ones arrive. pub(super) struct NeighborPacketWorker { last: Option<(Vec, NeighborPacket>)>, + rebroadcast_period: Duration, delay: Delay, rx: TracingUnboundedReceiver<(Vec, NeighborPacket>)>, } @@ -67,13 +65,16 @@ pub(super) struct NeighborPacketWorker { impl Unpin for NeighborPacketWorker {} impl NeighborPacketWorker { - pub(super) fn new() -> (Self, NeighborPacketSender) { + pub(super) fn new(rebroadcast_period: Duration) -> (Self, NeighborPacketSender) { let (tx, rx) = tracing_unbounded::<(Vec, NeighborPacket>)>( "mpsc_grandpa_neighbor_packet_worker", ); - let delay = Delay::new(REBROADCAST_AFTER); + let delay = Delay::new(rebroadcast_period); - (NeighborPacketWorker { last: None, delay, rx }, NeighborPacketSender(tx)) + ( + NeighborPacketWorker { last: None, rebroadcast_period, delay, rx }, + NeighborPacketSender(tx), + ) } } @@ -85,7 +86,7 @@ impl Stream for NeighborPacketWorker { match this.rx.poll_next_unpin(cx) { Poll::Ready(None) => return Poll::Ready(None), Poll::Ready(Some((to, packet))) => { - this.delay.reset(REBROADCAST_AFTER); + this.delay.reset(this.rebroadcast_period); this.last = Some((to.clone(), packet.clone())); return Poll::Ready(Some((to, GossipMessage::::from(packet)))) @@ -98,7 +99,7 @@ impl Stream for NeighborPacketWorker { // Getting this far here implies that the timer fired. - this.delay.reset(REBROADCAST_AFTER); + this.delay.reset(this.rebroadcast_period); // Make sure the underlying task is scheduled for wake-up. // From 983b6b0e5d93a3f7d99d8b3d3a8bb398af3ec045 Mon Sep 17 00:00:00 2001 From: Aaro Altonen <48052676+altonen@users.noreply.github.com> Date: Thu, 13 Oct 2022 12:37:09 +0300 Subject: [PATCH 2/7] Introduce mockable `ChainSync` object for testing (#12480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Introduce mockable `ChainSync` object for testing `mockall` allows to mock `ChainSync` and to verify that the calls made to `ChaiSync` are firstly executed at all, that they're executed in correct order and with correct parameters. This allows to verify, e.g., that delegating calls directly to `ChainSync` from `NetworkService` still calls the correct functions with correct arguments even if `Protocol` middleman is removed. * Add Cargo.lock * Fix tests * Update client/network/Cargo.toml Co-authored-by: Bastian Köcher * Update Cargo.lock * Fix clippy and documentation Co-authored-by: Bastian Köcher Co-authored-by: parity-processbot <> --- Cargo.lock | 58 +++ client/network/common/src/sync.rs | 14 +- client/network/src/protocol.rs | 2 +- client/network/src/service.rs | 2 + client/network/src/service/chainsync_tests.rs | 339 ++++++++++++++++++ client/network/sync/Cargo.toml | 1 + client/network/sync/src/lib.rs | 35 +- client/network/sync/src/mock.rs | 118 ++++++ 8 files changed, 546 insertions(+), 23 deletions(-) create mode 100644 client/network/src/service/chainsync_tests.rs create mode 100644 client/network/sync/src/mock.rs diff --git a/Cargo.lock b/Cargo.lock index d29023f330ce1..062195c70f8ab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1717,6 +1717,12 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fea41bba32d969b513997752735605054bc0dfa92b4c56bf1189f2e174be7a10" +[[package]] +name = "downcast" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1435fa1053d8b2fbbe9be7e97eca7f33d37b28409959813daefc1446a14247f1" + [[package]] name = "downcast-rs" version = "1.2.0" @@ -2093,6 +2099,15 @@ dependencies = [ "miniz_oxide", ] +[[package]] +name = "float-cmp" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "98de4bbd547a563b716d8dfa9aad1cb19bfab00f4fa09a6a4ed21dbcf44ce9c4" +dependencies = [ + "num-traits", +] + [[package]] name = "fnv" version = "1.0.7" @@ -2116,6 +2131,12 @@ dependencies = [ "percent-encoding", ] +[[package]] +name = "fragile" +version = "1.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85dcb89d2b10c5f6133de2efd8c11959ce9dbb46a2f7a4cab208c4eeda6ce1ab" + [[package]] name = "frame-benchmarking" version = "4.0.0-dev" @@ -4395,6 +4416,33 @@ dependencies = [ "windows-sys 0.36.1", ] +[[package]] +name = "mockall" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2be9a9090bc1cac2930688fa9478092a64c6a92ddc6ae0692d46b37d9cab709" +dependencies = [ + "cfg-if 1.0.0", + "downcast", + "fragile", + "lazy_static", + "mockall_derive", + "predicates", + "predicates-tree", +] + +[[package]] +name = "mockall_derive" +version = "0.11.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "86d702a0530a0141cf4ed147cf5ec7be6f2c187d4e37fcbefc39cf34116bfe8f" +dependencies = [ + "cfg-if 1.0.0", + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "more-asserts" version = "0.2.1" @@ -4970,6 +5018,12 @@ dependencies = [ "version_check", ] +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + [[package]] name = "num-bigint" version = "0.2.6" @@ -6942,8 +6996,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c143348f141cc87aab5b950021bac6145d0e5ae754b0591de23244cee42c9308" dependencies = [ "difflib", + "float-cmp", "itertools", + "normalize-line-endings", "predicates-core", + "regex", ] [[package]] @@ -8586,6 +8643,7 @@ dependencies = [ "libp2p", "log", "lru", + "mockall", "parity-scale-codec", "prost 0.11.0", "prost-build 0.11.1", diff --git a/client/network/common/src/sync.rs b/client/network/common/src/sync.rs index 020b2c9efa4c7..d3603c6792c84 100644 --- a/client/network/common/src/sync.rs +++ b/client/network/common/src/sync.rs @@ -269,12 +269,14 @@ pub trait ChainSync: Send { ); /// Get an iterator over all scheduled justification requests. - fn justification_requests( - &mut self, - ) -> Box)> + '_>; + fn justification_requests<'a>( + &'a mut self, + ) -> Box)> + 'a>; /// Get an iterator over all block requests of all peers. - fn block_requests(&mut self) -> Box)> + '_>; + fn block_requests<'a>( + &'a mut self, + ) -> Box)> + 'a>; /// Get a state request, if any. fn state_request(&mut self) -> Option<(PeerId, OpaqueStateRequest)>; @@ -359,9 +361,9 @@ pub trait ChainSync: Send { /// /// If [`PollBlockAnnounceValidation::ImportHeader`] is returned, then the caller MUST try to /// import passed header (call `on_block_data`). The network request isn't sent in this case. - fn poll_block_announce_validation( + fn poll_block_announce_validation<'a>( &mut self, - cx: &mut std::task::Context, + cx: &mut std::task::Context<'a>, ) -> Poll>; /// Call when a peer has disconnected. diff --git a/client/network/src/protocol.rs b/client/network/src/protocol.rs index 325e044527efa..c3def8adc6cfe 100644 --- a/client/network/src/protocol.rs +++ b/client/network/src/protocol.rs @@ -1440,7 +1440,7 @@ where for (id, request) in self .chain_sync .block_requests() - .map(|(peer_id, request)| (*peer_id, request)) + .map(|(peer_id, request)| (peer_id, request)) .collect::>() { let event = diff --git a/client/network/src/service.rs b/client/network/src/service.rs index 28e479b702779..25916041285a3 100644 --- a/client/network/src/service.rs +++ b/client/network/src/service.rs @@ -92,6 +92,8 @@ use std::{ pub use behaviour::{InboundFailure, OutboundFailure, ResponseFailure}; +#[cfg(test)] +mod chainsync_tests; mod metrics; mod out_events; #[cfg(test)] diff --git a/client/network/src/service/chainsync_tests.rs b/client/network/src/service/chainsync_tests.rs new file mode 100644 index 0000000000000..ca44c65d267f4 --- /dev/null +++ b/client/network/src/service/chainsync_tests.rs @@ -0,0 +1,339 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +use crate::{config, NetworkWorker}; + +use futures::prelude::*; +use libp2p::PeerId; +use sc_block_builder::BlockBuilderProvider; +use sc_client_api::{BlockBackend, HeaderBackend}; +use sc_consensus::JustificationSyncLink; +use sc_network_common::{ + config::{ + NonDefaultSetConfig, NonReservedPeerMode, NotificationHandshake, ProtocolId, SetConfig, + TransportConfig, + }, + protocol::role::Roles, + service::NetworkSyncForkRequest, + sync::{message::BlockAnnouncesHandshake, ChainSync as ChainSyncT, SyncState, SyncStatus}, +}; +use sc_network_light::light_client_requests::handler::LightClientRequestHandler; +use sc_network_sync::{ + block_request_handler::BlockRequestHandler, mock::MockChainSync, + state_request_handler::StateRequestHandler, +}; +use sp_core::H256; +use sp_runtime::{ + generic::BlockId, + traits::{Block as BlockT, Header as _, Zero}, +}; +use std::{iter, sync::Arc, task::Poll}; +use substrate_test_runtime_client::{TestClientBuilder, TestClientBuilderExt as _}; + +type TestNetworkWorker = NetworkWorker< + substrate_test_runtime_client::runtime::Block, + substrate_test_runtime_client::runtime::Hash, + substrate_test_runtime_client::TestClient, +>; + +const BLOCK_ANNOUNCE_PROTO_NAME: &str = "/block-announces"; +const PROTOCOL_NAME: &str = "/foo"; + +fn make_network( + chain_sync: Box>, + client: Arc, +) -> (TestNetworkWorker, Arc) { + let network_config = config::NetworkConfiguration { + extra_sets: vec![NonDefaultSetConfig { + notifications_protocol: PROTOCOL_NAME.into(), + fallback_names: Vec::new(), + max_notification_size: 1024 * 1024, + handshake: None, + set_config: Default::default(), + }], + listen_addresses: vec![config::build_multiaddr![Memory(rand::random::())]], + transport: TransportConfig::MemoryOnly, + ..config::NetworkConfiguration::new_local() + }; + + #[derive(Clone)] + struct PassThroughVerifier(bool); + + #[async_trait::async_trait] + impl sc_consensus::Verifier for PassThroughVerifier { + async fn verify( + &mut self, + mut block: sc_consensus::BlockImportParams, + ) -> Result< + ( + sc_consensus::BlockImportParams, + Option)>>, + ), + String, + > { + let maybe_keys = block + .header + .digest() + .log(|l| { + l.try_as_raw(sp_runtime::generic::OpaqueDigestItemId::Consensus(b"aura")) + .or_else(|| { + l.try_as_raw(sp_runtime::generic::OpaqueDigestItemId::Consensus( + b"babe", + )) + }) + }) + .map(|blob| { + vec![(sp_blockchain::well_known_cache_keys::AUTHORITIES, blob.to_vec())] + }); + + block.finalized = self.0; + block.fork_choice = Some(sc_consensus::ForkChoiceStrategy::LongestChain); + Ok((block, maybe_keys)) + } + } + + let import_queue = Box::new(sc_consensus::BasicQueue::new( + PassThroughVerifier(false), + Box::new(client.clone()), + None, + &sp_core::testing::TaskExecutor::new(), + None, + )); + + let protocol_id = ProtocolId::from("/test-protocol-name"); + + let fork_id = Some(String::from("test-fork-id")); + + let block_request_protocol_config = { + let (handler, protocol_config) = + BlockRequestHandler::new(&protocol_id, None, client.clone(), 50); + async_std::task::spawn(handler.run().boxed()); + protocol_config + }; + + let state_request_protocol_config = { + let (handler, protocol_config) = + StateRequestHandler::new(&protocol_id, None, client.clone(), 50); + async_std::task::spawn(handler.run().boxed()); + protocol_config + }; + + let light_client_request_protocol_config = { + let (handler, protocol_config) = + LightClientRequestHandler::new(&protocol_id, None, client.clone()); + async_std::task::spawn(handler.run().boxed()); + protocol_config + }; + + let block_announce_config = NonDefaultSetConfig { + notifications_protocol: BLOCK_ANNOUNCE_PROTO_NAME.into(), + fallback_names: vec![], + max_notification_size: 1024 * 1024, + handshake: Some(NotificationHandshake::new(BlockAnnouncesHandshake::< + substrate_test_runtime_client::runtime::Block, + >::build( + Roles::from(&config::Role::Full), + client.info().best_number, + client.info().best_hash, + client + .block_hash(Zero::zero()) + .ok() + .flatten() + .expect("Genesis block exists; qed"), + ))), + set_config: SetConfig { + in_peers: 0, + out_peers: 0, + reserved_nodes: Vec::new(), + non_reserved_mode: NonReservedPeerMode::Deny, + }, + }; + + let worker = NetworkWorker::new(config::Params { + block_announce_config, + role: config::Role::Full, + executor: None, + network_config, + chain: client.clone(), + protocol_id, + fork_id, + import_queue, + chain_sync, + metrics_registry: None, + block_request_protocol_config, + state_request_protocol_config, + light_client_request_protocol_config, + warp_sync_protocol_config: None, + request_response_protocol_configs: Vec::new(), + }) + .unwrap(); + + (worker, client) +} + +fn set_default_expecations_no_peers( + chain_sync: &mut MockChainSync, +) { + chain_sync.expect_block_requests().returning(|| Box::new(iter::empty())); + chain_sync.expect_state_request().returning(|| None); + chain_sync.expect_justification_requests().returning(|| Box::new(iter::empty())); + chain_sync.expect_warp_sync_request().returning(|| None); + chain_sync.expect_poll_block_announce_validation().returning(|_| Poll::Pending); + chain_sync.expect_status().returning(|| SyncStatus { + state: SyncState::Idle, + best_seen_block: None, + num_peers: 0u32, + queued_blocks: 0u32, + state_sync: None, + warp_sync: None, + }); +} + +#[async_std::test] +async fn normal_network_poll_no_peers() { + let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0); + let mut chain_sync = + Box::new(MockChainSync::::new()); + set_default_expecations_no_peers(&mut chain_sync); + + let (mut network, _) = make_network(chain_sync, client); + + // poll the network once + futures::future::poll_fn(|cx| { + let _ = network.poll_unpin(cx); + Poll::Ready(()) + }) + .await; +} + +#[async_std::test] +async fn request_justification() { + let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0); + let mut chain_sync = + Box::new(MockChainSync::::new()); + + let hash = H256::random(); + let number = 1337u64; + + chain_sync + .expect_request_justification() + .withf(move |in_hash, in_number| &hash == in_hash && &number == in_number) + .once() + .returning(|_, _| ()); + + set_default_expecations_no_peers(&mut chain_sync); + let (mut network, _) = make_network(chain_sync, client); + + // send "request justifiction" message and poll the network + network.service().request_justification(&hash, number); + + futures::future::poll_fn(|cx| { + let _ = network.poll_unpin(cx); + Poll::Ready(()) + }) + .await; +} + +#[async_std::test] +async fn clear_justification_requests(&mut self) { + let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0); + let mut chain_sync = + Box::new(MockChainSync::::new()); + + chain_sync.expect_clear_justification_requests().once().returning(|| ()); + + set_default_expecations_no_peers(&mut chain_sync); + let (mut network, _) = make_network(chain_sync, client); + + // send "request justifiction" message and poll the network + network.service().clear_justification_requests(); + + futures::future::poll_fn(|cx| { + let _ = network.poll_unpin(cx); + Poll::Ready(()) + }) + .await; +} + +#[async_std::test] +async fn set_sync_fork_request() { + let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0); + let mut chain_sync = + Box::new(MockChainSync::::new()); + + let hash = H256::random(); + let number = 1337u64; + let peers = (0..3).map(|_| PeerId::random()).collect::>(); + let copy_peers = peers.clone(); + + chain_sync + .expect_set_sync_fork_request() + .withf(move |in_peers, in_hash, in_number| { + &peers == in_peers && &hash == in_hash && &number == in_number + }) + .once() + .returning(|_, _, _| ()); + + set_default_expecations_no_peers(&mut chain_sync); + let (mut network, _) = make_network(chain_sync, client); + + // send "set sync fork request" message and poll the network + network.service().set_sync_fork_request(copy_peers, hash, number); + + futures::future::poll_fn(|cx| { + let _ = network.poll_unpin(cx); + Poll::Ready(()) + }) + .await; +} + +#[async_std::test] +async fn on_block_finalized() { + let client = Arc::new(TestClientBuilder::with_default_backend().build_with_longest_chain().0); + let mut chain_sync = + Box::new(MockChainSync::::new()); + + let at = client.header(&BlockId::Hash(client.info().best_hash)).unwrap().unwrap().hash(); + let block = client + .new_block_at(&BlockId::Hash(at), Default::default(), false) + .unwrap() + .build() + .unwrap() + .block; + let header = block.header.clone(); + let block_number = *header.number(); + let hash = block.hash(); + + chain_sync + .expect_on_block_finalized() + .withf(move |in_hash, in_number| &hash == in_hash && &block_number == in_number) + .once() + .returning(|_, _| ()); + + set_default_expecations_no_peers(&mut chain_sync); + let (mut network, _) = make_network(chain_sync, client); + + // send "set sync fork request" message and poll the network + network.on_block_finalized(hash, header); + + futures::future::poll_fn(|cx| { + let _ = network.poll_unpin(cx); + Poll::Ready(()) + }) + .await; +} diff --git a/client/network/sync/Cargo.toml b/client/network/sync/Cargo.toml index 24d418f7233d7..9d032f5cca96c 100644 --- a/client/network/sync/Cargo.toml +++ b/client/network/sync/Cargo.toml @@ -25,6 +25,7 @@ futures = "0.3.21" libp2p = "0.46.1" log = "0.4.17" lru = "0.7.5" +mockall = "0.11.2" prost = "0.11" smallvec = "1.8.0" thiserror = "1.0" diff --git a/client/network/sync/src/lib.rs b/client/network/sync/src/lib.rs index 280e530eca9a9..84998c747b3cc 100644 --- a/client/network/sync/src/lib.rs +++ b/client/network/sync/src/lib.rs @@ -30,6 +30,7 @@ pub mod block_request_handler; pub mod blocks; +pub mod mock; mod schema; pub mod state; pub mod state_request_handler; @@ -643,9 +644,9 @@ where .extend(peers); } - fn justification_requests( - &mut self, - ) -> Box)> + '_> { + fn justification_requests<'a>( + &'a mut self, + ) -> Box)> + 'a> { let peers = &mut self.peers; let mut matcher = self.extra_justifications.matcher(); Box::new(std::iter::from_fn(move || { @@ -670,7 +671,9 @@ where })) } - fn block_requests(&mut self) -> Box)> + '_> { + fn block_requests<'a>( + &'a mut self, + ) -> Box)> + 'a> { if self.mode == SyncMode::Warp { return Box::new(std::iter::once(self.warp_target_block_request()).flatten()) } @@ -695,8 +698,8 @@ where let allowed_requests = self.allowed_requests.take(); let max_parallel = if major_sync { 1 } else { self.max_parallel_downloads }; let gap_sync = &mut self.gap_sync; - let iter = self.peers.iter_mut().filter_map(move |(id, peer)| { - if !peer.state.is_available() || !allowed_requests.contains(id) { + let iter = self.peers.iter_mut().filter_map(move |(&id, peer)| { + if !peer.state.is_available() || !allowed_requests.contains(&id) { return None } @@ -725,7 +728,7 @@ where }; Some((id, ancestry_request::(current))) } else if let Some((range, req)) = peer_block_request( - id, + &id, peer, blocks, attrs, @@ -744,7 +747,7 @@ where ); Some((id, req)) } else if let Some((hash, req)) = - fork_sync_request(id, fork_targets, best_queued, last_finalized, attrs, |hash| { + fork_sync_request(&id, fork_targets, best_queued, last_finalized, attrs, |hash| { if queue.contains(hash) { BlockStatus::Queued } else { @@ -756,7 +759,7 @@ where Some((id, req)) } else if let Some((range, req)) = gap_sync.as_mut().and_then(|sync| { peer_gap_block_request( - id, + &id, peer, &mut sync.blocks, attrs, @@ -2216,7 +2219,7 @@ where } /// Generate block request for downloading of the target block body during warp sync. - fn warp_target_block_request(&mut self) -> Option<(&PeerId, BlockRequest)> { + fn warp_target_block_request(&mut self) -> Option<(PeerId, BlockRequest)> { if let Some(sync) = &self.warp_sync { if self.allowed_requests.is_empty() || sync.is_complete() || @@ -2234,7 +2237,7 @@ where trace!(target: "sync", "New warp target block request for {}", id); peer.state = PeerSyncState::DownloadingWarpTargetBlock; self.allowed_requests.clear(); - return Some((id, request)) + return Some((*id, request)) } } } @@ -2482,7 +2485,7 @@ fn fork_sync_request( true }); for (hash, r) in targets { - if !r.peers.contains(id) { + if !r.peers.contains(&id) { continue } // Download the fork only if it is behind or not too far ahead our tip of the chain @@ -2740,7 +2743,7 @@ mod test { // we wil send block requests to these peers // for these blocks we don't know about - assert!(sync.block_requests().all(|(p, _)| { *p == peer_id1 || *p == peer_id2 })); + assert!(sync.block_requests().all(|(p, _)| { p == peer_id1 || p == peer_id2 })); // add a new peer at a known block sync.new_peer(peer_id3, b1_hash, b1_number).unwrap(); @@ -2835,7 +2838,7 @@ mod test { log::trace!(target: "sync", "Requests: {:?}", requests); assert_eq!(1, requests.len()); - assert_eq!(peer, requests[0].0); + assert_eq!(*peer, requests[0].0); let request = requests[0].1.clone(); @@ -3065,9 +3068,9 @@ mod test { send_block_announce(best_block.header().clone(), &peer_id2, &mut sync); let (peer1_req, peer2_req) = sync.block_requests().fold((None, None), |res, req| { - if req.0 == &peer_id1 { + if req.0 == peer_id1 { (Some(req.1), res.1) - } else if req.0 == &peer_id2 { + } else if req.0 == peer_id2 { (res.0, Some(req.1)) } else { panic!("Unexpected req: {:?}", req) diff --git a/client/network/sync/src/mock.rs b/client/network/sync/src/mock.rs new file mode 100644 index 0000000000000..2a3b059f735b2 --- /dev/null +++ b/client/network/sync/src/mock.rs @@ -0,0 +1,118 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0 + +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. + +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. + +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +//! Contains a mock implementation of `ChainSync` that can be used +//! for testing calls made to `ChainSync`. + +use futures::task::Poll; +use libp2p::PeerId; +use sc_consensus::{BlockImportError, BlockImportStatus}; +use sc_network_common::sync::{ + message::{BlockAnnounce, BlockData, BlockRequest, BlockResponse}, + warp::{EncodedProof, WarpProofRequest}, + BadPeer, ChainSync as ChainSyncT, Metrics, OnBlockData, OnBlockJustification, OnStateData, + OpaqueBlockRequest, OpaqueBlockResponse, OpaqueStateRequest, OpaqueStateResponse, PeerInfo, + PollBlockAnnounceValidation, SyncStatus, +}; +use sp_runtime::traits::{Block as BlockT, NumberFor}; + +mockall::mock! { + pub ChainSync {} + + impl ChainSyncT for ChainSync { + fn peer_info(&self, who: &PeerId) -> Option>; + fn status(&self) -> SyncStatus; + fn num_sync_requests(&self) -> usize; + fn num_downloaded_blocks(&self) -> usize; + fn num_peers(&self) -> usize; + fn new_peer( + &mut self, + who: PeerId, + best_hash: Block::Hash, + best_number: NumberFor, + ) -> Result>, BadPeer>; + fn update_chain_info(&mut self, best_hash: &Block::Hash, best_number: NumberFor); + fn request_justification(&mut self, hash: &Block::Hash, number: NumberFor); + fn clear_justification_requests(&mut self); + fn set_sync_fork_request( + &mut self, + peers: Vec, + hash: &Block::Hash, + number: NumberFor, + ); + fn justification_requests<'a>( + &'a mut self, + ) -> Box)> + 'a>; + fn block_requests<'a>(&'a mut self) -> Box)> + 'a>; + fn state_request(&mut self) -> Option<(PeerId, OpaqueStateRequest)>; + fn warp_sync_request(&mut self) -> Option<(PeerId, WarpProofRequest)>; + fn on_block_data( + &mut self, + who: &PeerId, + request: Option>, + response: BlockResponse, + ) -> Result, BadPeer>; + fn on_state_data( + &mut self, + who: &PeerId, + response: OpaqueStateResponse, + ) -> Result, BadPeer>; + fn on_warp_sync_data(&mut self, who: &PeerId, response: EncodedProof) -> Result<(), BadPeer>; + fn on_block_justification( + &mut self, + who: PeerId, + response: BlockResponse, + ) -> Result, BadPeer>; + fn on_blocks_processed( + &mut self, + imported: usize, + count: usize, + results: Vec<(Result>, BlockImportError>, Block::Hash)>, + ) -> Box), BadPeer>>>; + fn on_justification_import( + &mut self, + hash: Block::Hash, + number: NumberFor, + success: bool, + ); + fn on_block_finalized(&mut self, hash: &Block::Hash, number: NumberFor); + fn push_block_announce_validation( + &mut self, + who: PeerId, + hash: Block::Hash, + announce: BlockAnnounce, + is_best: bool, + ); + fn poll_block_announce_validation<'a>( + &mut self, + cx: &mut std::task::Context<'a>, + ) -> Poll>; + fn peer_disconnected(&mut self, who: &PeerId) -> Option>; + fn metrics(&self) -> Metrics; + fn create_opaque_block_request(&self, request: &BlockRequest) -> OpaqueBlockRequest; + fn encode_block_request(&self, request: &OpaqueBlockRequest) -> Result, String>; + fn decode_block_response(&self, response: &[u8]) -> Result; + fn block_response_into_blocks( + &self, + request: &BlockRequest, + response: OpaqueBlockResponse, + ) -> Result>, String>; + fn encode_state_request(&self, request: &OpaqueStateRequest) -> Result, String>; + fn decode_state_response(&self, response: &[u8]) -> Result; + } +} From 1f39c9029eb83e9432d86877f0694f643b7dd968 Mon Sep 17 00:00:00 2001 From: Sergej Sakac <73715684+Szegoo@users.noreply.github.com> Date: Thu, 13 Oct 2022 12:13:56 +0200 Subject: [PATCH 3/7] pallet-mmr: RPC API and Runtime API work with block numbers (#12345) * pallet-mmr: RPC API works with block_numbers * fixes * update rpc * fmt * final touches in the rpc * temporary fix * fix * fmt * docs * Update lib.rs * use NumberFor * validate input * update runtime * convert block_number to u64 * small edit * update runtime api * test fix * runtime fix * update test function * fmt * fix nits * remove block_num_to_leaf_index from runtime api * Update frame/merkle-mountain-range/src/lib.rs Co-authored-by: Robert Hambrock * fix tests * get the code to compile after merge * get the tests to compile * fix in tests? * fix test * Update frame/merkle-mountain-range/src/tests.rs Co-authored-by: Adrian Catangiu * Update frame/merkle-mountain-range/src/lib.rs Co-authored-by: Adrian Catangiu * Update primitives/merkle-mountain-range/src/lib.rs Co-authored-by: Adrian Catangiu * fix errors & nits * change block_num_to_leaf_index * don't make any assumptions * Update frame/merkle-mountain-range/src/tests.rs Co-authored-by: Adrian Catangiu * Update frame/merkle-mountain-range/src/tests.rs Co-authored-by: Adrian Catangiu * Update frame/merkle-mountain-range/src/tests.rs Co-authored-by: Adrian Catangiu * fix * small fix * use best_known_block_number * best_known_block_number instead of leaves_count * more readable? * remove warning * Update frame/merkle-mountain-range/src/lib.rs Co-authored-by: Robert Hambrock * simplify * update docs * nits * fmt & fix * merge fixes * fix * small fix * docs & nit fixes * Nit fixes * remove leaf_indices_to_block_numbers() * fmt Co-authored-by: Robert Hambrock Co-authored-by: Adrian Catangiu --- bin/node/rpc/src/lib.rs | 6 +- bin/node/runtime/src/lib.rs | 20 +++-- client/beefy/src/lib.rs | 4 +- client/beefy/src/tests.rs | 16 ++-- client/beefy/src/worker.rs | 2 +- frame/merkle-mountain-range/rpc/src/lib.rs | 59 +++++++-------- frame/merkle-mountain-range/src/lib.rs | 56 +++++++++++--- frame/merkle-mountain-range/src/tests.rs | 81 +++++++++++---------- primitives/beefy/src/mmr.rs | 6 +- primitives/merkle-mountain-range/src/lib.rs | 23 +++--- 10 files changed, 160 insertions(+), 113 deletions(-) diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 94e01619c6e63..8596fe23321ba 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -108,7 +108,11 @@ where + Send + 'static, C::Api: substrate_frame_rpc_system::AccountNonceApi, - C::Api: pallet_mmr_rpc::MmrRuntimeApi::Hash>, + C::Api: pallet_mmr_rpc::MmrRuntimeApi< + Block, + ::Hash, + BlockNumber, + >, C::Api: pallet_transaction_payment_rpc::TransactionPaymentRuntimeApi, C::Api: BabeApi, C::Api: BlockBuilder, diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 142173621036d..f137b36eff036 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -2016,11 +2016,15 @@ impl_runtime_apis! { } } - impl pallet_mmr::primitives::MmrApi for Runtime { - fn generate_proof(leaf_index: pallet_mmr::primitives::LeafIndex) + impl pallet_mmr::primitives::MmrApi< + Block, + mmr::Hash, + BlockNumber, + > for Runtime { + fn generate_proof(block_number: BlockNumber) -> Result<(mmr::EncodableOpaqueLeaf, mmr::Proof), mmr::Error> { - Mmr::generate_batch_proof(vec![leaf_index]).and_then(|(leaves, proof)| + Mmr::generate_batch_proof(vec![block_number]).and_then(|(leaves, proof)| Ok(( mmr::EncodableOpaqueLeaf::from_leaf(&leaves[0]), mmr::BatchProof::into_single_leaf_proof(proof)? @@ -2052,9 +2056,9 @@ impl_runtime_apis! { } fn generate_batch_proof( - leaf_indices: Vec, + block_numbers: Vec, ) -> Result<(Vec, mmr::BatchProof), mmr::Error> { - Mmr::generate_batch_proof(leaf_indices).map(|(leaves, proof)| { + Mmr::generate_batch_proof(block_numbers).map(|(leaves, proof)| { ( leaves .into_iter() @@ -2066,10 +2070,10 @@ impl_runtime_apis! { } fn generate_historical_batch_proof( - leaf_indices: Vec, - leaves_count: pallet_mmr::primitives::LeafIndex, + block_numbers: Vec, + best_known_block_number: BlockNumber, ) -> Result<(Vec, mmr::BatchProof), mmr::Error> { - Mmr::generate_historical_batch_proof(leaf_indices, leaves_count).map( + Mmr::generate_historical_batch_proof(block_numbers, best_known_block_number).map( |(leaves, proof)| { ( leaves diff --git a/client/beefy/src/lib.rs b/client/beefy/src/lib.rs index 1c61cac072207..441f6e4248117 100644 --- a/client/beefy/src/lib.rs +++ b/client/beefy/src/lib.rs @@ -24,7 +24,7 @@ use sc_consensus::BlockImport; use sc_network::ProtocolName; use sc_network_common::service::NetworkRequest; use sc_network_gossip::Network as GossipNetwork; -use sp_api::ProvideRuntimeApi; +use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_blockchain::HeaderBackend; use sp_consensus::{Error as ConsensusError, SyncOracle}; use sp_keystore::SyncCryptoStorePtr; @@ -200,7 +200,7 @@ where C: Client + BlockBackend, P: PayloadProvider, R: ProvideRuntimeApi, - R::Api: BeefyApi + MmrApi, + R::Api: BeefyApi + MmrApi>, N: GossipNetwork + NetworkRequest + SyncOracle + Send + Sync + 'static, { let BeefyParams { diff --git a/client/beefy/src/tests.rs b/client/beefy/src/tests.rs index 24cf89acd5734..89be1cac4f886 100644 --- a/client/beefy/src/tests.rs +++ b/client/beefy/src/tests.rs @@ -43,9 +43,7 @@ use beefy_primitives::{ KEY_TYPE as BeefyKeyType, }; use sc_network::{config::RequestResponseConfig, ProtocolName}; -use sp_mmr_primitives::{ - BatchProof, EncodableOpaqueLeaf, Error as MmrError, LeafIndex, MmrApi, Proof, -}; +use sp_mmr_primitives::{BatchProof, EncodableOpaqueLeaf, Error as MmrError, MmrApi, Proof}; use sp_api::{ApiRef, ProvideRuntimeApi}; use sp_consensus::BlockOrigin; @@ -247,8 +245,8 @@ macro_rules! create_test_api { } } - impl MmrApi for RuntimeApi { - fn generate_proof(_leaf_index: LeafIndex) + impl MmrApi> for RuntimeApi { + fn generate_proof(_block_number: u64) -> Result<(EncodableOpaqueLeaf, Proof), MmrError> { unimplemented!() } @@ -270,13 +268,13 @@ macro_rules! create_test_api { Ok($mmr_root) } - fn generate_batch_proof(_leaf_indices: Vec) -> Result<(Vec, BatchProof), MmrError> { + fn generate_batch_proof(_block_numbers: Vec) -> Result<(Vec, BatchProof), MmrError> { unimplemented!() } fn generate_historical_batch_proof( - _leaf_indices: Vec, - _leaves_count: LeafIndex + _block_numbers: Vec, + _best_known_block_number: u64 ) -> Result<(Vec, BatchProof), MmrError> { unimplemented!() } @@ -349,7 +347,7 @@ fn initialize_beefy( ) -> impl Future where API: ProvideRuntimeApi + Default + Sync + Send, - API::Api: BeefyApi + MmrApi, + API::Api: BeefyApi + MmrApi>, { let tasks = FuturesUnordered::new(); diff --git a/client/beefy/src/worker.rs b/client/beefy/src/worker.rs index a21807c8ee875..4381081f74ebd 100644 --- a/client/beefy/src/worker.rs +++ b/client/beefy/src/worker.rs @@ -252,7 +252,7 @@ where C: Client, P: PayloadProvider, R: ProvideRuntimeApi, - R::Api: BeefyApi + MmrApi, + R::Api: BeefyApi + MmrApi>, N: NetworkEventStream + NetworkRequest + SyncOracle + Send + Sync + Clone + 'static, { /// Return a new BEEFY worker instance. diff --git a/frame/merkle-mountain-range/rpc/src/lib.rs b/frame/merkle-mountain-range/rpc/src/lib.rs index e939ff8ae7cd0..ffc7ac2da56bf 100644 --- a/frame/merkle-mountain-range/rpc/src/lib.rs +++ b/frame/merkle-mountain-range/rpc/src/lib.rs @@ -30,10 +30,10 @@ use jsonrpsee::{ }; use serde::{Deserialize, Serialize}; -use sp_api::ProvideRuntimeApi; +use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_blockchain::HeaderBackend; use sp_core::Bytes; -use sp_mmr_primitives::{BatchProof, Error as MmrError, LeafIndex, Proof}; +use sp_mmr_primitives::{BatchProof, Error as MmrError, Proof}; use sp_runtime::{generic::BlockId, traits::Block as BlockT}; pub use sp_mmr_primitives::MmrApi as MmrRuntimeApi; @@ -96,11 +96,11 @@ impl LeafBatchProof { /// MMR RPC methods. #[rpc(client, server)] -pub trait MmrApi { - /// Generate MMR proof for given leaf index. +pub trait MmrApi { + /// Generate MMR proof for given block number. /// /// This method calls into a runtime with MMR pallet included and attempts to generate - /// MMR proof for leaf at given `leaf_index`. + /// MMR proof for a block with a specified `block_number`. /// Optionally, a block hash at which the runtime should be queried can be specified. /// /// Returns the (full) leaf itself and a proof for this leaf (compact encoding, i.e. hash of @@ -108,49 +108,49 @@ pub trait MmrApi { #[method(name = "mmr_generateProof")] fn generate_proof( &self, - leaf_index: LeafIndex, + block_number: BlockNumber, at: Option, ) -> RpcResult>; - /// Generate MMR proof for the given leaf indices. + /// Generate MMR proof for the given block numbers. /// /// This method calls into a runtime with MMR pallet included and attempts to generate - /// MMR proof for a set of leaves at the given `leaf_indices`. + /// MMR proof for a set of blocks with the specific `block_numbers`. /// Optionally, a block hash at which the runtime should be queried can be specified. /// /// Returns the leaves and a proof for these leaves (compact encoding, i.e. hash of /// the leaves). Both parameters are SCALE-encoded. /// The order of entries in the `leaves` field of the returned struct - /// is the same as the order of the entries in `leaf_indices` supplied + /// is the same as the order of the entries in `block_numbers` supplied #[method(name = "mmr_generateBatchProof")] fn generate_batch_proof( &self, - leaf_indices: Vec, + block_numbers: Vec, at: Option, ) -> RpcResult>; - /// Generate a MMR proof for the given `leaf_indices` of the MMR that had `leaves_count` leaves. + /// Generate a MMR proof for the given `block_numbers` given the `best_known_block_number`. /// /// This method calls into a runtime with MMR pallet included and attempts to generate - /// a MMR proof for the set of leaves at the given `leaf_indices` with MMR fixed to the state - /// with exactly `leaves_count` leaves. `leaves_count` must be larger than all `leaf_indices` - /// for the function to succeed. + /// a MMR proof for the set of blocks that have the given `block_numbers` with MMR given the + /// `best_known_block_number`. `best_known_block_number` must be larger than all the + /// `block_numbers` for the function to succeed. /// /// Optionally, a block hash at which the runtime should be queried can be specified. /// Note that specifying the block hash isn't super-useful here, unless you're generating /// proof using non-finalized blocks where there are several competing forks. That's because - /// MMR state will be fixed to the state with `leaves_count`, which already points to some - /// historical block. + /// MMR state will be fixed to the state with `best_known_block_number`, which already points to + /// some historical block. /// /// Returns the leaves and a proof for these leaves (compact encoding, i.e. hash of /// the leaves). Both parameters are SCALE-encoded. /// The order of entries in the `leaves` field of the returned struct - /// is the same as the order of the entries in `leaf_indices` supplied + /// is the same as the order of the entries in `block_numbers` supplied #[method(name = "mmr_generateHistoricalBatchProof")] fn generate_historical_batch_proof( &self, - leaf_indices: Vec, - leaves_count: LeafIndex, + block_numbers: Vec, + best_known_block_number: BlockNumber, at: Option, ) -> RpcResult>; } @@ -169,16 +169,17 @@ impl Mmr { } #[async_trait] -impl MmrApiServer<::Hash> for Mmr +impl MmrApiServer<::Hash, NumberFor> + for Mmr where Block: BlockT, Client: Send + Sync + 'static + ProvideRuntimeApi + HeaderBackend, - Client::Api: MmrRuntimeApi, + Client::Api: MmrRuntimeApi>, MmrHash: Codec + Send + Sync + 'static, { fn generate_proof( &self, - leaf_index: LeafIndex, + block_number: NumberFor, at: Option<::Hash>, ) -> RpcResult> { let api = self.client.runtime_api(); @@ -188,7 +189,7 @@ where .generate_proof_with_context( &BlockId::hash(block_hash), sp_core::ExecutionContext::OffchainCall(None), - leaf_index, + block_number, ) .map_err(runtime_error_into_rpc_error)? .map_err(mmr_error_into_rpc_error)?; @@ -198,7 +199,7 @@ where fn generate_batch_proof( &self, - leaf_indices: Vec, + block_numbers: Vec>, at: Option<::Hash>, ) -> RpcResult::Hash>> { let api = self.client.runtime_api(); @@ -210,7 +211,7 @@ where .generate_batch_proof_with_context( &BlockId::hash(block_hash), sp_core::ExecutionContext::OffchainCall(None), - leaf_indices, + block_numbers, ) .map_err(runtime_error_into_rpc_error)? .map_err(mmr_error_into_rpc_error)?; @@ -220,8 +221,8 @@ where fn generate_historical_batch_proof( &self, - leaf_indices: Vec, - leaves_count: LeafIndex, + block_numbers: Vec>, + best_known_block_number: NumberFor, at: Option<::Hash>, ) -> RpcResult::Hash>> { let api = self.client.runtime_api(); @@ -233,8 +234,8 @@ where .generate_historical_batch_proof_with_context( &BlockId::hash(block_hash), sp_core::ExecutionContext::OffchainCall(None), - leaf_indices, - leaves_count, + block_numbers, + best_known_block_number, ) .map_err(runtime_error_into_rpc_error)? .map_err(mmr_error_into_rpc_error)?; diff --git a/frame/merkle-mountain-range/src/lib.rs b/frame/merkle-mountain-range/src/lib.rs index 8b4f2b60bc198..ad3ce340496e8 100644 --- a/frame/merkle-mountain-range/src/lib.rs +++ b/frame/merkle-mountain-range/src/lib.rs @@ -59,7 +59,7 @@ use codec::Encode; use frame_support::weights::Weight; use sp_runtime::{ - traits::{self, One, Saturating}, + traits::{self, CheckedSub, One, Saturating}, SaturatedConversion, }; @@ -318,37 +318,73 @@ impl, I: 'static> Pallet { .saturating_add(leaf_index.saturated_into()) } - /// Generate a MMR proof for the given `leaf_indices`. + /// Convert a `block_num` into a leaf index. + fn block_num_to_leaf_index(block_num: T::BlockNumber) -> Result + where + T: frame_system::Config, + { + // leaf_idx = (leaves_count - 1) - (current_block_num - block_num); + let best_block_num = >::block_number(); + let blocks_diff = best_block_num.checked_sub(&block_num).ok_or_else(|| { + primitives::Error::BlockNumToLeafIndex + .log_debug("The provided block_number is greater than the best block number.") + })?; + let blocks_diff_as_leaf_idx = blocks_diff.try_into().map_err(|_| { + primitives::Error::BlockNumToLeafIndex + .log_debug("The `blocks_diff` couldn't be converted to `LeafIndex`.") + })?; + + let leaf_idx = Self::mmr_leaves() + .checked_sub(1) + .and_then(|last_leaf_idx| last_leaf_idx.checked_sub(blocks_diff_as_leaf_idx)) + .ok_or_else(|| { + primitives::Error::BlockNumToLeafIndex + .log_debug("There aren't enough leaves in the chain.") + })?; + Ok(leaf_idx) + } + + /// Generate a MMR proof for the given `block_numbers`. /// /// Note this method can only be used from an off-chain context /// (Offchain Worker or Runtime API call), since it requires /// all the leaves to be present. /// It may return an error or panic if used incorrectly. pub fn generate_batch_proof( - leaf_indices: Vec, + block_numbers: Vec, ) -> Result< (Vec>, primitives::BatchProof<>::Hash>), primitives::Error, > { - Self::generate_historical_batch_proof(leaf_indices, Self::mmr_leaves()) + Self::generate_historical_batch_proof( + block_numbers, + >::block_number(), + ) } - /// Generate a MMR proof for the given `leaf_indices` for the MMR of `leaves_count` size. + /// Generate a MMR proof for the given `block_numbers` given the `best_known_block_number`. /// /// Note this method can only be used from an off-chain context /// (Offchain Worker or Runtime API call), since it requires /// all the leaves to be present. /// It may return an error or panic if used incorrectly. pub fn generate_historical_batch_proof( - leaf_indices: Vec, - leaves_count: LeafIndex, + block_numbers: Vec, + best_known_block_number: T::BlockNumber, ) -> Result< (Vec>, primitives::BatchProof<>::Hash>), primitives::Error, > { - if leaves_count > Self::mmr_leaves() { - return Err(Error::InvalidLeavesCount) - } + let leaves_count = + Self::block_num_to_leaf_index(best_known_block_number)?.saturating_add(1); + + // we need to translate the block_numbers into leaf indices. + let leaf_indices = block_numbers + .iter() + .map(|block_num| -> Result { + Self::block_num_to_leaf_index(*block_num) + }) + .collect::, _>>()?; let mmr: ModuleMmr = mmr::Mmr::new(leaves_count); mmr.generate_batch_proof(leaf_indices) diff --git a/frame/merkle-mountain-range/src/tests.rs b/frame/merkle-mountain-range/src/tests.rs index bcb775ba02819..a63a433029295 100644 --- a/frame/merkle-mountain-range/src/tests.rs +++ b/frame/merkle-mountain-range/src/tests.rs @@ -235,22 +235,21 @@ fn should_generate_proofs_correctly() { // to retrieve full leaf data. register_offchain_ext(&mut ext); ext.execute_with(|| { - // when generate proofs for all leaves - let proofs = (0_u64..crate::NumberOfLeaves::::get()) + let best_block_number = frame_system::Pallet::::block_number(); + // when generate proofs for all leaves. + let proofs = (1_u64..=best_block_number) .into_iter() - .map(|leaf_index| { - crate::Pallet::::generate_batch_proof(vec![leaf_index]).unwrap() - }) + .map(|block_num| crate::Pallet::::generate_batch_proof(vec![block_num]).unwrap()) .collect::>(); // when generate historical proofs for all leaves - let historical_proofs = (0_u64..crate::NumberOfLeaves::::get()) + let historical_proofs = (1_u64..best_block_number) .into_iter() - .map(|leaf_index| { + .map(|block_num| { let mut proofs = vec![]; - for leaves_count in leaf_index + 1..=num_blocks { + for leaves_count in block_num..=num_blocks { proofs.push( crate::Pallet::::generate_historical_batch_proof( - vec![leaf_index], + vec![block_num], leaves_count, ) .unwrap(), @@ -321,7 +320,7 @@ fn should_generate_proofs_correctly() { leaf_count: 3, items: vec![hex( "672c04a9cd05a644789d769daa552d35d8de7c33129f8a7cbf49e595234c4854" - ),], + )], } ) ); @@ -352,6 +351,7 @@ fn should_generate_proofs_correctly() { assert_eq!( proofs[4], ( + // NOTE: the leaf index is equivalent to the block number(in this case 5) - 1 vec![Compact::new(((4, H256::repeat_byte(5)).into(), LeafData::new(5).into(),))], BatchProof { leaf_indices: vec![4], @@ -393,7 +393,7 @@ fn should_generate_proofs_correctly() { } ) ); - assert_eq!(historical_proofs[6][0], proofs[6]); + assert_eq!(historical_proofs[5][1], proofs[5]); }); } @@ -410,11 +410,12 @@ fn should_generate_batch_proof_correctly() { register_offchain_ext(&mut ext); ext.execute_with(|| { // when generate proofs for a batch of leaves - let (.., proof) = crate::Pallet::::generate_batch_proof(vec![0, 4, 5]).unwrap(); + let (.., proof) = crate::Pallet::::generate_batch_proof(vec![1, 5, 6]).unwrap(); // then assert_eq!( proof, BatchProof { + // the leaf indices are equivalent to the above specified block numbers - 1. leaf_indices: vec![0, 4, 5], leaf_count: 7, items: vec![ @@ -427,7 +428,7 @@ fn should_generate_batch_proof_correctly() { // when generate historical proofs for a batch of leaves let (.., historical_proof) = - crate::Pallet::::generate_historical_batch_proof(vec![0, 4, 5], 6).unwrap(); + crate::Pallet::::generate_historical_batch_proof(vec![1, 5, 6], 6).unwrap(); // then assert_eq!( historical_proof, @@ -443,7 +444,7 @@ fn should_generate_batch_proof_correctly() { // when generate historical proofs for a batch of leaves let (.., historical_proof) = - crate::Pallet::::generate_historical_batch_proof(vec![0, 4, 5], 7).unwrap(); + crate::Pallet::::generate_historical_batch_proof(vec![1, 5, 6], 7).unwrap(); // then assert_eq!(historical_proof, proof); }); @@ -500,15 +501,15 @@ fn should_verify() { fn should_verify_batch_proofs() { fn generate_and_verify_batch_proof( ext: &mut sp_io::TestExternalities, - leaf_indices: &Vec, + block_numbers: &Vec, blocks_to_add: usize, ) { let (leaves, proof) = ext.execute_with(|| { - crate::Pallet::::generate_batch_proof(leaf_indices.to_vec()).unwrap() + crate::Pallet::::generate_batch_proof(block_numbers.to_vec()).unwrap() }); let mmr_size = ext.execute_with(|| crate::Pallet::::mmr_leaves()); - let min_mmr_size = leaf_indices.iter().max().unwrap() + 1; + let min_mmr_size = block_numbers.iter().max().unwrap() + 1; // generate historical proofs for all possible mmr sizes, // lower bound being index of highest leaf to be proven @@ -516,7 +517,7 @@ fn should_verify_batch_proofs() { .map(|mmr_size| { ext.execute_with(|| { crate::Pallet::::generate_historical_batch_proof( - leaf_indices.to_vec(), + block_numbers.to_vec(), mmr_size, ) .unwrap() @@ -546,39 +547,41 @@ fn should_verify_batch_proofs() { // to retrieve full leaf data when generating proofs register_offchain_ext(&mut ext); - // verify that up to n=10, valid proofs are generated for all possible leaf combinations - for n in 0..10 { + // verify that up to n=10, valid proofs are generated for all possible block number + // combinations. + for n in 1..=10 { ext.execute_with(|| new_block()); ext.persist_offchain_overlay(); - // generate powerset (skipping empty set) of all possible leaf combinations for mmr size n - let leaves_set: Vec> = (0..=n).into_iter().powerset().skip(1).collect(); + // generate powerset (skipping empty set) of all possible block number combinations for mmr + // size n. + let blocks_set: Vec> = (1..=n).into_iter().powerset().skip(1).collect(); - leaves_set.iter().for_each(|leaves_subset| { - generate_and_verify_batch_proof(&mut ext, leaves_subset, 0); + blocks_set.iter().for_each(|blocks_subset| { + generate_and_verify_batch_proof(&mut ext, &blocks_subset, 0); ext.persist_offchain_overlay(); }); } - // verify that up to n=15, valid proofs are generated for all possible 2-leaf combinations - for n in 10..15 { - // (MMR Leafs) + // verify that up to n=15, valid proofs are generated for all possible 2-block number + // combinations. + for n in 11..=15 { ext.execute_with(|| new_block()); ext.persist_offchain_overlay(); - // generate all possible 2-leaf combinations for mmr size n - let leaves_set: Vec> = (0..=n).into_iter().combinations(2).collect(); + // generate all possible 2-block number combinations for mmr size n. + let blocks_set: Vec> = (1..=n).into_iter().combinations(2).collect(); - leaves_set.iter().for_each(|leaves_subset| { - generate_and_verify_batch_proof(&mut ext, leaves_subset, 0); + blocks_set.iter().for_each(|blocks_subset| { + generate_and_verify_batch_proof(&mut ext, &blocks_subset, 0); ext.persist_offchain_overlay(); }); } - generate_and_verify_batch_proof(&mut ext, &vec![7, 11], 20); + generate_and_verify_batch_proof(&mut ext, &vec![8, 12], 20); ext.execute_with(|| add_blocks(1000)); ext.persist_offchain_overlay(); - generate_and_verify_batch_proof(&mut ext, &vec![7, 11, 100, 800], 100); + generate_and_verify_batch_proof(&mut ext, &vec![8, 12, 100, 800], 100); } #[test] @@ -650,11 +653,11 @@ fn should_verify_batch_proof_statelessly() { register_offchain_ext(&mut ext); let (leaves, proof) = ext.execute_with(|| { // when - crate::Pallet::::generate_batch_proof(vec![0, 4, 5]).unwrap() + crate::Pallet::::generate_batch_proof(vec![1, 4, 5]).unwrap() }); let (historical_leaves, historical_proof) = ext.execute_with(|| { // when - crate::Pallet::::generate_historical_batch_proof(vec![0, 4, 5], 6).unwrap() + crate::Pallet::::generate_historical_batch_proof(vec![1, 4, 5], 6).unwrap() }); // Verify proof without relying on any on-chain data. @@ -920,7 +923,7 @@ fn should_verify_canonicalized() { // Generate proofs for some blocks. let (leaves, proofs) = - ext.execute_with(|| crate::Pallet::::generate_batch_proof(vec![0, 4, 5, 7]).unwrap()); + ext.execute_with(|| crate::Pallet::::generate_batch_proof(vec![1, 4, 5, 7]).unwrap()); // Verify all previously generated proofs. ext.execute_with(|| { assert_eq!(crate::Pallet::::verify_leaves(leaves, proofs), Ok(())); @@ -953,19 +956,19 @@ fn does_not_panic_when_generating_historical_proofs() { // when leaf index is invalid assert_eq!( crate::Pallet::::generate_historical_batch_proof(vec![10], 7), - Err(Error::LeafNotFound), + Err(Error::BlockNumToLeafIndex), ); // when leaves count is invalid assert_eq!( crate::Pallet::::generate_historical_batch_proof(vec![3], 100), - Err(Error::InvalidLeavesCount), + Err(Error::BlockNumToLeafIndex), ); // when both leaf index and leaves count are invalid assert_eq!( crate::Pallet::::generate_historical_batch_proof(vec![10], 100), - Err(Error::InvalidLeavesCount), + Err(Error::BlockNumToLeafIndex), ); }); } diff --git a/primitives/beefy/src/mmr.rs b/primitives/beefy/src/mmr.rs index b479d979f13f3..0edb8babd608e 100644 --- a/primitives/beefy/src/mmr.rs +++ b/primitives/beefy/src/mmr.rs @@ -142,7 +142,7 @@ pub use mmr_root_provider::MmrRootProvider; mod mmr_root_provider { use super::*; use crate::{known_payloads, payload::PayloadProvider, Payload}; - use sp_api::ProvideRuntimeApi; + use sp_api::{NumberFor, ProvideRuntimeApi}; use sp_mmr_primitives::MmrApi; use sp_runtime::generic::BlockId; use sp_std::{marker::PhantomData, sync::Arc}; @@ -159,7 +159,7 @@ mod mmr_root_provider { where B: Block, R: ProvideRuntimeApi, - R::Api: MmrApi, + R::Api: MmrApi>, { /// Create new BEEFY Payload provider with MMR Root as payload. pub fn new(runtime: Arc) -> Self { @@ -182,7 +182,7 @@ mod mmr_root_provider { where B: Block, R: ProvideRuntimeApi, - R::Api: MmrApi, + R::Api: MmrApi>, { fn payload(&self, header: &B::Header) -> Option { self.mmr_root_from_digest_or_runtime(header).map(|mmr_root| { diff --git a/primitives/merkle-mountain-range/src/lib.rs b/primitives/merkle-mountain-range/src/lib.rs index 7a26cae839ea9..06bc1f4bffe84 100644 --- a/primitives/merkle-mountain-range/src/lib.rs +++ b/primitives/merkle-mountain-range/src/lib.rs @@ -387,6 +387,8 @@ impl Proof { /// Merkle Mountain Range operation error. #[derive(RuntimeDebug, codec::Encode, codec::Decode, PartialEq, Eq)] pub enum Error { + /// Error during translation of a block number into a leaf index. + BlockNumToLeafIndex, /// Error while pushing new node. Push, /// Error getting the new root. @@ -403,8 +405,8 @@ pub enum Error { PalletNotIncluded, /// Cannot find the requested leaf index InvalidLeafIndex, - /// The provided leaves count is larger than the actual leaves count. - InvalidLeavesCount, + /// The provided best know block number is invalid. + InvalidBestKnownBlock, } impl Error { @@ -434,9 +436,9 @@ impl Error { sp_api::decl_runtime_apis! { /// API to interact with MMR pallet. - pub trait MmrApi { - /// Generate MMR proof for a leaf under given index. - fn generate_proof(leaf_index: LeafIndex) -> Result<(EncodableOpaqueLeaf, Proof), Error>; + pub trait MmrApi { + /// Generate MMR proof for a block with a specified `block_number`. + fn generate_proof(block_number: BlockNumber) -> Result<(EncodableOpaqueLeaf, Proof), Error>; /// Verify MMR proof against on-chain MMR. /// @@ -457,14 +459,13 @@ sp_api::decl_runtime_apis! { /// Return the on-chain MMR root hash. fn mmr_root() -> Result; - /// Generate MMR proof for a series of leaves under given indices. - fn generate_batch_proof(leaf_indices: Vec) - -> Result<(Vec, BatchProof), Error>; + /// Generate MMR proof for a series of blocks with the specified block numbers. + fn generate_batch_proof(block_numbers: Vec) -> Result<(Vec, BatchProof), Error>; - /// Generate MMR proof for a series of leaves under given indices, using MMR at given `leaves_count` size. + /// Generate MMR proof for a series of `block_numbers`, given the `best_known_block_number`. fn generate_historical_batch_proof( - leaf_indices: Vec, - leaves_count: LeafIndex + block_numbers: Vec, + best_known_block_number: BlockNumber ) -> Result<(Vec, BatchProof), Error>; /// Verify MMR proof against on-chain MMR for a batch of leaves. From fd00b14932d37bee06371325269c163bf80d1d16 Mon Sep 17 00:00:00 2001 From: Koute Date: Thu, 13 Oct 2022 19:31:00 +0900 Subject: [PATCH 4/7] Enable the `wasmtime`-based WASM executor by default (#12486) --- client/cli/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/cli/Cargo.toml b/client/cli/Cargo.toml index 37a8fd2e0b64d..66742e14789ef 100644 --- a/client/cli/Cargo.toml +++ b/client/cli/Cargo.toml @@ -51,6 +51,6 @@ sp-version = { version = "5.0.0", path = "../../primitives/version" } tempfile = "3.1.0" [features] -default = ["rocksdb"] +default = ["rocksdb", "wasmtime"] rocksdb = ["sc-client-db/rocksdb"] wasmtime = ["sc-service/wasmtime"] From 6e04e48f36e1839532d566a4dcad3c57b4be939d Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Thu, 13 Oct 2022 15:22:57 +0200 Subject: [PATCH 5/7] Trivial BlockId::Number => Hash (#12490) --- primitives/api/test/benches/bench.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/primitives/api/test/benches/bench.rs b/primitives/api/test/benches/bench.rs index 2682c91f94106..2445a5c07f09e 100644 --- a/primitives/api/test/benches/bench.rs +++ b/primitives/api/test/benches/bench.rs @@ -27,14 +27,14 @@ fn sp_api_benchmark(c: &mut Criterion) { c.bench_function("add one with same runtime api", |b| { let client = substrate_test_runtime_client::new(); let runtime_api = client.runtime_api(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); b.iter(|| runtime_api.benchmark_add_one(&block_id, &1)) }); c.bench_function("add one with recreating runtime api", |b| { let client = substrate_test_runtime_client::new(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); b.iter(|| client.runtime_api().benchmark_add_one(&block_id, &1)) }); @@ -42,7 +42,7 @@ fn sp_api_benchmark(c: &mut Criterion) { c.bench_function("vector add one with same runtime api", |b| { let client = substrate_test_runtime_client::new(); let runtime_api = client.runtime_api(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); let data = vec![0; 1000]; b.iter_with_large_drop(|| runtime_api.benchmark_vector_add_one(&block_id, &data)) @@ -50,7 +50,7 @@ fn sp_api_benchmark(c: &mut Criterion) { c.bench_function("vector add one with recreating runtime api", |b| { let client = substrate_test_runtime_client::new(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); let data = vec![0; 1000]; b.iter_with_large_drop(|| client.runtime_api().benchmark_vector_add_one(&block_id, &data)) @@ -60,7 +60,7 @@ fn sp_api_benchmark(c: &mut Criterion) { let client = TestClientBuilder::new() .set_execution_strategy(ExecutionStrategy::AlwaysWasm) .build(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); b.iter(|| client.runtime_api().benchmark_indirect_call(&block_id).unwrap()) }); @@ -68,7 +68,7 @@ fn sp_api_benchmark(c: &mut Criterion) { let client = TestClientBuilder::new() .set_execution_strategy(ExecutionStrategy::AlwaysWasm) .build(); - let block_id = BlockId::Number(client.chain_info().best_number); + let block_id = BlockId::Hash(client.chain_info().best_hash); b.iter(|| client.runtime_api().benchmark_direct_call(&block_id).unwrap()) }); } From f3139874cb50f9028ecba9bdbd3004e7f3f228f5 Mon Sep 17 00:00:00 2001 From: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com> Date: Fri, 14 Oct 2022 11:27:32 +0200 Subject: [PATCH 6/7] BlockId removal: refactor: Backend::state_at (#12488) * Minor naming improved * BlockId removal refactor: Backend::state_at * formatting --- bin/node/bench/src/import.rs | 9 +++- client/api/src/backend.rs | 4 +- client/api/src/in_mem.rs | 21 ++++---- .../basic-authorship/src/basic_authorship.rs | 2 +- client/block-builder/src/lib.rs | 5 +- client/db/benches/state_access.rs | 10 ++-- client/db/src/lib.rs | 48 ++++++------------ client/service/src/client/call_executor.rs | 21 ++++---- client/service/src/client/client.rs | 49 ++++++++++++------- client/service/test/src/client/mod.rs | 26 ++++++---- primitives/blockchain/src/backend.rs | 4 +- .../rpc/state-trie-migration-rpc/src/lib.rs | 6 +-- 12 files changed, 105 insertions(+), 100 deletions(-) diff --git a/bin/node/bench/src/import.rs b/bin/node/bench/src/import.rs index 47f630eb68700..26f9391800ceb 100644 --- a/bin/node/bench/src/import.rs +++ b/bin/node/bench/src/import.rs @@ -34,7 +34,7 @@ use std::borrow::Cow; use node_primitives::Block; use node_testing::bench::{BenchDb, BlockType, DatabaseType, KeyTypes, Profile}; -use sc_client_api::backend::Backend; +use sc_client_api::{backend::Backend, HeaderBackend}; use sp_runtime::generic::BlockId; use sp_state_machine::InspectState; @@ -127,10 +127,15 @@ impl core::Benchmark for ImportBenchmark { context.import_block(self.block.clone()); let elapsed = start.elapsed(); + let hash = context + .client + .expect_block_hash_from_id(&BlockId::number(1)) + .expect("Block 1 was imported; qed"); + // Sanity checks. context .client - .state_at(&BlockId::number(1)) + .state_at(&hash) .expect("state_at failed for block#1") .inspect_state(|| { match self.block_type { diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index bcc7a9bff3b2d..0e94d28b75dd5 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -505,11 +505,11 @@ pub trait Backend: AuxStore + Send + Sync { /// Returns true if state for given block is available. fn have_state_at(&self, hash: &Block::Hash, _number: NumberFor) -> bool { - self.state_at(BlockId::Hash(*hash)).is_ok() + self.state_at(hash).is_ok() } /// Returns state backend with post-state of given block. - fn state_at(&self, block: BlockId) -> sp_blockchain::Result; + fn state_at(&self, hash: &Block::Hash) -> sp_blockchain::Result; /// Attempts to revert the chain by `n` blocks. If `revert_finalized` is set it will attempt to /// revert past any finalized block, this is unsafe and can potentially leave the node in an diff --git a/client/api/src/in_mem.rs b/client/api/src/in_mem.rs index 9000f62aa6cc3..9cea1883bdcdc 100644 --- a/client/api/src/in_mem.rs +++ b/client/api/src/in_mem.rs @@ -686,7 +686,7 @@ where type OffchainStorage = OffchainStorage; fn begin_operation(&self) -> sp_blockchain::Result { - let old_state = self.state_at(BlockId::Hash(Default::default()))?; + let old_state = self.state_at(&Default::default())?; Ok(BlockImportOperation { pending_block: None, old_state, @@ -702,7 +702,8 @@ where operation: &mut Self::BlockImportOperation, block: BlockId, ) -> sp_blockchain::Result<()> { - operation.old_state = self.state_at(block)?; + let hash = self.blockchain.expect_block_hash_from_id(&block)?; + operation.old_state = self.state_at(&hash)?; Ok(()) } @@ -768,16 +769,16 @@ where None } - fn state_at(&self, block: BlockId) -> sp_blockchain::Result { - match block { - BlockId::Hash(h) if h == Default::default() => return Ok(Self::State::default()), - _ => {}, + fn state_at(&self, hash: &Block::Hash) -> sp_blockchain::Result { + if *hash == Default::default() { + return Ok(Self::State::default()) } - self.blockchain - .id(block) - .and_then(|id| self.states.read().get(&id).cloned()) - .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", block))) + self.states + .read() + .get(hash) + .cloned() + .ok_or_else(|| sp_blockchain::Error::UnknownBlock(format!("{}", hash))) } fn revert( diff --git a/client/basic-authorship/src/basic_authorship.rs b/client/basic-authorship/src/basic_authorship.rs index f5ccd9023a3db..da98ccab9cb07 100644 --- a/client/basic-authorship/src/basic_authorship.rs +++ b/client/basic-authorship/src/basic_authorship.rs @@ -736,7 +736,7 @@ mod tests { let api = client.runtime_api(); api.execute_block(&block_id, proposal.block).unwrap(); - let state = backend.state_at(block_id).unwrap(); + let state = backend.state_at(&genesis_hash).unwrap(); let storage_changes = api.into_storage_changes(&state, genesis_hash).unwrap(); diff --git a/client/block-builder/src/lib.rs b/client/block-builder/src/lib.rs index 803e9c1e8bf26..cd5e62e264200 100644 --- a/client/block-builder/src/lib.rs +++ b/client/block-builder/src/lib.rs @@ -258,12 +258,11 @@ where let proof = self.api.extract_proof(); - let state = self.backend.state_at(self.block_id)?; - let parent_hash = self.parent_hash; + let state = self.backend.state_at(&self.parent_hash)?; let storage_changes = self .api - .into_storage_changes(&state, parent_hash) + .into_storage_changes(&state, self.parent_hash) .map_err(sp_blockchain::Error::StorageChanges)?; Ok(BuiltBlock { diff --git a/client/db/benches/state_access.rs b/client/db/benches/state_access.rs index 4f4c10bcc8f53..912a9b028f638 100644 --- a/client/db/benches/state_access.rs +++ b/client/db/benches/state_access.rs @@ -84,7 +84,7 @@ fn insert_blocks(db: &Backend, storage: Vec<(Vec, Vec)>) -> H256 .map(|(k, v)| (k.clone(), Some(v.clone()))) .collect::>(); - let (state_root, tx) = db.state_at(BlockId::Hash(parent_hash)).unwrap().storage_root( + let (state_root, tx) = db.state_at(&parent_hash).unwrap().storage_root( changes.iter().map(|(k, v)| (k.as_slice(), v.as_deref())), StateVersion::V1, ); @@ -176,7 +176,7 @@ fn state_access_benchmarks(c: &mut Criterion) { group.bench_function(desc, |b| { b.iter_batched( - || backend.state_at(BlockId::Hash(block_hash)).expect("Creates state"), + || backend.state_at(&block_hash).expect("Creates state"), |state| { for key in keys.iter().cycle().take(keys.len() * multiplier) { let _ = state.storage(&key).expect("Doesn't fail").unwrap(); @@ -214,7 +214,7 @@ fn state_access_benchmarks(c: &mut Criterion) { group.bench_function(desc, |b| { b.iter_batched( - || backend.state_at(BlockId::Hash(block_hash)).expect("Creates state"), + || backend.state_at(&block_hash).expect("Creates state"), |state| { for key in keys.iter().take(1).cycle().take(multiplier) { let _ = state.storage(&key).expect("Doesn't fail").unwrap(); @@ -252,7 +252,7 @@ fn state_access_benchmarks(c: &mut Criterion) { group.bench_function(desc, |b| { b.iter_batched( - || backend.state_at(BlockId::Hash(block_hash)).expect("Creates state"), + || backend.state_at(&block_hash).expect("Creates state"), |state| { for key in keys.iter().take(1).cycle().take(multiplier) { let _ = state.storage_hash(&key).expect("Doesn't fail").unwrap(); @@ -290,7 +290,7 @@ fn state_access_benchmarks(c: &mut Criterion) { group.bench_function(desc, |b| { b.iter_batched( - || backend.state_at(BlockId::Hash(block_hash)).expect("Creates state"), + || backend.state_at(&block_hash).expect("Creates state"), |state| { let _ = state .storage_hash(sp_core::storage::well_known_keys::CODE) diff --git a/client/db/src/lib.rs b/client/db/src/lib.rs index 32c4c9ef85ed9..ae777ad154f6d 100644 --- a/client/db/src/lib.rs +++ b/client/db/src/lib.rs @@ -1963,10 +1963,11 @@ impl sc_client_api::backend::Backend for Backend { operation: &mut Self::BlockImportOperation, block: BlockId, ) -> ClientResult<()> { + let hash = self.blockchain.expect_block_hash_from_id(&block)?; if block.is_pre_genesis() { operation.old_state = self.empty_state()?; } else { - operation.old_state = self.state_at(block)?; + operation.old_state = self.state_at(&hash)?; } operation.commit_state = true; @@ -2302,15 +2303,8 @@ impl sc_client_api::backend::Backend for Backend { &self.blockchain } - fn state_at(&self, block: BlockId) -> ClientResult { - use sc_client_api::blockchain::HeaderBackend as BcHeaderBackend; - - let is_genesis = match &block { - BlockId::Number(n) if n.is_zero() => true, - BlockId::Hash(h) if h == &self.blockchain.meta.read().genesis_hash => true, - _ => false, - }; - if is_genesis { + fn state_at(&self, hash: &Block::Hash) -> ClientResult { + if hash == &self.blockchain.meta.read().genesis_hash { if let Some(genesis_state) = &*self.genesis_state.read() { let root = genesis_state.root; let db_state = DbStateBuilder::::new(genesis_state.clone(), root) @@ -2322,14 +2316,7 @@ impl sc_client_api::backend::Backend for Backend { } } - let hash = match block { - BlockId::Hash(h) => h, - BlockId::Number(n) => self.blockchain.hash(n)?.ok_or_else(|| { - sp_blockchain::Error::UnknownBlock(format!("Unknown block number {}", n)) - })?, - }; - - match self.blockchain.header_metadata(hash) { + match self.blockchain.header_metadata(*hash) { Ok(ref hdr) => { let hint = || { sc_state_db::NodeDb::get(self.storage.as_ref(), hdr.state_root.as_ref()) @@ -2337,7 +2324,7 @@ impl sc_client_api::backend::Backend for Backend { .is_some() }; if let Ok(()) = - self.storage.state_db.pin(&hash, hdr.number.saturated_into::(), hint) + self.storage.state_db.pin(hash, hdr.number.saturated_into::(), hint) { let root = hdr.state_root; let db_state = DbStateBuilder::::new(self.storage.clone(), root) @@ -2345,12 +2332,12 @@ impl sc_client_api::backend::Backend for Backend { self.shared_trie_cache.as_ref().map(|c| c.local_cache()), ) .build(); - let state = RefTrackingState::new(db_state, self.storage.clone(), Some(hash)); - Ok(RecordStatsState::new(state, Some(hash), self.state_usage.clone())) + let state = RefTrackingState::new(db_state, self.storage.clone(), Some(*hash)); + Ok(RecordStatsState::new(state, Some(*hash), self.state_usage.clone())) } else { Err(sp_blockchain::Error::UnknownBlock(format!( "State already discarded for {:?}", - block + hash ))) } }, @@ -2588,7 +2575,7 @@ pub(crate) mod tests { db.commit_operation(op).unwrap(); - let state = db.state_at(BlockId::Number(0)).unwrap(); + let state = db.state_at(&hash).unwrap(); assert_eq!(state.storage(&[1, 3, 5]).unwrap(), Some(vec![2, 4, 6])); assert_eq!(state.storage(&[1, 2, 3]).unwrap(), Some(vec![9, 9, 9])); @@ -2623,7 +2610,8 @@ pub(crate) mod tests { db.commit_operation(op).unwrap(); - let state = db.state_at(BlockId::Number(1)).unwrap(); + let hash = db.blockchain().expect_block_hash_from_id(&BlockId::Number(1)).unwrap(); + let state = db.state_at(&hash).unwrap(); assert_eq!(state.storage(&[1, 3, 5]).unwrap(), None); assert_eq!(state.storage(&[1, 2, 3]).unwrap(), Some(vec![9, 9, 9])); @@ -3139,11 +3127,7 @@ pub(crate) mod tests { hash }; - let block0_hash = backend - .state_at(BlockId::Hash(hash0)) - .unwrap() - .storage_hash(&b"test"[..]) - .unwrap(); + let block0_hash = backend.state_at(&hash0).unwrap().storage_hash(&b"test"[..]).unwrap(); let hash1 = { let mut op = backend.begin_operation().unwrap(); @@ -3182,11 +3166,7 @@ pub(crate) mod tests { backend.commit_operation(op).unwrap(); } - let block1_hash = backend - .state_at(BlockId::Hash(hash1)) - .unwrap() - .storage_hash(&b"test"[..]) - .unwrap(); + let block1_hash = backend.state_at(&hash1).unwrap().storage_hash(&b"test"[..]).unwrap(); assert_ne!(block0_hash, block1_hash); } diff --git a/client/service/src/client/call_executor.rs b/client/service/src/client/call_executor.rs index e39436ec641d7..8ab332a24be78 100644 --- a/client/service/src/client/call_executor.rs +++ b/client/service/src/client/call_executor.rs @@ -147,17 +147,14 @@ where extensions: Option, ) -> sp_blockchain::Result> { let mut changes = OverlayedChanges::default(); - let state = self.backend.state_at(*at)?; + let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?; + let state = self.backend.state_at(&at_hash)?; let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); let runtime_code = state_runtime_code.runtime_code().map_err(sp_blockchain::Error::RuntimeCode)?; let runtime_code = self.check_override(runtime_code, at)?; - let at_hash = self.backend.blockchain().block_hash_from_id(at)?.ok_or_else(|| { - sp_blockchain::Error::UnknownBlock(format!("Could not find block hash for {:?}", at)) - })?; - let mut sm = StateMachine::new( &state, &mut changes, @@ -195,14 +192,11 @@ where { let mut storage_transaction_cache = storage_transaction_cache.map(|c| c.borrow_mut()); - let state = self.backend.state_at(*at)?; + let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?; + let state = self.backend.state_at(&at_hash)?; let changes = &mut *changes.borrow_mut(); - let at_hash = self.backend.blockchain().block_hash_from_id(at)?.ok_or_else(|| { - sp_blockchain::Error::UnknownBlock(format!("Could not find block hash for {:?}", at)) - })?; - // It is important to extract the runtime code here before we create the proof // recorder to not record it. We also need to fetch the runtime code from `state` to // make sure we use the caching layers. @@ -255,7 +249,9 @@ where fn runtime_version(&self, id: &BlockId) -> sp_blockchain::Result { let mut overlay = OverlayedChanges::default(); - let state = self.backend.state_at(*id)?; + + let at_hash = self.backend.blockchain().expect_block_hash_from_id(id)?; + let state = self.backend.state_at(&at_hash)?; let mut cache = StorageTransactionCache::::default(); let mut ext = Ext::new(&mut overlay, &mut cache, &state, None); let state_runtime_code = sp_state_machine::backend::BackendRuntimeCode::new(&state); @@ -272,7 +268,8 @@ where method: &str, call_data: &[u8], ) -> sp_blockchain::Result<(Vec, StorageProof)> { - let state = self.backend.state_at(*at)?; + let at_hash = self.backend.blockchain().expect_block_hash_from_id(at)?; + let state = self.backend.state_at(&at_hash)?; let trie_backend = state.as_trie_backend(); diff --git a/client/service/src/client/client.rs b/client/service/src/client/client.rs index 27561046c3481..e2fd5cda1d2f0 100644 --- a/client/service/src/client/client.rs +++ b/client/service/src/client/client.rs @@ -414,8 +414,8 @@ where } /// Get a reference to the state at a given block. - pub fn state_at(&self, block: &BlockId) -> sp_blockchain::Result { - self.backend.state_at(*block) + pub fn state_at(&self, hash: &Block::Hash) -> sp_blockchain::Result { + self.backend.state_at(hash) } /// Get the code at a given block. @@ -813,7 +813,7 @@ where Block::new(import_block.header.clone(), body.clone()), )?; - let state = self.backend.state_at(at)?; + let state = self.backend.state_at(parent_hash)?; let gen_storage_changes = runtime_api .into_storage_changes(&state, *parent_hash) .map_err(sp_blockchain::Error::Storage)?; @@ -1154,7 +1154,9 @@ where id: &BlockId, keys: &mut dyn Iterator, ) -> sp_blockchain::Result { - self.state_at(id).and_then(|state| prove_read(state, keys).map_err(Into::into)) + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + self.state_at(&hash) + .and_then(|state| prove_read(state, keys).map_err(Into::into)) } fn read_child_proof( @@ -1163,7 +1165,8 @@ where child_info: &ChildInfo, keys: &mut dyn Iterator, ) -> sp_blockchain::Result { - self.state_at(id) + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + self.state_at(&hash) .and_then(|state| prove_child_read(state, child_info, keys).map_err(Into::into)) } @@ -1182,7 +1185,8 @@ where start_key: &[Vec], size_limit: usize, ) -> sp_blockchain::Result<(CompactProof, u32)> { - let state = self.state_at(id)?; + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let state = self.state_at(&hash)?; // this is a read proof, using version V0 or V1 is equivalent. let root = state.storage_root(std::iter::empty(), StateVersion::V0).0; @@ -1204,7 +1208,8 @@ where if start_key.len() > MAX_NESTED_TRIE_DEPTH { return Err(Error::Backend("Invalid start key.".to_string())) } - let state = self.state_at(id)?; + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let state = self.state_at(&hash)?; let child_info = |storage_key: &Vec| -> sp_blockchain::Result { let storage_key = PrefixedStorageKey::new_ref(storage_key); match ChildType::from_prefixed_key(storage_key) { @@ -1400,7 +1405,8 @@ where id: &BlockId, key_prefix: &StorageKey, ) -> sp_blockchain::Result> { - let keys = self.state_at(id)?.keys(&key_prefix.0).into_iter().map(StorageKey).collect(); + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let keys = self.state_at(&hash)?.keys(&key_prefix.0).into_iter().map(StorageKey).collect(); Ok(keys) } @@ -1409,7 +1415,8 @@ where id: &BlockId, key_prefix: &StorageKey, ) -> sp_blockchain::Result> { - let state = self.state_at(id)?; + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let state = self.state_at(&hash)?; let keys = state .keys(&key_prefix.0) .into_iter() @@ -1427,7 +1434,8 @@ where prefix: Option<&'a StorageKey>, start_key: Option<&StorageKey>, ) -> sp_blockchain::Result> { - let state = self.state_at(id)?; + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let state = self.state_at(&hash)?; let start_key = start_key.or(prefix).map(|key| key.0.clone()).unwrap_or_else(Vec::new); Ok(KeyIterator::new(state, prefix, start_key)) } @@ -1439,7 +1447,8 @@ where prefix: Option<&'a StorageKey>, start_key: Option<&StorageKey>, ) -> sp_blockchain::Result> { - let state = self.state_at(id)?; + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + let state = self.state_at(&hash)?; let start_key = start_key.or(prefix).map(|key| key.0.clone()).unwrap_or_else(Vec::new); Ok(KeyIterator::new_child(state, child_info, prefix, start_key)) } @@ -1449,8 +1458,9 @@ where id: &BlockId, key: &StorageKey, ) -> sp_blockchain::Result> { + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; Ok(self - .state_at(id)? + .state_at(&hash)? .storage(&key.0) .map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))? .map(StorageData)) @@ -1461,7 +1471,8 @@ where id: &BlockId, key: &StorageKey, ) -> sp_blockchain::Result> { - self.state_at(id)? + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + self.state_at(&hash)? .storage_hash(&key.0) .map_err(|e| sp_blockchain::Error::from_state(Box::new(e))) } @@ -1472,8 +1483,9 @@ where child_info: &ChildInfo, key_prefix: &StorageKey, ) -> sp_blockchain::Result> { + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; let keys = self - .state_at(id)? + .state_at(&hash)? .child_keys(child_info, &key_prefix.0) .into_iter() .map(StorageKey) @@ -1487,8 +1499,9 @@ where child_info: &ChildInfo, key: &StorageKey, ) -> sp_blockchain::Result> { + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; Ok(self - .state_at(id)? + .state_at(&hash)? .child_storage(child_info, &key.0) .map_err(|e| sp_blockchain::Error::from_state(Box::new(e)))? .map(StorageData)) @@ -1500,7 +1513,8 @@ where child_info: &ChildInfo, key: &StorageKey, ) -> sp_blockchain::Result> { - self.state_at(id)? + let hash = self.backend.blockchain().expect_block_hash_from_id(&id)?; + self.state_at(&hash)? .child_storage_hash(child_info, &key.0) .map_err(|e| sp_blockchain::Error::from_state(Box::new(e))) } @@ -1681,7 +1695,8 @@ where } fn state_at(&self, at: &BlockId) -> Result { - self.state_at(at).map_err(Into::into) + let hash = self.backend.blockchain().expect_block_hash_from_id(at)?; + self.state_at(&hash).map_err(Into::into) } } diff --git a/client/service/test/src/client/mod.rs b/client/service/test/src/client/mod.rs index e0f47110d9046..c6ac1fc7d73d9 100644 --- a/client/service/test/src/client/mod.rs +++ b/client/service/test/src/client/mod.rs @@ -20,7 +20,7 @@ use futures::executor::block_on; use parity_scale_codec::{Decode, Encode, Joiner}; use sc_block_builder::BlockBuilderProvider; use sc_client_api::{ - in_mem, BlockBackend, BlockchainEvents, FinalityNotifications, StorageProvider, + in_mem, BlockBackend, BlockchainEvents, FinalityNotifications, HeaderBackend, StorageProvider, }; use sc_client_db::{Backend, BlocksPruning, DatabaseSettings, DatabaseSource, PruningMode}; use sc_consensus::{ @@ -338,11 +338,15 @@ fn block_builder_works_with_transactions() { let block = builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, block)).unwrap(); + let hash0 = client + .expect_block_hash_from_id(&BlockId::Number(0)) + .expect("block 0 was just imported. qed"); + let hash1 = client + .expect_block_hash_from_id(&BlockId::Number(1)) + .expect("block 1 was just imported. qed"); + assert_eq!(client.chain_info().best_number, 1); - assert_ne!( - client.state_at(&BlockId::Number(1)).unwrap().pairs(), - client.state_at(&BlockId::Number(0)).unwrap().pairs() - ); + assert_ne!(client.state_at(&hash1).unwrap().pairs(), client.state_at(&hash0).unwrap().pairs()); assert_eq!( client .runtime_api() @@ -392,11 +396,15 @@ fn block_builder_does_not_include_invalid() { let block = builder.build().unwrap().block; block_on(client.import(BlockOrigin::Own, block)).unwrap(); + let hash0 = client + .expect_block_hash_from_id(&BlockId::Number(0)) + .expect("block 0 was just imported. qed"); + let hash1 = client + .expect_block_hash_from_id(&BlockId::Number(1)) + .expect("block 1 was just imported. qed"); + assert_eq!(client.chain_info().best_number, 1); - assert_ne!( - client.state_at(&BlockId::Number(1)).unwrap().pairs(), - client.state_at(&BlockId::Number(0)).unwrap().pairs() - ); + assert_ne!(client.state_at(&hash1).unwrap().pairs(), client.state_at(&hash0).unwrap().pairs()); assert_eq!(client.body(&BlockId::Number(1)).unwrap().unwrap().len(), 1) } diff --git a/primitives/blockchain/src/backend.rs b/primitives/blockchain/src/backend.rs index f80c6d0269116..79c05aec8adca 100644 --- a/primitives/blockchain/src/backend.rs +++ b/primitives/blockchain/src/backend.rs @@ -78,8 +78,8 @@ pub trait HeaderBackend: Send + Sync { /// Convert an arbitrary block ID into a block hash. Returns `UnknownBlock` error if block is /// not found. fn expect_block_hash_from_id(&self, id: &BlockId) -> Result { - self.block_hash_from_id(id).and_then(|n| { - n.ok_or_else(|| Error::UnknownBlock(format!("Expect block hash from id: {}", id))) + self.block_hash_from_id(id).and_then(|h| { + h.ok_or_else(|| Error::UnknownBlock(format!("Expect block hash from id: {}", id))) }) } } diff --git a/utils/frame/rpc/state-trie-migration-rpc/src/lib.rs b/utils/frame/rpc/state-trie-migration-rpc/src/lib.rs index f9a57206ece4d..c3d3ec816f97e 100644 --- a/utils/frame/rpc/state-trie-migration-rpc/src/lib.rs +++ b/utils/frame/rpc/state-trie-migration-rpc/src/lib.rs @@ -24,7 +24,7 @@ use jsonrpsee::{ }; use sc_rpc_api::DenyUnsafe; use serde::{Deserialize, Serialize}; -use sp_runtime::{generic::BlockId, traits::Block as BlockT}; +use sp_runtime::traits::Block as BlockT; use std::sync::Arc; use sp_core::{ @@ -144,8 +144,8 @@ where fn call(&self, at: Option<::Hash>) -> RpcResult { self.deny_unsafe.check_if_safe()?; - let block_id = BlockId::hash(at.unwrap_or_else(|| self.client.info().best_hash)); - let state = self.backend.state_at(block_id).map_err(error_into_rpc_err)?; + let hash = at.unwrap_or_else(|| self.client.info().best_hash); + let state = self.backend.state_at(&hash).map_err(error_into_rpc_err)?; let (top, child) = migration_status(&state).map_err(error_into_rpc_err)?; Ok(MigrationStatusResult { From 0ee03277c33b6334ddba7434e014fa637dcb6107 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 14 Oct 2022 13:26:13 +0200 Subject: [PATCH 7/7] Try-runtime CLI fix weight parsing (#12491) Signed-off-by: Oliver Tale-Yazdi Signed-off-by: Oliver Tale-Yazdi --- Cargo.lock | 1 + utils/frame/try-runtime/cli/Cargo.toml | 1 + .../try-runtime/cli/src/commands/follow_chain.rs | 5 +++-- .../cli/src/commands/on_runtime_upgrade.rs | 14 ++++++++------ 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 062195c70f8ab..032886b9945e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11356,6 +11356,7 @@ dependencies = [ "sp-runtime", "sp-state-machine", "sp-version", + "sp-weights", "tokio", "zstd", ] diff --git a/utils/frame/try-runtime/cli/Cargo.toml b/utils/frame/try-runtime/cli/Cargo.toml index 56ead30644d86..956eaff745335 100644 --- a/utils/frame/try-runtime/cli/Cargo.toml +++ b/utils/frame/try-runtime/cli/Cargo.toml @@ -31,6 +31,7 @@ sp-keystore = { version = "0.12.0", path = "../../../../primitives/keystore" } sp-runtime = { version = "6.0.0", path = "../../../../primitives/runtime" } sp-state-machine = { version = "0.12.0", path = "../../../../primitives/state-machine" } sp-version = { version = "5.0.0", path = "../../../../primitives/version" } +sp-weights = { version = "4.0.0", path = "../../../../primitives/weights" } frame-try-runtime = { path = "../../../../frame/try-runtime" } [dev-dependencies] diff --git a/utils/frame/try-runtime/cli/src/commands/follow_chain.rs b/utils/frame/try-runtime/cli/src/commands/follow_chain.rs index 88866b538169b..fb5345827858a 100644 --- a/utils/frame/try-runtime/cli/src/commands/follow_chain.rs +++ b/utils/frame/try-runtime/cli/src/commands/follow_chain.rs @@ -33,6 +33,7 @@ use sc_service::Configuration; use serde::de::DeserializeOwned; use sp_core::H256; use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; +use sp_weights::Weight; use std::{collections::VecDeque, fmt::Debug, str::FromStr}; const SUB: &str = "chain_subscribeFinalizedHeads"; @@ -294,8 +295,8 @@ where full_extensions(), )?; - let consumed_weight = ::decode(&mut &*encoded_result) - .map_err(|e| format!("failed to decode output: {:?}", e))?; + let consumed_weight = ::decode(&mut &*encoded_result) + .map_err(|e| format!("failed to decode weight: {:?}", e))?; let storage_changes = changes .drain_storage_changes( diff --git a/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs index 5055e4fb34581..1d7d876a4aa92 100644 --- a/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs +++ b/utils/frame/try-runtime/cli/src/commands/on_runtime_upgrade.rs @@ -21,6 +21,7 @@ use parity_scale_codec::Decode; use sc_executor::NativeExecutionDispatch; use sc_service::Configuration; use sp_runtime::traits::{Block as BlockT, NumberFor}; +use sp_weights::Weight; use crate::{ build_executor, ensure_matching_spec, extract_code, local_spec, state_machine_call_with_proof, @@ -78,14 +79,15 @@ where Default::default(), // we don't really need any extensions here. )?; - let (weight, total_weight) = <(u64, u64) as Decode>::decode(&mut &*encoded_result) - .map_err(|e| format!("failed to decode output: {:?}", e))?; + let (weight, total_weight) = <(Weight, Weight) as Decode>::decode(&mut &*encoded_result) + .map_err(|e| format!("failed to decode weight: {:?}", e))?; log::info!( target: LOG_TARGET, - "TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = {}, total weight = {} ({})", - weight, - total_weight, - weight as f64 / total_weight.max(1) as f64 + "TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = ({} ps, {} byte), total weight = ({} ps, {} byte) ({:.2} %, {:.2} %).", + weight.ref_time(), weight.proof_size(), + total_weight.ref_time(), total_weight.proof_size(), + (weight.ref_time() as f64 / total_weight.ref_time().max(1) as f64) * 100.0, + (weight.proof_size() as f64 / total_weight.proof_size().max(1) as f64) * 100.0, ); Ok(())