Feat/eth fee witnessing

[kylezs]

We have 3 storage items:

	/// A mapping from signer id to the chain amount (in Atomic units) that the signer is owed
	/// for paying transaction fees
	#[pallet::storage]
	pub type AccountIdTransactionFeeDeficit<T: Config<I>, I: 'static = ()> =
		StorageMap<_, Twox64Concat, T::AccountId, ChainAmountFor<T, I>, OptionQuery>;

	/// A mapping of signer id to the the account id of the authority that registered the signer.
	/// through a transaction_ready_for_transmission extrinsic.
	#[pallet::storage]
	pub type SignerIdToAccountId<T: Config<I>, I: 'static = ()> =
		StorageMap<_, Twox64Concat, SignerIdFor<T, I>, T::AccountId, OptionQuery>;

	/// A mapping from AccountId to the last registered SignerId, which is where the refunds
	/// will be sent to
	#[pallet::storage]
	pub type AccountIdToRefundSignerId<T: Config<I>, I: 'static = ()> =
		StorageMap<_, Twox64Concat, T::AccountId, SignerIdFor<T, I>, OptionQuery>;

On a transaction_ready_for_transmission() we whitelist the SignerId (non SC pubkey). This whitelisting is necessary so that we only refund authorities (since it's possible for anyone with ETH to read our chain and submit a transaction).
On a SignatureAccepted event which means this transaction has been transmitted and confirmed on the ETH chain, we do a lookup, using the SignerId contained in the SignatureAccepted, to get the AccountId of the authority whose address it is. We then increase the deficit by the fee amount paid in that tx.

In the future, refunds will be sent to each authority's RefundSignerId, which will be set to the most recent SignerId used to sign (assuming the transaction_ready_for_transmission for that submission was sent too)

State Chain

• Does this break CFE compatibility (API) - If yes/not sure, have you tagged relevant Engine Echidna on the PR?
• Were any changes to the genesis config of any pallets? If yes:
  • Has the Chainspec been updated accordingly?
• Is types.json up to date? Test this against polka js.
• Have any new dependencies been added? If yes:
  • Has Cargo.toml/std section been updated accordingly? Reference
• Has the external interface been changed? Have any extrinsics been updated or removed? If yes:
  • Has the runtime version been bumped accordingly (transaction_version and spec_version)
• Do the changes require a runtime upgrade? If yes:
  • Have any storage items or stored data types been modified? If yes:
    • Has the pallet's storage version been bumped and a storage migration been defined?

[morelazers]

This seems ok but I'll let someone else review in more detail before approving.

I'm not a huge fan of the fact that we're mapping SignerID (ETH address) to Deficit, and then providing a reverse mapping from ValidatorID to SignerID. This is going to increase the amount of lookups we're performing at the time of paying people back.

Number of storage reads would be greatly reduced by creating the mapping of ValidatorBroadcastFeeDeficit(ValidatorId => ChainAmount). In the current implementation, a Validator could use hundreds of accounts and then we eventually have to deal with this to the detriment of everyone else.

We don't really care which address the Validator used to sign, the only thing we care about is which Validator signed.

[kylezs]

Number of storage reads would be greatly reduced by creating the mapping of ValidatorBroadcastFeeDeficit(ValidatorId => ChainAmount). In the current implementation, a Validator could use hundreds of accounts and then we eventually have to deal with this to the detriment of everyone else.

There are fewer reads required doing it this way, because when we construct an ETH tx, we only need the signer (send to) and the deficit (amount)... which are in this single storage item. To have the ValidatorId would mean another query. Where are you seeing these other storage reads? We don't actually care which ValidatorId signed at this point, it's not related to ETH, they are conceptually separate things, and we can keep them separate doing it this way. We create the second mapping in case we change our minds later.

a Validator could use hundreds of accounts and then we eventually have to deal with this to the detriment of everyone else

Why would an authority do this? What do you mean by "deal with this to the detriment of everyone else" ?

Would argue it's a feature that authorities can change their address whenever they want with no worrying whether they'll get refunded to that new address now. They will as soon as they start signing with that address.

We don't really care which address the Validator used to sign, the only thing we care about is which Validator signed.



Not sure what you mean by this. We must care what address they used to sign, because we have to refund them to that address.

[morelazers]

Not sure what you mean by this. We must care what address they used to sign, because we have to refund them to that address.

We don't - this is the crux of my point.

As long as we refund the Validator, and we know that the Validator owns a particular address, it doesn't matter to which address the refund goes. E.g.

• Validator A spends 1 ETH on fees, equally distributed over five addresses
• State Chain now thinks that we have to make five separate refund transactions

This is a fallacy! We only need to make one refund transaction, as long as we know one of the addresses owned by the Validator.

We're concerned in this issue with ensuring that the Validator does not lose money, we don't care about an individual address.

[albert-llimos]

Approved the gas calculation part.

[dandanlen]

Only thing we might also add is a hook on account closure to ensure that if someone's account is killed, we remove all related account tracking storage.

Sounds like a reasonable suggestion. Would say it's out of scope for this PR though.

Yes - won't be a concern until we start refunding.

[kylezs]

Will wait for #1650 to merge before merging this.