Skip to content
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

More tests and checks confirming that ledger.controller is always correct. #2599

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Dec 4, 2023

A bonded ledger fetched with the StakingLedger implementation exposes a method ledger.controller() that returns the controller of the ledger. However, that controller is computed and stored under the ledger.controller field on the fly - i.e when the ledger is fetched from storage using the StakingLedger::get method. The controller field is never stored in storage.

This PR add a few more tests checks and improves the ledger try-state checks to make sure these invariants hold true.

@gpestana gpestana requested review from Ank4n, rossbulat and a team December 4, 2023 12:03
@gpestana
Copy link
Contributor Author

gpestana commented Dec 4, 2023

bot fmt

@command-bot
Copy link

command-bot bot commented Dec 4, 2023

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4590979 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 41-be19f69c-b59c-434f-920b-e76cc0094f57 to cancel this command or bot cancel to cancel all commands in this pull request.

@gpestana gpestana self-assigned this Dec 4, 2023
@command-bot
Copy link

command-bot bot commented Dec 4, 2023

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4590979 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4590979/artifacts/download.

@gpestana gpestana added T2-pallets This PR/Issue is related to a particular pallet. T10-tests This PR/Issue is related to tests. labels Dec 4, 2023
// `ledger.controller` is never stored in raw storage.
let raw = Ledger::<T>::get(stash).unwrap_or_else(|| {
Ledger::<T>::get(ctrl.clone())
.expect("try_check: bonded stash/ctrl does not have an associated ledger")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just log an error or panic? Since try state checks are mostly caused by older issues, we may not want CI to fail for these errors?

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Is there any inconsistent/wrong state in a production network?

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
@bkchr bkchr added the R0-silent Changes should not be mentioned in any release notes label Jan 18, 2024
@bkchr bkchr enabled auto-merge January 18, 2024 21:21
@bkchr bkchr added this pull request to the merge queue Jan 18, 2024
Merged via the queue into master with commit 9db9211 Jan 18, 2024
124 checks passed
@bkchr bkchr deleted the gpestana/staking-ledger_controller branch January 18, 2024 23:03
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…orrect. (paritytech#2599)

A bonded ledger fetched with the `StakingLedger` implementation exposes
a method `ledger.controller()` that returns the controller of the
ledger. However, that controller is computed and stored under the
`ledger.controller` field on the fly - i.e when the ledger is fetched
from storage using the `StakingLedger::get` method. The controller field
is never stored in storage.

This PR add a few more tests checks and improves the ledger try-state
checks to make sure these invariants hold true.

---------

Co-authored-by: command-bot <>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T2-pallets This PR/Issue is related to a particular pallet. T10-tests This PR/Issue is related to tests.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants