Simplify the consensus chain storage proof in FP by gathering necessary storage into one place

[NingLin-P]

Tasks

Combine:

• randomness -> shuffling seed
• timestamp
• block fees (2 fields -> 1 field)
• block digest -> domain runtime upgrades
• domain chain allow list (key by domain ID)

Cleanups:

• move runtime API calls into a new CreateInherentDataProvider: Fix bug in fraud proof transaction fee value, and combine storage #3320 (review)

Description

Atm, there are many consensus chain storage proofs used by different fraud proof, see:

subspace/crates/sp-domains-fraud-proof/src/storage_proof.rs

Lines 55 to 67 in cd44905

	#[derive(Clone, Debug, Decode, Encode, Eq, PartialEq, TypeInfo)]
	pub enum FraudProofStorageKeyRequest<Number> {
	BlockRandomness,
	Timestamp,
	SuccessfulBundles(DomainId),
	TransactionByteFee,
	DomainAllowlistUpdates(DomainId),
	BlockDigest,
	RuntimeRegistry(RuntimeId),
	DynamicCostOfStorage,
	DomainSudoCall(DomainId),
	MmrRoot(Number),
	}

And we need to generate and verify each of them during fraud proof generation & verification, to simplify that we can gather all the small storage into one place which does introduce duplicated storage but will simplify things a lot (and even remove some of the proof data) as we only need to generate/verify one storage proof to cover many proof data.

For example:

struct FraudProofData {
    timestamp: Moment,
    block_randomness: Randomness,
    // Replace `TransactionByteFee` and `DynamicCostOfStorage`
    TransactionByteFee: BlockTransactionByteFee<Balance>,
    // Replace `BlockDigest`
    RuntimeUpgrade: Option<RuntimeId>,
    ...
}

// In pallet-domains:
fn on_finalize(_: BlockNumberFor<T>) {
    ...

    // if this consensus block will derive any domain block, gather the necessary storage for potential FP usage
    if !SuccessfulBundles::is_empty() || !DomainRuntimeUpgrades::is_empty() {
        let fraud_proof_data = ...;
        FraudProofData::set(Some(fraud_proof_data));
    }
}

Then the storage proof of FraudProofData can replace many storage proofs usage in https://github.com/autonomys/subspace/blob/main/crates/sp-domains-fraud-proof/src/fraud_proof.rs

cc @teor2345

[teor2345]

let fraud_proof_data = ...;

I think I'm a bit lost here, is the code here something like:

let fraud_proof_data = FraudProofData {
    block_randomness: StorageKeyProvider::storage_key(FraudProofStorageKeyRequest::BlockRandomness),
	...
};

Then the storage proof of FraudProofData can replace many storage proofs usage in main/crates/sp-domains-fraud-proof/src/fraud_proof.rs

In FraudProofVariant, FraudProof, or somewhere else?

subspace/crates/sp-domains-fraud-proof/src/fraud_proof.rs

Lines 318 to 352 in bdf31b8

	#[derive(Decode, Encode, TypeInfo, PartialEq, Eq, Clone)]
	pub struct FraudProof<Number, Hash, DomainHeader: HeaderT, MmrHash> {
	pub domain_id: DomainId,
	/// Hash of the bad receipt this fraud proof targeted
	pub bad_receipt_hash: HeaderHashFor<DomainHeader>,
	/// The MMR proof for the consensus state root that used to verify the storage proof
	///
	/// It is set `None` if the specific fraud proof variant doesn't contains storage proof
	pub maybe_mmr_proof: Option<ConsensusChainMmrLeafProof<Number, Hash, MmrHash>>,
	/// The domain runtime code storage proof
	///
	/// It is set `None` if the specific fraud proof variant doesn't required domain runtime code
	/// or the required domain runtime code is available from the current runtime state.
	pub maybe_domain_runtime_code_proof: Option<DomainRuntimeCodeAt<Number, Hash, MmrHash>>,
	/// The specific fraud proof variant
	pub proof: FraudProofVariant<Number, Hash, MmrHash, DomainHeader>,
	}
	#[allow(clippy::large_enum_variant)]
	#[derive(Debug, Decode, Encode, TypeInfo, PartialEq, Eq, Clone)]
	pub enum FraudProofVariant<Number, Hash, MmrHash, DomainHeader: HeaderT> {
	InvalidStateTransition(InvalidStateTransitionProof),
	ValidBundle(ValidBundleProof<Number, Hash, DomainHeader>),
	InvalidExtrinsicsRoot(InvalidExtrinsicsRootProof),
	InvalidBundles(InvalidBundlesProof<Number, Hash, MmrHash, DomainHeader>),
	InvalidDomainBlockHash(InvalidDomainBlockHashProof),
	InvalidBlockFees(InvalidBlockFeesProof),
	InvalidTransfers(InvalidTransfersProof),
	// Dummy fraud proof only used in test and benchmark
	//
	// NOTE: the `Dummy` must be the last variant, because the `#[cfg(..)]` will apply to
	// all the variants after it.
	#[cfg(any(feature = "std", feature = "runtime-benchmarks"))]
	Dummy,
	}

