Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions bridges/modules/grandpa/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ workspace = true

[dependencies]
codec = { workspace = true }
log = { workspace = true }
scale-info = { features = ["derive"], workspace = true }
tracing = { workspace = true }

# Bridge Dependencies
bp-header-chain = { workspace = true }
Expand Down Expand Up @@ -47,11 +47,11 @@ std = [
"frame-benchmarking/std",
"frame-support/std",
"frame-system/std",
"log/std",
"scale-info/std",
"sp-consensus-grandpa/std",
"sp-runtime/std",
"sp-std/std",
"tracing/std",
]
runtime-benchmarks = [
"bp-test-utils",
Expand Down
30 changes: 14 additions & 16 deletions bridges/modules/grandpa/src/call_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {

// else - if we can not accept more free headers, "reject" the transaction
if !Self::has_free_header_slots() {
log::trace!(
tracing::trace!(
target: crate::LOG_TARGET,
"Cannot accept free {:?} header {:?}. No more free slots remaining",
Copy link
Contributor

Choose a reason for hiding this comment

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

do you leave these {:?} header {:?} on purpose here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

T::BridgedChain::ID,
Expand All @@ -99,14 +99,12 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
if !call_info.is_mandatory {
if let Some(free_headers_interval) = T::FreeHeadersInterval::get() {
if improved_by < free_headers_interval.into() {
log::trace!(
tracing::trace!(
target: crate::LOG_TARGET,
"Cannot accept free {:?} header {:?}. Too small difference \
between submitted headers: {:?} vs {}",
between submitted headers: {improved_by:?} vs {free_headers_interval}",
Copy link
Contributor

Choose a reason for hiding this comment

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

@raymondkfcheung is this tracing syntax? Shouldnt' improved_by, free_headers_interval be provided as separate params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

T::BridgedChain::ID,
call_info.block_number,
improved_by,
free_headers_interval,
);

return Err(Error::<T, I>::BelowFreeHeaderInterval);
Expand Down Expand Up @@ -137,22 +135,22 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
current_set_id: Option<SetId>,
) -> Result<BlockNumberOf<T::BridgedChain>, Error<T, I>> {
let best_finalized = BestFinalized::<T, I>::get().ok_or_else(|| {
log::trace!(
tracing::trace!(
target: crate::LOG_TARGET,
"Cannot finalize header {:?} because pallet is not yet initialized",
finality_target,
header=?finality_target,
"Cannot finalize header because pallet is not yet initialized"
);
<Error<T, I>>::NotInitialized
})?;

let improved_by = match finality_target.checked_sub(&best_finalized.number()) {
Some(improved_by) if improved_by > Zero::zero() => improved_by,
_ => {
log::trace!(
tracing::trace!(
target: crate::LOG_TARGET,
"Cannot finalize obsolete header: bundled {:?}, best {:?}",
finality_target,
best_finalized,
bundled=?finality_target,
best=?best_finalized,
"Cannot finalize obsolete header"
);

return Err(Error::<T, I>::OldHeader)
Expand All @@ -162,11 +160,11 @@ impl<T: Config<I>, I: 'static> SubmitFinalityProofHelper<T, I> {
if let Some(current_set_id) = current_set_id {
let actual_set_id = <CurrentAuthoritySet<T, I>>::get().set_id;
if current_set_id != actual_set_id {
log::trace!(
tracing::trace!(
target: crate::LOG_TARGET,
"Cannot finalize header signed by unknown authority set: bundled {:?}, best {:?}",
current_set_id,
actual_set_id,
bundled=?current_set_id,
best=?actual_set_id,
"Cannot finalize header signed by unknown authority set"
);

return Err(Error::<T, I>::InvalidAuthoritySetId)
Expand Down
43 changes: 21 additions & 22 deletions bridges/modules/grandpa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,10 @@ pub mod pallet {
ensure!(init_allowed, <Error<T, I>>::AlreadyInitialized);
initialize_bridge::<T, I>(init_data.clone())?;

log::info!(
tracing::info!(
target: LOG_TARGET,
"Pallet has been initialized with the following parameters: {:?}",
init_data
parameters=?init_data,
"Pallet has been initialized"
);

Ok(().into())
Expand Down Expand Up @@ -292,10 +292,10 @@ pub mod pallet {
ensure_signed(origin)?;

let (hash, number) = (finality_target.hash(), *finality_target.number());
log::trace!(
tracing::trace!(
target: LOG_TARGET,
"Going to try and finalize header {:?}",
finality_target
header=?finality_target,
"Going to try and finalize header"
);

// it checks whether the `number` is better than the current best block number
Expand Down Expand Up @@ -332,10 +332,10 @@ pub mod pallet {
// to pay for the transaction.
let pays_fee = if may_refund_call_fee { Pays::No } else { Pays::Yes };

log::info!(
tracing::info!(
target: LOG_TARGET,
"Successfully imported finalized header with hash {:?}! Free: {}",
hash,
?hash,
"Successfully imported finalized header! Free: {}",
if may_refund_call_fee { "Yes" } else { "No" },
Copy link
Contributor

Choose a reason for hiding this comment

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

there is pays_fee above already maybe worth to change if may_refund_call_fee { "Yes" } else { "No" } to just print ?pays_fee ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

);

Expand Down Expand Up @@ -656,12 +656,12 @@ pub mod pallet {

<CurrentAuthoritySet<T, I>>::put(&next_authorities);

log::info!(
tracing::info!(
target: LOG_TARGET,
"Transitioned from authority set {} to {}! New authorities are: {:?}",
old_current_set_id,
new_current_set_id,
next_authorities,
%old_current_set_id,
%new_current_set_id,
?next_authorities,
"Transitioned from authority set!"
);

Ok(Some(next_authorities.into()))
Expand All @@ -687,11 +687,11 @@ pub mod pallet {
justification,
)
.map_err(|e| {
log::error!(
tracing::error!(
target: LOG_TARGET,
"Received invalid justification for {:?}: {:?}",
hash,
e,
error=?e,
?hash,
"Received invalid justification"
);
<Error<T, I>>::InvalidJustification
})?)
Expand All @@ -714,7 +714,7 @@ pub mod pallet {
// Update ring buffer pointer and remove old header.
<ImportedHashesPointer<T, I>>::put((index + 1) % T::HeadersToKeep::get());
if let Ok(hash) = pruning {
log::debug!(target: LOG_TARGET, "Pruning old header: {:?}.", hash);
tracing::debug!(target: LOG_TARGET, ?hash, "Pruning old header.");
<ImportedHeaders<T, I>>::remove(hash);
}
}
Expand All @@ -729,10 +729,9 @@ pub mod pallet {
let authority_set_length = authority_list.len();
let authority_set = StoredAuthoritySet::<T, I>::try_new(authority_list, set_id)
.inspect_err(|_| {
log::error!(
tracing::error!(
target: LOG_TARGET,
"Failed to initialize bridge. Number of authorities in the set {} is larger than the configured value {}",
authority_set_length,
"Failed to initialize bridge. Number of authorities in the set {authority_set_length} is larger than the configured value {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

is {authority_set_length} on purpose formatted into a string instead of used as structured param ?

Copy link
Contributor Author

@raymondkfcheung raymondkfcheung Jul 23, 2025

Choose a reason for hiding this comment

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

Originally, authority_set_length is a string. We can change it to %authority_set_length.

T::BridgedChain::MAX_AUTHORITIES_COUNT,
);
})?;
Expand Down
8 changes: 8 additions & 0 deletions prdoc/pr_9294.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
title: Replace `log` with `tracing` on `pallet-bridge-grandpa`
doc:
- audience: Runtime Dev
description: This PR replaces `log` with `tracing` instrumentation on `pallet-bridge-grandpa`
by providing structured logging.
crates:
- name: bridge-runtime-common
bump: minor
Loading