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

[Staking] Patch delegations total mismatch #1291

Merged
merged 7 commits into from Feb 17, 2022

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Feb 16, 2022

What does it do?

MOON-1517

  • unit tests that fail before changes and pass after
  • migration to fix incorrect state (by resumming all the delegations for all candidates)
  • unit test for migration
  • add migration to migrations config

Summary of Bug

The totalTopDelegations value was not updated after delegator_bond_more which means that total_counted = collator_bond + totalTopDelegations was not correctly updated and total_counted is used to determine which candidates are chosen in the active set.

Consequences

This bug does not enable abuse AFAICT. It does mean some data is incorrect, but the runtime migration fixes it.

It will understate the total_counted for collators that have delegations which were increased via delegator_bond_more. Moreover, it will overestimate each delegation share when paying out rewards leading to increased inflation rewards. Likewise, the patch needs to be applied ASAP to minimize the impact on reward payouts.

@4meta5 4meta5 added A3-inprogress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Feb 16, 2022
@4meta5 4meta5 marked this pull request as ready for review February 16, 2022 08:55
@4meta5 4meta5 added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Feb 16, 2022
pallets/parachain-staking/src/lib.rs Show resolved Hide resolved
pallets/parachain-staking/src/lib.rs Show resolved Hide resolved
pallets/parachain-staking/src/lib.rs Show resolved Hide resolved
T::Currency::unreserve(&delegator, amount);
if state.is_active() && total_changed {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment here as before, what happens if its bottom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if its just bottom then total_changed == false so it would never enter this if branch

pallets/parachain-staking/src/migrations.rs Outdated Show resolved Hide resolved
…g why we have conditional candidate pool update
@girazoki
Copy link
Collaborator

Unrelated, but can you comment the xcm migrations that have been applied in 1200 too? I think this PR might be a good opportunity to do so

@girazoki
Copy link
Collaborator

@4meta5 did you check with try-runtime?

@girazoki
Copy link
Collaborator

I recall @notlesh said try-runtime was broken

@4meta5
Copy link
Contributor Author

4meta5 commented Feb 17, 2022

It compiles with the try-runtime flag, but I'm not sure how to use it to test because the state needs to be corrupted before the migration is called. It was more direct do this in the unit test context because I could set the state directly to corrupt it like what the bug caused.

@librelois
Copy link
Collaborator

It compiles with the try-runtime flag, but I'm not sure how to use it to test because the state needs to be corrupted before the migration is called. It was more direct do this in the unit test context because I could set the state directly to corrupt it like what the bug caused.

The purpose of try-runtime is to be able to download a storage of a live chain for testing on real data.

To test the migration you can use the following command:

./target/release/moonbeam try-runtime on-runtime-upgrade live -u <rpc_endpoint> -a <block_hash> -p ParachainStaking,Migrations

Thus, try-runtime will download only the storage entries for the listed pallets instances.

@librelois librelois mentioned this pull request Feb 17, 2022
20 tasks
@girazoki
Copy link
Collaborator

girazoki commented Feb 17, 2022

I think for what @librelois wants to do you need to have a node synched (right @notlesh ?). If so, what you can is get the state of the corrupted storage in one of the chains for an account (with the RPC getStorage, by inserting the key of the corrupted storage).

Then you can launch locally a network, and use sudo.setStorage, same key as before, and you will have your corrupted storage there, and you can pass your migration to your local network

@librelois
Copy link
Collaborator

I think for what @librelois wants to do you need to have a node synched. If so, what you can is get the state of the corrupted storage in one of the chains for an account (with the RPC getStorage, by inserting the key of the corrupted storage).

Then you can launch locally a network, and use sudo.setStorage, same key as before, and you will have your corrupted storage there, and you can pass your migration to your local network

No, with try-runtime you don't need any node synced

@4meta5
Copy link
Contributor Author

4meta5 commented Feb 17, 2022

I ran it on moonbase and try-runtime executed without error, logs look like it worked and did fix a few candidate total_counted

Command and output

@4meta5 4meta5 added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Feb 17, 2022
@4meta5 4meta5 merged commit 30b9553 into master Feb 17, 2022
@4meta5 4meta5 deleted the amar-patch-staking-sum-neq-total branch February 17, 2022 20:00
@crystalin crystalin added the D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Feb 18, 2022
@crystalin
Copy link
Collaborator

@4meta5 can you get the PoV size also with try-runtime ?

@4meta5
Copy link
Contributor Author

4meta5 commented Feb 18, 2022

@crystalin for moonbase, it is included in the bottom of the output https://gist.github.com/4meta5/7ef1357d2ceca43476566284ea362856

2022-02-18 10:08:56 proof size: 1.73 MB (1772.384765625 KB) (1814922 bytes)    
2022-02-18 10:08:56 compact proof size: 1.72 MB (1756.1943359375 KB) (1798343 bytes)    
2022-02-18 10:08:56 zstd-compressed compact proof 1.45 MB (1482.875 KB) (1518464 bytes)    
2022-02-18 10:08:56 TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 186050000000, total weight = 500000000000 (0.3721)

for moonriver/moonbeam, try-runtime does not yet work (MOON-1200 is the ticket to update it)

@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants