-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Implement detailed logging for XCM failures #8724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement detailed logging for XCM failures #8724
Conversation
|
/cmd prdoc --audience runtime_dev --bump patch |
acatangiu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change all the log::error! to log::debug! - error logs are reserved for errors relevant to node operator - these types of runtime errors should be coming up only when debugging (otherwise validators get spammed with error logs for on-chain stuff that has no relevance to them).
The rest is good!
| _ => { | ||
| // Anything else is unhandled. This includes a message that is not meant for us. | ||
| // We need to make sure that dest/msg is not consumed here. | ||
| log::error!(target: LOG_TARGET, "Failed to validate XCM destination: unexpected location {d:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely not error here, because we use XcmpQueue in XcmRouter tuple and for example on AssetHubs, we have other routers after XcmpQueue for Ethereum or Polkadot/Kusama bridge routers.
So basically, we would log this error for every other location than (1, [Parachain(id)]) => {
| log::error!(target: LOG_TARGET, "Failed to validate XCM destination: unexpected location {d:?}"); | |
| log::trace!(target: LOG_TARGET, "Failed to validate XCM destination: unexpected location {d:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I use debug or trace on xcmp-queue/src/lib.rs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tuple SendXcm routing is expected not to pass depending on the d, so definitely trace,
maybe questionable if really need a log here, but if so, then trace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, removed logs.
| _ => { | ||
| // Anything else is unhandled. This includes a message that is not meant for us. | ||
| // We need to make sure that dest/msg is not consumed here. | ||
| log::debug!(target: LOG_TARGET, "Failed to validate XCM destination: unexpected location {d:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we want to remove this log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| let asset_amount = BalanceConverter::to_asset_balance(amount, asset_id) | ||
| .map_err(|_| XcmError::TooExpensive)?; | ||
| .map_err(|error| { | ||
| log::debug!(target: "xcm::charge_weight_in_fungibles", "AssetFeeAsExistentialDepositMultiplier Cannot convert to valid balance (possibly below ED): {error:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| log::debug!(target: "xcm::charge_weight_in_fungibles", "AssetFeeAsExistentialDepositMultiplier Cannot convert to valid balance (possibly below ED): {error:?}"); | |
| log::debug!(target: "xcm::charge_weight_in_fungibles", "AssetFeeAsExistentialDepositMultiplier cannot convert to valid balance (possibly below ED): {error:?}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
| .map_err(|e| XcmError::FailedToTransactAsset(e.into()))?; | ||
| let _ = Currency::withdraw(&who, amount, WithdrawReasons::TRANSFER, AllowDeath).map_err( | ||
| |error| { | ||
| tracing::debug!(target: "xcm::currency_adapter", ?error, "Failed to withdraw asset"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add here &who, amount to the log?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| Currency::transfer(&from, &to, amount, AllowDeath) | ||
| .map_err(|e| XcmError::FailedToTransactAsset(e.into()))?; | ||
| Currency::transfer(&from, &to, amount, AllowDeath).map_err(|error| { | ||
| tracing::debug!(target: "xcm::currency_adapter", ?error, "Failed to transfer asset"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here, I would add &from, &to, amount to the log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
| .map_err(|error| XcmError::FailedToTransactAsset(error.into()))?; | ||
| Fungible::transfer(&source, &dest, amount, Expendable).map_err(|error| { | ||
| tracing::debug!( | ||
| target: "xcm::fungible_adapter", ?error, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here: &source, &dest, amount - please check also other logs, if possible to add without clone
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
prdoc/pr_8724.prdoc
Outdated
| This PR improves diagnostics in XCM-related code by adding detailed error logging, especially within map_err paths. It includes clearer messages, standardized log targets, and richer context to aid runtime developers and node operators in debugging and monitoring. | ||
| crates: | ||
| - name: cumulus-pallet-xcmp-queue | ||
| bump: minor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be patch according to https://github.com/paritytech/polkadot-sdk/actions/runs/15417460762/job/43383686510?pr=8724#step:11:103
| bump: minor | |
| bump: patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
* master: omni-node: fix `benchmark pallet` to work with `--runtime` (#8594) Handle and suppress "New unknown `FromSwarm` libp2p event" warning (#8731) Implement detailed logging for XCM failures (#8724) [pallet-revive] contract's nonce starts at 1 (#8734) sync/fix: Clear gap sync on known imported blocks (#8445) [PoP] Add personhood tracking pallets (#8164) client/net: Use litep2p as the default network backend (#8461) Unflake `returns_status_for_pruned_blocks` (#8709) [AHM] Report the weights of epmb pallet to expose kusama and polkadot weights (#8704) Remove all XCM dependencies from `pallet-revive` (#8584) Docker master image tag fix (#8711) Record ed as part of the storage deposit (#8718) [pallet-revive] update dry-run logic (#8662) feat: add collator peer ID to ParachainInherentData (#8708) Nest errors in pallet-xcm (#7730) pallet-assets ERC20 precompile (#8554) Broker: Introduce min price + adjust renewals to lower market. (#8630) [AHM] Staking async fixes for XCM and election planning (#8422) Staking (EPMB): Add defensive error handling to voter snapshot creation and solution verification (#8687)
This PR enhances diagnostics across XCM-related components by implementing more detailed logging, especially for failures. The primary aim is to improve visibility into XCM errors (e.g., within `map_err` blocks), enabling faster troubleshooting for developers and node operators. Key enhancements include: * Logging specific error conditions (e.g., `BadVersion`, `BadLocation`, execution errors). * Including essential context in error logs (e.g., message hashes, origin, destination, relevant parameters). * Standardising log targets for easier analysis (e.g., `xcm::module::function_name`). * Clarifying log messages. This work continues #7003 and partially addresses #6119. ## Integration Downstream projects using XCM functionalities should see improved diagnostic logs without needing direct integration changes, as this primarily enhances internal logging. Node operators will find new, more detailed XCM error logs, aiding in monitoring and troubleshooting. Log parsing scripts might need updates for new log formats. ## Review Notes This PR enhances logging throughout XCM-related code paths, particularly in error handling (e.g., `map_err` blocks), to improve failure visibility. Reviewers are encouraged to assess: * **Clarity & Actionability:** Are logs clear, concise, and actionable for failures? * **Context:** Do error logs capture essential context (message hash, origin, destination, error specifics, parameters) appropriately for their level? * **Log Levels:** Is the chosen log level (e.g., `error`, `warn`, `debug`, `trace`) suitable? Are any level adjustments appropriate? * **Targets:** Are `target:` fields (e.g., `xcm::module::function`) consistent and specific? * **Error Accuracy:** Do logs accurately reflect the handled error condition? * **Impact:** Logging should minimally affect core logic and performance; favour simple logging of existing/easily derived data. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR replaces `log` with `tracing` instrumentation on XCM-related modules to significantly improve debugging capabilities for XCM execution flows. Continues #8724 and partially addresses #6119 by providing structured logging throughout XCM components, making it easier to diagnose execution failures, fee calculation errors, and routing issues. ## Key Features - **Consistent targets**: All components use predictable `xcm::*` log targets - **Structured fields**: Uses `?variable` syntax for automatic Debug formatting - **Zero runtime impact**: No behavioural changes, only observability improvements
This PR enhances diagnostics across XCM-related components by implementing more detailed logging, especially for failures. The primary aim is to improve visibility into XCM errors (e.g., within `map_err` blocks), enabling faster troubleshooting for developers and node operators. Key enhancements include: * Logging specific error conditions (e.g., `BadVersion`, `BadLocation`, execution errors). * Including essential context in error logs (e.g., message hashes, origin, destination, relevant parameters). * Standardising log targets for easier analysis (e.g., `xcm::module::function_name`). * Clarifying log messages. This work continues #7003 and partially addresses #6119. ## Integration Downstream projects using XCM functionalities should see improved diagnostic logs without needing direct integration changes, as this primarily enhances internal logging. Node operators will find new, more detailed XCM error logs, aiding in monitoring and troubleshooting. Log parsing scripts might need updates for new log formats. ## Review Notes This PR enhances logging throughout XCM-related code paths, particularly in error handling (e.g., `map_err` blocks), to improve failure visibility. Reviewers are encouraged to assess: * **Clarity & Actionability:** Are logs clear, concise, and actionable for failures? * **Context:** Do error logs capture essential context (message hash, origin, destination, error specifics, parameters) appropriately for their level? * **Log Levels:** Is the chosen log level (e.g., `error`, `warn`, `debug`, `trace`) suitable? Are any level adjustments appropriate? * **Targets:** Are `target:` fields (e.g., `xcm::module::function`) consistent and specific? * **Error Accuracy:** Do logs accurately reflect the handled error condition? * **Impact:** Logging should minimally affect core logic and performance; favour simple logging of existing/easily derived data. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR replaces `log` with `tracing` instrumentation on XCM-related modules to significantly improve debugging capabilities for XCM execution flows. Continues #8724 and partially addresses #6119 by providing structured logging throughout XCM components, making it easier to diagnose execution failures, fee calculation errors, and routing issues. ## Key Features - **Consistent targets**: All components use predictable `xcm::*` log targets - **Structured fields**: Uses `?variable` syntax for automatic Debug formatting - **Zero runtime impact**: No behavioural changes, only observability improvements
This PR enhances diagnostics across XCM-related components by implementing more detailed logging, especially for failures. The primary aim is to improve visibility into XCM errors (e.g., within
map_errblocks), enabling faster troubleshooting for developers and node operators.Key enhancements include:
BadVersion,BadLocation, execution errors).xcm::module::function_name).This work continues #7003 and partially addresses #6119.
Integration
Downstream projects using XCM functionalities should see improved diagnostic logs without needing direct integration changes, as this primarily enhances internal logging. Node operators will find new, more detailed XCM error logs, aiding in monitoring and troubleshooting. Log parsing scripts might need updates for new log formats.
Review Notes
This PR enhances logging throughout XCM-related code paths, particularly in error handling (e.g.,
map_errblocks), to improve failure visibility.Reviewers are encouraged to assess:
error,warn,debug,trace) suitable? Are any level adjustments appropriate?target:fields (e.g.,xcm::module::function) consistent and specific?