pallet-session: track consumer refs and release deposits for externally set keys

[sigurpol]

Tracks whether session keys were set externally (via SessionInterface, e.g. from AH) or locally.
Transitions between the two paths correctly manage the key deposit and consumer ref in both directions.

[sigurpol]

/cmd prdoc --audience runtime_dev --bump minor

[Ank4n]

Wouldn't it also work if we just incremented and decremented consumer when set(new)/purge via SessionInterface as well? Simpler I think, and consumer would still be correctly accounted?

[sigurpol]

Wouldn't it also work if we just incremented and decremented consumer when set(new)/purge via SessionInterface as well? Simpler I think, and consumer would still be correctly accounted?

Yes it would be simpler but for a new validator who only sets keys via AH , there is no real need to have an account on RC and so no need to increment the counter (or to hold a deposit on RC).
I believe that having a clear distinction between “keys set on AH” ( and so no associated counter / deposit needed ) and “keys set on RC” ( mandatory inc counter plus deposit if key deposit is set) is more logically correct and cleaner and not too complicated either.

But if the consensus is to go with your proposal , I can def make the changes

[kianenigma]

I am not convinced this is the right way to fix this. Now the session pallet needs to know about the idea of external keys, which I think can be avoided:

1. If key is set locally, consumer and deposit behavior is atm correct.
2. If key is set remotely, an inner code path that ignores consumer and deposit should be called. The remote chain handles the deposit. Consumer can be fully ignored, because your account that is actually alive is in another chain. Perhaps the remote chain should tweak your consumer on the remote chain, but not here.

[sigurpol]

I am not convinced this is the right way to fix this. Now the session pallet needs to know about the idea of external keys, which I think can be avoided:

1. If key is set locally, consumer and deposit behavior is atm correct.
2. If key is set remotely, an inner code path that ignores consumer and deposit should be called. The remote chain handles the deposit. Consumer can be fully ignored, because your account that is actually alive is in another chain. Perhaps the remote chain should tweak your consumer on the remote chain, but not here.

And how would you handle the case of a key set on AH and purged on RC? If RC-purge doesn't have to know about external keys, then it will just decrease the counter...

ps note that atm (So w/o this PR) If key is set remotely, an inner code path that ignores [RC] consumer and deposit should be called -> this is the case already. The missing bit in your proposal would be deposit handling on remote chain (as discussed separately)

[kianenigma]

I am not convinced this is the right way to fix this. Now the session pallet needs to know about the idea of external keys, which I think can be avoided:

1. If key is set locally, consumer and deposit behavior is atm correct.
2. If key is set remotely, an inner code path that ignores consumer and deposit should be called. The remote chain handles the deposit. Consumer can be fully ignored, because your account that is actually alive is in another chain. Perhaps the remote chain should tweak your consumer on the remote chain, but not here.

And how would you handle the case of a key set on AH and purged on RC? If RC-purge doesn't have to know about external keys, then it will just decrease the counter...

I would fully disable RC session mgmt, and my comment was stemming from the idea that this is the path to go. Isn't that easier?

[sigurpol]

I am not convinced this is the right way to fix this. Now the session pallet needs to know about the idea of external keys, which I think can be avoided:

1. If key is set locally, consumer and deposit behavior is atm correct.
2. If key is set remotely, an inner code path that ignores consumer and deposit should be called. The remote chain handles the deposit. Consumer can be fully ignored, because your account that is actually alive is in another chain. Perhaps the remote chain should tweak your consumer on the remote chain, but not here.

And how would you handle the case of a key set on AH and purged on RC? If RC-purge doesn't have to know about external keys, then it will just decrease the counter...

I would fully disable RC session mgmt, and my comment was stemming from the idea that this is the path to go. Isn't that easier?

So ignoring the deposit (since on Polkadot and Kusama we are not setting KeyDeposit on RC and we won't so let's keep that out from the discussion) - the only drawback of disabling RC Session mgmt would be that we just don't decrement consumer on purge on AH

[UPDATE] to be noted also that setting keys on AH requires 10k DOT, settings keys on RC ED (1DOT)

[kianenigma]

Okay, reading the

	/// Accounts whose keys were set via `SessionInterface` (external path) without
	/// incrementing the consumer reference or placing a key deposit. `do_purge_keys`
	/// only decrements consumers for accounts that were registered through the local
	/// session pallet.
	#[pallet::storage]
	pub type ExternallySetKeys<T: Config> =
		StorageMap<_, Twox64Concat, T::AccountId, (), OptionQuery>;

Gives me a better idea of what is intended here.

Suggestion to simplify this + make sure it is a reasonable feature that won't bite us in the future:

• There is two distinct paths to set keys: Via other pallets (SessionInterface) and #[pallet::call]
• Former ignores consumer + deposit, latter manages them ()
• They should generally not share any code path.
• They cannot be inter-mixed -- If you set it via SessionInterface, you have to purge it with the same method. And visa versa.

My reading of the PR is that this will it easier to understand + still meet our needs. KISS: Keeping It Stupid Simple.

[sigurpol]

