Conversation
…tHub fails on relay chain Emit SessionKeysUpdateFailed with the operation type and dispatch error for observability so set_keys/purge_kets failures from AssetHub are observable on-chain.
|
/cmd prdoc --audience runtime_dev --bump major |
sigurpol
added
T2-pallets
…time_dev --bump major'
| stash, | ||
| update: SessionKeysUpdate::Set, | ||
| }); | ||
| match T::SessionInterface::set_keys(&stash, session_keys) { |
There was a problem hiding this comment.
Are you sure this will fix the issue?
I looked a bit late yesterday into the:
message-queue -> T::MessageProcessor -> XcmExecutor::process -> ..
code path, and I was not sure how we can have success: true while the inner code path has failed somewhere.
I also am pretty sure that XcmExecutor runs in transactional layer, meaning that any failures, if happens, reverts all storage changes including events.
My knowledge OOTMH is spotty here, but just raising some concerns.
There was a problem hiding this comment.
But we are returning Ok(()) now even when session pallet update fails.
There was a problem hiding this comment.
Exactly, as Ankan said .
MY understanding :
how we can have success: true while the inner code path has failed somewhere.
we call dispatch here -> dispatch returns an error here -> but we swallow the error and return Ok() unconditionally here
I also am pretty sure that XcmExecutor runs in transactional layer, meaning that any failures, if happens, reverts all storage changes including events.
This would be true if set_keys_from_ah returned Err but we are returning Ok() on purpose
There was a problem hiding this comment.
what I am less sure about (maybe @franciscoaguirre can you help here?) is that the self.transact_status containing the error is effectively surfaced ONLY if we have a following ReportTransactStatus - that we don't have in our case. Maybe we should as alternative approach (very uncharted territories for me). But so far staking pallets seemed to adopt the approach returns Ok() + events for observability (debatable but out of the scope of this PR)
There was a problem hiding this comment.
so if we wanted to go with ReportTransactStatus we would need quick few changes if I get it right:
- on RC side, add ReportTransactStatus at the end of build_xcm here https://github.com/paritytech/polkadot-sdk/blob/sigurpol-better-error-handlingh-session-duplicate-keys/substrate/frame/staking-async/rc-client/src/lib.rs#L154-L154
- on AH side , we would need to register the query before sending with some callback or similar to be notified when response arrive or something like that (again, Cisco knows and I clearly don't :) )
Might be worth investigating for the future but this PR aimed to be a quick fix for better observability and no more than that
| }, | ||
| }; | ||
|
|
||
| T::SessionInterface::set_keys(&stash, session_keys)?; |
There was a problem hiding this comment.
Nice 👌. I guess lesson for us is: xcm receiver calls should always succeed (except for origin privilege).
sigurpol
deleted the
sigurpol-better-error-handlingh-session-duplicate-keys
branch
|
Successfully created backport PR for |
…ttHub fails on relay chain (#11055) Emit SessionKeysUpdateFailed with the operation type and dispatch error for observability so set_keys/purge_kets failures from AssetHub are observable on-chain. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 3512a73)
…ttHub fails on relay chain (#11055) Emit SessionKeysUpdateFailed with the operation type and dispatch error for observability so set_keys/purge_kets failures from AssetHub are observable on-chain. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 3512a73)
|
Successfully created backport PR for |
Backport #11055 into `stable2512` from sigurpol. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Backport #11055 into `stable2603` from sigurpol. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
github-project-automation
bot
moved this from Todo
to Done
in [preview] release tracker
Backport paritytech#11055 into `stable2512` from sigurpol. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Paolo La Camera <paolo@parity.io> Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
5 participants
Emit SessionKeysUpdateFailed with the operation type and dispatch error for observability so set_keys/purge_kets failures from AssetHub are observable on-chain.