[NingLin-P]

I think I'm a bit lost here, is the code here something like:

let fraud_proof_data = FraudProofData {
block_randomness: StorageKeyProvider::storage_key(FraudProofStorageKeyRequest::BlockRandomness),
...
};

What I mean is something like:

pub struct FraudfProofDataStorageProof(StorageProof);

impl_storage_proof!(FraudfProofDataStorageProof);
impl<Block: BlockT> BasicStorageProof<Block> for FraudfProofDataStorageProof {
    type StorageValue = FraudfProofData;
    fn storage_key_request(_key: Self::Key) -> FraudProofStorageKeyRequest<NumberFor<Block>> {
        FraudProofStorageKeyRequest::FraudfProofData
    }
}

and FraudfProofDataStorageProof can be used to replace the storage proof in different fraud proof variants (mostly InvalidExtrinsicsRootProof but I didn't look closer at other fraud proofs).

[teor2345]

This is looking a bit complicated, I might start with one of the simpler proofs first.

[teor2345]

I've done some analysis here, and I think the InvalidExtrinsicsRoot is the only proof that will benefit from this change.

It's also possible to merge the storage for SuccessfulBundlesProof and DomainChainsAllowlistUpdateStorageProof, but their size is variable, and not guaranteed to be small (particularly in a fraud situation). And since they're in different fraud proof variants, we'd be duplicating the storage without actually reducing the number of proofs (I think?).

The other proofs are either:

• large extrinsics or execution (with included extrinsics): state transition, invalid bundles, DomainSudoCallStorageProof
• already stored (and as a single storage proof): domain block hash, block fees, transfers

They're also stored using different keys, which might complicate things.

[teor2345]

Here's a copy of my notes for the successful bundles proof. We should consider merging this fraud proof data, if the savings on fraud proof generation outweigh the data duplication.

ValidBundle
replaces SuccessfulBundlesProof
pub bundle_storage_proof: DomainId -> Vec<H256>,

InvalidExtrinsicsRoot
replaces DomainChainsAllowlistUpdateStorageProof
pub domain_chain_allowlist: DomainId -> DomainAllowlistUpdates(BTreeSet<ChainId> x 2),

[NingLin-P]

Let's leave SuccessfulBundlesProof and DomainChainsAllowlistUpdateStorageProof as it is, SuccessfulBundlesProof is used in many FPs, merging them may even make things complicated.

[teor2345]

I think I'm a bit lost here, is the code here something like:

let fraud_proof_data = FraudProofData {

// In pallet-domains:
fn on_finalize(_: BlockNumberFor<T>) {
    ...

    // if this consensus block will derive any domain block, gather the necessary storage for potential FP usage
    if !SuccessfulBundles::is_empty() || !DomainRuntimeUpgrades::is_empty() {
        let fraud_proof_data = ...;
        FraudProofData::set(Some(fraud_proof_data));
    }
}

I'm lost at this stage, I can't work out how to get the value of the block_randomness field in fraud_proof_data.

Should I make pallet_domains::Config depend on pallet_subspace::Config so that I can get pallet_subspace::BlockRandomness?

Or is there some other way to call code that needs T: pallet_subspace::Config inside pallet_domains, where T: pallet_domains::Config?

impl<T: pallet_domains::Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
    fn on_finalize(_: BlockNumberFor<T>) {
        let block_randomness = pallet_subspace::BlockRandomness::<T: pallet_subspace::Config>::get()
            .expect("randomness is always set on initialize; qed");

Here's my draft branch, I'm stuck on commit 1c563d3.

[NingLin-P]

Should I make pallet_domains::Config depend on pallet_subspace::Config so that I can get pallet_subspace::BlockRandomness?

Yes, that is what I expected, and it already exists:

subspace/crates/pallet-domains/src/lib.rs

Lines 382 to 383 in 3d5483e

	/// Randomness source.
	type Randomness: RandomnessT<Self::Hash, BlockNumberFor<Self>>;

[teor2345]

Should I make pallet_domains::Config depend on pallet_subspace::Config so that I can get pallet_subspace::BlockRandomness?

Yes, that is what I expected, and it already exists:

subspace/crates/pallet-domains/src/lib.rs

Lines 382 to 383 in 3d5483e

	/// Randomness source.
	type Randomness: RandomnessT<Self::Hash, BlockNumberFor<Self>>;

RandomnessT is:

subspace/crates/pallet-domains/src/lib.rs

Line 56 in 3d5483e

use frame_support::traits::{Get, Randomness as RandomnessT};

Which has methods that provide access to:

• hash(data | BlockRandomness)
• hash(BlockRandomness)
  

  subspace/crates/pallet-subspace/src/lib.rs

  Lines 1789 to 1814 in 3d5483e

  	impl<T: Config> frame_support::traits::Randomness<T::Hash, BlockNumberFor<T>> for Pallet<T> {
  	fn random(subject: &[u8]) -> (T::Hash, BlockNumberFor<T>) {
  	let mut subject = subject.to_vec();
  	subject.extend_from_slice(
  	BlockRandomness::<T>::get()
  	.expect("Block randomness is always set in block initialization; qed")
  	.as_ref(),
  	);
  	(
  	T::Hashing::hash(&subject),
  	frame_system::Pallet::<T>::current_block_number(),
  	)
  	}
  	fn random_seed() -> (T::Hash, BlockNumberFor<T>) {
  	(
  	T::Hashing::hash(
  	BlockRandomness::<T>::get()
  	.expect("Block randomness is always set in block initialization; qed")
  	.as_ref(),
  	),
  	frame_system::Pallet::<T>::current_block_number(),
  	)
  	}
  	}

So getting access to BlockRandomness itself will require a new trait and type alias, I guess.

[NingLin-P]

So getting access to BlockRandomness itself will require a new trait and type alias, I guess.

Not necessary, the BlockRandomness is used in FP to derive the extrinsics_shuffling_seed:

subspace/crates/sp-domains-fraud-proof/src/verification.rs

Lines 82 to 85 in 3d5483e

	let shuffling_seed = H256::from_slice(
	extrinsics_shuffling_seed::<Hashing>(domain_inherent_extrinsic_data.block_randomness)
	.as_ref(),
	);

which is identical to

subspace/crates/pallet-domains/src/lib.rs

Lines 2719 to 2723 in 3d5483e

	pub fn extrinsics_shuffling_seed() -> T::Hash {
	let seed = DOMAIN_EXTRINSICS_SHUFFLING_SEED_SUBJECT;
	let (randomness, _) = T::Randomness::random(seed);
	randomness
	}

so we can just store the extrinsics_shuffling_seed

The transaction fee should be something similar where we don't need DynamicCostOfStorageProof and ConsensusTransactionByteFeeProof but only the final consensus_transaction_byte_fee in the FraudProofData:

subspace/crates/sp-domains-fraud-proof/src/storage_proof.rs

Lines 512 to 529 in 3d5483e

	let dynamic_cost_of_storage =
	<DynamicCostOfStorageProof as BasicStorageProof<Block>>::verify::<SKP>(
	self.dynamic_cost_of_storage_proof.clone(),
	(),
	state_root,
	)?;
	let consensus_transaction_byte_fee = if dynamic_cost_of_storage {
	let raw_transaction_byte_fee =
	<ConsensusTransactionByteFeeProof as BasicStorageProof<Block>>::verify::<SKP>(
	self.consensus_chain_byte_fee_proof.clone(),
	(),
	state_root,
	)?;
	sp_domains::DOMAIN_STORAGE_FEE_MULTIPLIER * raw_transaction_byte_fee.next
	} else {
	Balance::from(1u32)
	};

[teor2345]

MaybeDomainRuntimeUpgradedProof's DomainRuntimeCodeProof has a key of RuntimeId, and DomainChainsAllowlistUpdateStorageProof has a key of DomainId.

Should they be combined into the shared storage? What should the key be?

We also use DomainRuntimeCodeProof in FraudProof.maybe_domain_runtime_code_proof, so I'm wondering if we should just leave MaybeDomainRuntimeUpgradedProof alone.

[NingLin-P]

Should they be combined into the shared storage? What should the key be?

We should use DomainId because runtime id can be found by domain id in the runtime during FP verification.

We also use DomainRuntimeCodeProof in FraudProof.maybe_domain_runtime_code_proof, so I'm wondering if we should just leave MaybeDomainRuntimeUpgradedProof alone.

The DomainRuntimeCodeProof in maybe_domain_runtime_code_proof and MaybeDomainRuntimeUpgradedProof is different, the former is the runtime code from the parent block and is used to execute the current block, the latter is the new runtime code used to upgrade in the current block.

We probably should not put the runtime code in InvalidInherentExtrinsicData as it is too large and duplicated in the state, but we can replace the BlockDigestProof with the storage proof of DomainRuntimeUpgrades, either we should put/or merge DomainRuntimeUpgrades into InvalidInherentExtrinsicData is fine to me.