Okay, reading the

	/// Accounts whose keys were set via `SessionInterface` (external path) without
	/// incrementing the consumer reference or placing a key deposit. `do_purge_keys`
	/// only decrements consumers for accounts that were registered through the local
	/// session pallet.
	#[pallet::storage]
	pub type ExternallySetKeys<T: Config> =
		StorageMap<_, Twox64Concat, T::AccountId, (), OptionQuery>;

Gives me a better idea of what is intended here.

Suggestion to simplify this + make sure it is a reasonable feature that won't bite us in the future:

• There is two distinct paths to set keys: Via other pallets (SessionInterface) and #[pallet::call]
• Former ignores consumer + deposit, latter manages them ()
• They should generally not share any code path.
• They cannot be inter-mixed -- If you set it via SessionInterface, you have to purge it with the same method. And visa versa.

My reading of the PR is that this will it easier to understand + still meet our needs. KISS: Keeping It Stupid Simple.

I thought about exactly this and I wanted to implement this in the 1st place because keeps a proper separation of concerns.

The main reason I didn't go for that is that in the moment we remove the #[pallet::call] option and there are still keys set via RC, we can't purge it anymore (*unless we modify the call in a following runtime upgrade)
Secondly, we wanted to rely on "setting + purging keys via AH" as blessed way to proceed from now on - meaning that ideally. Maybe that's debatable, and should be the blessed way only for new validators only and not for existing ones.
But I think it would be nicer if an existing validator wants to renew existing keys and could do that directly on AH

[sigurpol]

There is also a brute-force approach that won't bite us in the future:

1. remove [#pallet::call] for set_keys/ purge_keys now
2. remove key deposit as well or not (it was set to 0 in any case on Polkadot and Kusama RC, doesn't matter)
3. optionally if we care on having a consistent consumers, each set/purge keys from AH just decrease consumer

[UPDATE]: which is the equivalent of disabling the call in the runtime then (Except that we can keep consumer ref correct) - so what you already proposed and to fast-track the release we could do that first

[kianenigma]

Upon further reading, I think this PR is fine. To convince myself more that it is right, I tried to break down all code paths and test coverage, and this is what I think we should test for:

// test coverage overview:
// set local, purge local
// set local, purge remote
// set remote, purge local
// set remote, purge remote
// non-purge scenarios:
// 		- set local, switch remote
// 		- set remote, switch local
// each test ensures at each step:
// 	- consumer count
// 	- deposit
// 	- externally set key

#[test]
fn set_local_purge_local() {}

#[test]
fn set_local_purge_remote() {}

#[test]
fn set_remote_purge_remote() {}

#[test]
fn set_remote_purge_local() {}

#[test]
fn set_local_to_remote() {}

#[test]
fn set_remote_to_local() {}

I think most are covered now, but perhaps this is a better breakdown of it. I think all other E2E scenarios are built from these 6 components.

Feel free to incorporate in whatever way it fits the PR best. Happy to do a final review afterwards.

[sigurpol]

Upon further reading, I think this PR is fine. To convince myself more that it is right, I tried to break down all code paths and test coverage, and this is what I think we should test for:

// test coverage overview:
// set local, purge local
// set local, purge remote
// set remote, purge local
// set remote, purge remote
// non-purge scenarios:
// 		- set local, switch remote
// 		- set remote, switch local
// each test ensures at each step:
// 	- consumer count
// 	- deposit
// 	- externally set key

#[test]
fn set_local_purge_local() {}

#[test]
fn set_local_purge_remote() {}

#[test]
fn set_remote_purge_remote() {}

#[test]
fn set_remote_purge_local() {}

#[test]
fn set_local_to_remote() {}

#[test]
fn set_remote_to_local() {}

I think most are covered now, but perhaps this is a better breakdown of it. I think all other E2E scenarios are built from these 6 components.

Feel free to incorporate in whatever way it fits the PR best. Happy to do a final review afterwards.

Done - PTAL 🙇

[Ank4n]

Wouldn't it also work if we just incremented and decremented consumer when set(new)/purge via SessionInterface as well? Simpler I think, and consumer would still be correctly accounted?

Yes it would be simpler but for a new validator who only sets keys via AH , there is no real need to have an account on RC and so no need to increment the counter (or to hold a deposit on RC). I believe that having a clear distinction between “keys set on AH” ( and so no associated counter / deposit needed ) and “keys set on RC” ( mandatory inc counter plus deposit if key deposit is set) is more logically correct and cleaner and not too complicated either.

But if the consensus is to go with your proposal , I can def make the changes

Having an extra storage in session pallet is almost equivalent to keeping an account on RC by incrementing consumer on it, no? And less code changes? Additionally, pretty soon we are going to get rid of setting keys locally so there will eventually be only one path.

This solution is perfectly fine btw and since you are already done with it, lets go with it 🚀.

[paritytech-release-backport-bot]

Successfully created backport PR for stable2512:

• [stable2512] Backport #11197 #11209

[paritytech-release-backport-bot]

Successfully created backport PR for stable2603:

• [stable2603] Backport #11197 #11210