[Staking] Patch candidate bond more to update CandidatePool + hotfix extrinsic to fix incorrect state

[4meta5]

What does it do?

patches candidate bond more to update the candidate pool value

Adds hotfix extrinsic hotfix_update_candidate_pool_value that takes as input Vec<T::AccountId> and is only callable via root. It enables passing in all candidate account_ids that were affected by the bug and updating their candidate pool value to the correct value.

short-term solution for users is to call go_offline, go_online directly after each other to fix incorrect CandidatePool value; the incentive for doing so is higher probability of selection as a collator at round transition

What important points reviewers should know?

This PR removes all incorrect bond comparisons in the tests. We cannot compare Bonds in that way unless we first remove the OrderedSet dependency and then fix the PartialEq impl for Bond.

Is there something left for follow-up PRs?

Optimizing DelegatorState and removing the dependency of OrderedSet will be be next in the same PR, coming after #1117

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

[4meta5]

As reported by @notlesh , this test passed because the owners are the same and the PartialEq impl for Bond uses the owners (the reason for this was owner was the uniqueness requirement for OrderedSet so we need to remove usage of that in the way it is used to remove the impl). Anyway, the short term solution is to stop comparing Bonds in that way in the tests which is now done.

#[test]
fn candidate_bond_more_updates_candidate_pool() {
	ExtBuilder::default()
		.with_balances(vec![(1, 50)])
		.with_candidates(vec![(1, 20)])
		.build()
		.execute_with(|| {
			assert_eq!(
				Stake::candidate_pool().0[0],
				Bond {
					owner: 1,
					amount: 20
				}
			);
			assert_ok!(Stake::candidate_bond_more(Origin::signed(1), 30));
			assert_eq!(
				Stake::candidate_pool().0[0],
				Bond {
					owner: 1,
					amount: 50
				}
			);
		});
}

[notlesh]

I actually think your note about doing an extrinsic to fix this rather than a migration is a really good idea, especially in this case. Here are some pretty compelling reasons to prefer a "hotfix extrinsic" over a migration:

• migrations:
  • come with more planning overhead
  • execute at a fixed time
  • are not properly paid for
  • can be an attack vector
  • may brick a chain when they fail
  • difficult to test / have less tooling
• "hotfix extrinsics":
  • can be executed whenever needed
  • can be executed in small chunks
  • can only create an invalid block if they fail
  • less of an attack vector (or none at all)
  • are properly paid for
  • easier testing

I'm going to play around with a PoC, I'll share it with you if I get anywhere with it...

[notlesh]

Actually, a hotfix extrinsic would be trivial, right? Basically this:

        #[pallet::weight(0)] // TODO
        /// Hotfix to fix the state of any candidate whose CandidatePool state is incorrect
        pub fn hotfix_fix_candidate_pool_storage_for(
            origin: OriginFor<T>,
            candidate: T::AccountId,
        ) -> DispatchResultWithPostInfo {
            let _account = ensure_signed(origin)?; // TODO: only root? only self? anyone?
            let state = <CandidateState<T>>::get(&candidate).ok_or(Error::<T>::CandidateDNE)?;
            Self::update_active(candidate, state.total_counted);
            Ok(().into())
        }

[4meta5]

I actually think your note about doing an extrinsic to fix this rather than a migration is a really good idea, especially in this case. Here are some pretty compelling reasons to prefer a "hotfix extrinsic" over a migration:
Actually, a hotfix extrinsic would be trivial, right? Basically this...

Sounds good, I'll move the migration into an extrinsic which is trivial, but it should take an input candidates: Vec<T::AccountId> so we can just propose one call instead of a bunch.

The con of using a hotfix extrinsic is that execution is not automatic so if there is inconsistent state that we do not notice, it will persist.

[notlesh]

I actually think your note about doing an extrinsic to fix this rather than a migration is a really good idea, especially in this case. Here are some pretty compelling reasons to prefer a "hotfix extrinsic" over a migration:
Actually, a hotfix extrinsic would be trivial, right? Basically this...

Sounds good, I'll move the migration into an extrinsic which is trivial, but it should take an input candidates: Vec<T::AccountId> so we can just propose one call instead of a bunch.

The con of using a hotfix extrinsic is that execution is not automatic so if there is inconsistent state that we do not notice, it will persist.

Yes, we should definitely make sure this can't leave things in an inconsistent state. One way to help us ensure that is to require root origin. I imagine it will be us calling this anyway.

Making it take a vec is fine, but we can also batch these through the utility pallet. Might as well have both, I think.

[girazoki]

Is there anyway we can test the new hotfix_update_candidate_pool_value? Maybe I missed it but I dont see a test for it, and I think it would be good to make sure it does what we expect. What if in the test, we insert a wrong value in candidate_pool and call the hotfix extrinsic?

[girazoki]

I think having a unitest is enough

[4meta5]

Is there anyway we can test the new hotfix_update_candidate_pool_value? Maybe I missed it but I dont see a test for it, and I think it would be good to make sure it does what we expect.

It's really simple code but I'm adding a unit test for it now. We use the update_active helper method everywhere and this basically calls it for all the candidates passed in as input.

What if in the test, we insert a wrong value in candidate_pool and call the hotfix extrinsic?

This is not possible. The input is not the candidate pool value, it is the list of candidates. Then we go through this list, get their CandidateState and put the CandidateState.total_counted into the CandidatePool as their associated amount. Because CandidateState was always correct, this ensures the new CandidatePool value will be correct.

I see, yes am doing this now.

[girazoki]

Sorry, I meant polluting CandidatePool in storage to wrong value (manually setting the wrong value), and the call this extrinsic making sure it fixes it.

[notlesh]

Sorry, I meant polluting CandidatePool in storage to wrong value (manually setting the wrong value), and the call this extrinsic making sure it fixes it.

This is the sort of thing I had in mind when I talked about an inconsistent state above (or, for that matter, doing something like kicking someone else out, etc.) It probably suffices to have a test or two showing that you can't invoke this against a candidate that isn't in the pool, though.

[4meta5]

@girazoki @notlesh The added pallet unit test verifies that a corrupt CandidatePool is corrected by calling the hotfix extrinsic && passing in a non-candidate to the hotfix extrinsic has no negative effect.