From e3de79f774f9ecf9b604195b293b08202d0f53c0 Mon Sep 17 00:00:00 2001 From: Svyatoslav Nikolsky Date: Fri, 15 Jan 2021 18:52:31 +0300 Subject: [PATCH] Improve Substrate pallet logs (#650) --- bridges/modules/substrate/src/fork_tests.rs | 8 ++++-- bridges/modules/substrate/src/lib.rs | 21 ++++++++++++---- bridges/modules/substrate/src/verifier.rs | 28 +++++++++++---------- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/bridges/modules/substrate/src/fork_tests.rs b/bridges/modules/substrate/src/fork_tests.rs index f90d2c0231ca6..c71ed3938e1bb 100644 --- a/bridges/modules/substrate/src/fork_tests.rs +++ b/bridges/modules/substrate/src/fork_tests.rs @@ -397,7 +397,9 @@ where } // Try and import into storage - let res = verifier.import_header(header.clone()).map_err(TestError::Import); + let res = verifier + .import_header(header.hash(), header.clone()) + .map_err(TestError::Import); assert_eq!( res, *expected_result, "Expected {:?} while importing header ({}, {}), got {:?}", @@ -427,7 +429,9 @@ where header.digest = change_log(*delay); } - let res = verifier.import_header(header.clone()).map_err(TestError::Import); + let res = verifier + .import_header(header.hash(), header.clone()) + .map_err(TestError::Import); assert_eq!( res, *expected_result, "Expected {:?} while importing header ({}, {}), got {:?}", diff --git a/bridges/modules/substrate/src/lib.rs b/bridges/modules/substrate/src/lib.rs index fa957c384fb05..fbd6cab95734e 100644 --- a/bridges/modules/substrate/src/lib.rs +++ b/bridges/modules/substrate/src/lib.rs @@ -172,15 +172,21 @@ decl_module! { ) -> DispatchResult { ensure_operational::()?; let _ = ensure_signed(origin)?; - frame_support::debug::trace!("Got header {:?}", header); + let hash = header.hash(); + frame_support::debug::trace!("Going to import header {:?}: {:?}", hash, header); let mut verifier = verifier::Verifier { storage: PalletStorage::::new(), }; let _ = verifier - .import_header(header) - .map_err(|_| >::InvalidHeader)?; + .import_header(hash, header) + .map_err(|e| { + frame_support::debug::error!("Failed to import header {:?}: {:?}", hash, e); + >::InvalidHeader + })?; + + frame_support::debug::trace!("Successfully imported header: {:?}", hash); Ok(()) } @@ -199,7 +205,7 @@ decl_module! { ) -> DispatchResult { ensure_operational::()?; let _ = ensure_signed(origin)?; - frame_support::debug::trace!("Got header hash {:?}", hash); + frame_support::debug::trace!("Going to finalize header: {:?}", hash); let mut verifier = verifier::Verifier { storage: PalletStorage::::new(), @@ -207,7 +213,12 @@ decl_module! { let _ = verifier .import_finality_proof(hash, finality_proof.into()) - .map_err(|_| >::UnfinalizedHeader)?; + .map_err(|e| { + frame_support::debug::error!("Failed to finalize header {:?}: {:?}", hash, e); + >::UnfinalizedHeader + })?; + + frame_support::debug::trace!("Successfully finalized header: {:?}", hash); Ok(()) } diff --git a/bridges/modules/substrate/src/verifier.rs b/bridges/modules/substrate/src/verifier.rs index 913129f19c6ef..41139cfebeb0c 100644 --- a/bridges/modules/substrate/src/verifier.rs +++ b/bridges/modules/substrate/src/verifier.rs @@ -112,8 +112,7 @@ where /// /// Will perform some basic checks to make sure that this header doesn't break any assumptions /// such as being on a different finalized fork. - pub fn import_header(&mut self, header: H) -> Result<(), ImportError> { - let hash = header.hash(); + pub fn import_header(&mut self, hash: H::Hash, header: H) -> Result<(), ImportError> { let best_finalized = self.storage.best_finalized_header(); if header.number() <= best_finalized.number() { @@ -424,7 +423,7 @@ mod tests { let header = test_header(1); let mut verifier = Verifier { storage }; - assert_err!(verifier.import_header(header), ImportError::OldHeader); + assert_err!(verifier.import_header(header.hash(), header), ImportError::OldHeader); }) } @@ -440,7 +439,10 @@ mod tests { let header = TestHeader::new_from_number(2); let mut verifier = Verifier { storage }; - assert_err!(verifier.import_header(header), ImportError::MissingParent); + assert_err!( + verifier.import_header(header.hash(), header), + ImportError::MissingParent + ); }) } @@ -460,7 +462,7 @@ mod tests { >::insert(header.hash(), &imported_header); let mut verifier = Verifier { storage }; - assert_err!(verifier.import_header(header), ImportError::OldHeader); + assert_err!(verifier.import_header(header.hash(), header), ImportError::OldHeader); }) } @@ -484,7 +486,7 @@ mod tests { let mut verifier = Verifier { storage: storage.clone(), }; - assert_ok!(verifier.import_header(header.clone())); + assert_ok!(verifier.import_header(header.hash(), header.clone())); let stored_header = storage .header_by_hash(header.hash()) @@ -514,8 +516,8 @@ mod tests { }; // It should be fine to import both - assert_ok!(verifier.import_header(header_on_fork1.clone())); - assert_ok!(verifier.import_header(header_on_fork2.clone())); + assert_ok!(verifier.import_header(header_on_fork1.hash(), header_on_fork1.clone())); + assert_ok!(verifier.import_header(header_on_fork2.hash(), header_on_fork2.clone())); // We should have two headers marked as being the best since they're // both at the same height @@ -559,7 +561,7 @@ mod tests { let mut verifier = Verifier { storage: storage.clone(), }; - assert_ok!(verifier.import_header(better_header.clone())); + assert_ok!(verifier.import_header(better_header.hash(), better_header.clone())); // Since `better_header` is the only one at height = 2 we should only have // a single "best header" now. @@ -668,7 +670,7 @@ mod tests { let mut verifier = Verifier { storage }; assert_eq!( - verifier.import_header(header).unwrap_err(), + verifier.import_header(header.hash(), header).unwrap_err(), ImportError::InvalidAuthoritySet ); }) @@ -697,7 +699,7 @@ mod tests { storage: storage.clone(), }; - assert_ok!(verifier.import_header(header.clone())); + assert_ok!(verifier.import_header(header.hash(), header.clone())); assert_ok!(verifier.import_finality_proof(header.hash(), justification.into())); assert_eq!(storage.best_finalized_header().header, header); }) @@ -724,7 +726,7 @@ mod tests { let mut verifier = Verifier { storage: storage.clone(), }; - assert!(verifier.import_header(header.clone()).is_ok()); + assert!(verifier.import_header(header.hash(), header.clone()).is_ok()); assert!(verifier .import_finality_proof(header.hash(), justification.into()) .is_ok()); @@ -776,7 +778,7 @@ mod tests { storage: storage.clone(), }; - assert_ok!(verifier.import_header(header.clone())); + assert_ok!(verifier.import_header(header.hash(), header.clone())); assert_eq!(storage.missing_justifications().len(), 1); assert_eq!(storage.missing_justifications()[0].hash, header.hash());