Remove Default bound for AccountId

[gavofyork]

Polkadot companion: paritytech/polkadot#4500
Cumulus companion: paritytech/cumulus#842

Having a default AccountId effectively privileges the zero account as a special case, to be used when "no other account makes sense". This should obviously not be the case. AccountId conceptually does not have a default value and therefore the implementation should not exist and pallets should not assume it does.

There are several API ramifications of this change, though all of them have simple solutions.

Code migration

SignedExtension

Pre no longer has a Default bound. This means that the default implementation for pre_dispatch (which executes the validate function and returns a default Pre value) cannot exist and must be defined at the implementation site. There is a "standard" implementation which is functionally equivalent to the previous default implementation as long as Pre is ():

fn pre_dispatch(
	self,
	who: &Self::AccountId,
	call: &Self::Call,
	info: &sp_runtime::traits::DispatchInfoOf<Self::Call>,
	len: usize,
) -> Result<Self::Pre, sp_runtime::transaction_validity::TransactionValidityError> {
	Ok(self.validate(who, call, info, len).map(|_| ())?)
}

This can generally just be dropped in to a SignedExtension impl which was not otherwise defining it.

Authoring pallet fn author

pallet_authoring::author() now returns an Option, which is None if the block author is unknown. This should not typically be the case on production chains, but often is the case for tests. A future question is whether we really want to support this or rather to force tests to introduce a dummy author. The easiest way to transition is wrap any usage in an if let Some(author) = author() { ... }.

Session keys in tests

Session pallet now forces the existence of session keys for validators whereas previously it used default keys if they had not been set. This means that tests must explicitly set any session keys of validator accounts if they expect to see the account in the list of elected validators.

Public trait split into ByteArray

The functions within the sp_core::crypto::Public trait which worked on the assumption that the underlying type was a fixed-size u8 array are now in a new trait ByteArray. Public implies this. Generally you should use sp_core::crypto::ByteArray; instead of Public.

from_slice is fallible

The function ByteArray::from_slice (which used to be Public::from_slice) has been changed to return a Result in the case that the slice provided was the incorrect size. For an infallible conversion, use the UncheckedFrom trait with a fixed size array.

Sudo pallet GenesisConfig

Sudo pallet's GenesisConfig::key parameter is now an Option<AccountId> since a safe default value for the field must be provided. Wrap any previous value in Some, so that what used to be:

sudo: my_chain_runtime::SudoConfig { key: root_key },

Now becomes:

sudo: my_chain_runtime::SudoConfig { key: Some(root_key) },

[kianenigma]

About this: after looking at the code, this bit seems like the only controversial bit. My conclusion is that this diff is not changing the logic of this snippet at all. The Default::default(), and min_index = 0 were clearly meant to be overwritten in the for loop that follows. In fact, the outcome is (pretty bad) UB if it doesn't. The changes of this PR are not altering any of that. The assumption still must hold that these values get written to. Making them an Option<_> doesn't harm that.

I wish I had the time now, or in the near future, to re-write the majority of this reduce algorithm. Admittedly, it is the last historical Spaghetti code left in sp-npos-election that I myself even have a very hard time reading. It was mostly written during a crunch and never been re-visited ever since. While I can't take the time now to improve this code fundamentally, I did propose some reasonable changes in this branch to make it more compatible with Option<_>, most notably making the assumption explicit that these Option will all become Some and stop the algorithm otherwise.

Lastly, luckily this code has a fuzzer that has proven quite helpful in the past. I'll re-run it again on my aforementioned branch and ring some bells if anything is broken.

Feel free to either apply the commits of https://github.com/paritytech/substrate/compare/gav-no-default-accountid...gav-no-default-accountid-staking?expand=1 either directly here, or I can open a PR on top of this. All changes in it are pretty clear in my opinion.