From 9068a663fb568374468bbda67f8b016215db04d0 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Tue, 24 Mar 2020 21:06:10 +0100 Subject: [PATCH 1/4] client/finality-grandpa/src/until_imported: Refactor BlockGlobalMessage `BlockGlobalMessage` owns an `inner` which contains (1) a count for the amount of outstanding blocks to be waited on and (2) the message itself. Given that both is already wrapped in an `Arc` there is no need to keep track of the outstanding blocks, given that it simply corresponds to the amount of strong reference counts on the `Arc` itself. This commit removes the atomic counter within `inner` and piggy backs on the `Arc` reference counter instead. --- client/finality-grandpa/src/until_imported.rs | 104 ++++++++++++++---- 1 file changed, 80 insertions(+), 24 deletions(-) diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index c3a52fcf56fe1..f4f8eb848b2bd 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -40,7 +40,7 @@ use sp_runtime::traits::{Block as BlockT, Header as HeaderT, NumberFor}; use std::collections::{HashMap, VecDeque}; use std::pin::Pin; -use std::sync::{atomic::{AtomicUsize, Ordering}, Arc}; +use std::sync::Arc; use std::task::{Context, Poll}; use std::time::{Duration, Instant}; use sp_finality_grandpa::AuthorityId; @@ -308,7 +308,7 @@ pub(crate) type UntilVoteTargetImported { - inner: Arc<(AtomicUsize, Mutex>>)>, + inner: Arc>>>, target_number: NumberFor, } @@ -416,7 +416,7 @@ impl BlockUntilImported for BlockGlobalMessage { return Ok(()) } - let locked_global = Arc::new((AtomicUsize::new(unknown_count), Mutex::new(Some(input)))); + let locked_global = Arc::new(Mutex::new(Some(input))); // schedule waits for all unknown messages. // when the last one of these has `wait_completed` called on it, @@ -438,30 +438,23 @@ impl BlockUntilImported for BlockGlobalMessage { fn wait_completed(self, canon_number: NumberFor) -> Option { if self.target_number != canon_number { - // if we return without deducting the counter, then none of the other - // handles can return the commit message. + // Delete the inner message so it won't ever be forwarded. Future calls to + // `wait_completed` on the same `inner` will ignore it. + *self.inner.lock() = None; return None; } - let mut last_count = self.inner.0.load(Ordering::Acquire); - - // CAS loop to ensure that we always have a last reader. - loop { - if last_count == 1 { // we are the last one left. - return self.inner.1.lock().take(); - } - - let prev_value = self.inner.0.compare_and_swap( - last_count, - last_count - 1, - Ordering::SeqCst, - ); - - if prev_value == last_count { - return None; - } else { - last_count = prev_value; - } + match Arc::try_unwrap(self.inner) { + // This is the last reference, thus this was the last outstanding block to be awaited. + Ok(inner) => match Mutex::into_inner(inner) { + msg @ Some(_) => msg, + // A past call to `wait_completed` deemed this message as faulty due to a block + // number mismatch. + None => None, + }, + // There are still other strong references to this `Arc`, thus the message is blocked on + // other blocks to be imported. + Err(_self) => None, } } } @@ -941,4 +934,67 @@ mod tests { futures::executor::block_on(test); } + + fn test_catch_up() -> Arc>>> { + let header = make_header(5); + + let unknown_catch_up = finality_grandpa::CatchUp { + round_number: 1, + precommits: vec![], + prevotes: vec![], + base_hash: header.hash(), + base_number: *header.number(), + }; + + let catch_up = voter::CommunicationIn::CatchUp( + unknown_catch_up.clone(), + voter::Callback::Blank, + ); + + Arc::new(Mutex::new(Some(catch_up))) + } + + #[test] + fn block_global_message_wait_completed_return_when_all_awaited() { + let msg_inner = test_catch_up(); + + let waiting_block_1 = BlockGlobalMessage:: { + inner: msg_inner.clone(), + target_number: 1, + }; + + let waiting_block_2 = BlockGlobalMessage:: { + inner: msg_inner, + target_number: 2, + }; + + // waiting_block_2 is still waiting for block 2, thus this should return `None`. + assert!(waiting_block_1.wait_completed(1).is_none()); + + // Message only depended on block 1 and 2. Both have been imported, thus this should yield + // the message. + assert!(waiting_block_2.wait_completed(2).is_some()); + } + + #[test] + fn block_global_message_wait_completed_return_none_on_block_number_missmatch() { + let msg_inner = test_catch_up(); + + let waiting_block_1 = BlockGlobalMessage:: { + inner: msg_inner.clone(), + target_number: 1, + }; + + let waiting_block_2 = BlockGlobalMessage:: { + inner: msg_inner, + target_number: 2, + }; + + // Calling wait_completed with wrong block number should yield None. + assert!(waiting_block_1.wait_completed(1234).is_none()); + + // All blocks, that the message depended on, have been imported. Still, given the above + // block number mismatch this should return None. + assert!(waiting_block_2.wait_completed(2).is_none()); + } } From 498cea03743ed30a49d235f424882f8caca9e32d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 25 Mar 2020 15:50:18 +0100 Subject: [PATCH 2/4] client/finality-grandpa/src/until_imported: Remove useless match --- client/finality-grandpa/src/until_imported.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index f4f8eb848b2bd..b58c358a71ad5 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -445,13 +445,10 @@ impl BlockUntilImported for BlockGlobalMessage { } match Arc::try_unwrap(self.inner) { - // This is the last reference, thus this was the last outstanding block to be awaited. - Ok(inner) => match Mutex::into_inner(inner) { - msg @ Some(_) => msg, - // A past call to `wait_completed` deemed this message as faulty due to a block - // number mismatch. - None => None, - }, + // This is the last reference and thus the last outstanding block to be awaited. `inner` + // is either `Some(_)` or `None`. The latter implies that a previous `wait_completed` + // call witnessed a block number mismatch (see above). + Ok(inner) => Mutex::into_inner(inner), // There are still other strong references to this `Arc`, thus the message is blocked on // other blocks to be imported. Err(_self) => None, From 33e341005bab76f5d86d4daec44952f77330ea09 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 25 Mar 2020 18:10:26 +0100 Subject: [PATCH 3/4] client/finality-grandpa/src/until_imported.rs: Remove unused var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: André Silva --- client/finality-grandpa/src/until_imported.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index b58c358a71ad5..616178301b453 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -451,7 +451,7 @@ impl BlockUntilImported for BlockGlobalMessage { Ok(inner) => Mutex::into_inner(inner), // There are still other strong references to this `Arc`, thus the message is blocked on // other blocks to be imported. - Err(_self) => None, + Err(_) => None, } } } From d6336bce3e3539a3ea6610c6646f61d2fae2ca7c Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 25 Mar 2020 18:12:27 +0100 Subject: [PATCH 4/4] client/finality-grandpa/src/until_imported: Address comment suggestion --- client/finality-grandpa/src/until_imported.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/client/finality-grandpa/src/until_imported.rs b/client/finality-grandpa/src/until_imported.rs index 616178301b453..c3804e1272ffa 100644 --- a/client/finality-grandpa/src/until_imported.rs +++ b/client/finality-grandpa/src/until_imported.rs @@ -307,6 +307,10 @@ pub(crate) type UntilVoteTargetImported { inner: Arc>>>, target_number: NumberFor,