beefy: put not only lease parachain heads into mmr #4751

ordian · 2024-06-10T16:30:41Z

Short-term addresses #4737.

Resolve benchmarking
I've digged into benchmarking mentioned offchain XCMP: Merkle tree of para headers in beefy ignores Coretime chains #4737 (comment), but it seemed to me that this code is different proof/path. @acatangiu could you confirm? (btw, in this bench, where do you actually set the fn parachains() to a reasonable number? i've only seen 1)
Communicate to Snowfork team:
This seems to be the relevant code: https://github.com/Snowfork/snowbridge/blob/1e18e010331777042aa7e8fff3c118094af856ba/relayer/cmd/parachain_head_proof.go#L95-L120
Is it preferred to iter() in some random order as suggested in offchain XCMP: Merkle tree of para headers in beefy ignores Coretime chains #4737 (comment) or take lowest para ids instead as implemented here currently?
PRDoc

Updating Polkadot and Kusama runtimes:

New weights need to be generated (pallet_mmr) and configs updated similar to Rococo/Westend:

diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs
index 5adffbd7422..c7da339b981 100644
--- a/polkadot/runtime/rococo/src/lib.rs
+++ b/polkadot/runtime/rococo/src/lib.rs
@@ -1307,9 +1307,11 @@ impl pallet_mmr::Config for Runtime {
        const INDEXING_PREFIX: &'static [u8] = mmr::INDEXING_PREFIX;
        type Hashing = Keccak256;
        type OnNewRoot = pallet_beefy_mmr::DepositBeefyDigest<Runtime>;
-       type WeightInfo = ();
        type LeafData = pallet_beefy_mmr::Pallet<Runtime>;
        type BlockHashProvider = pallet_mmr::DefaultBlockHashProvider<Runtime>;
+       type WeightInfo = weights::pallet_mmr::WeightInfo<Runtime>;
+       #[cfg(feature = "runtime-benchmarks")]
+       type BenchmarkHelper = parachains_paras::benchmarking::mmr_setup::MmrSetup<Runtime>;
 }

 parameter_types! {
@@ -1319,13 +1321,8 @@ parameter_types! {
 pub struct ParaHeadsRootProvider;
 impl BeefyDataProvider<H256> for ParaHeadsRootProvider {
        fn extra_data() -> H256 {
-               let mut para_heads: Vec<(u32, Vec<u8>)> = parachains_paras::Parachains::<Runtime>::get()
-                       .into_iter()
-                       .filter_map(|id| {
-                               parachains_paras::Heads::<Runtime>::get(&id).map(|head| (id.into(), head.0))
-                       })
-                       .collect();
-               para_heads.sort();
+               let para_heads: Vec<(u32, Vec<u8>)> =
+                       parachains_paras::Pallet::<Runtime>::sorted_para_heads();
                binary_merkle_tree::merkle_root::<mmr::Hashing, _>(
                        para_heads.into_iter().map(|pair| pair.encode()),
                )
@@ -1746,6 +1743,7 @@ mod benches {
                [pallet_identity, Identity]
                [pallet_indices, Indices]
                [pallet_message_queue, MessageQueue]
+               [pallet_mmr, Mmr]
                [pallet_multisig, Multisig]
                [pallet_parameters, Parameters]
                [pallet_preimage, Preimage]

polkadot/runtime/parachains/src/paras/mod.rs

vgeddes · 2024-06-10T19:15:46Z

polkadot/runtime/parachains/src/paras/mod.rs

+ let mut heads: Vec<(u32, Vec<u8>)> =
+ Heads::<T>::iter().map(|(id, head)| (id.into(), head.0)).collect();
+ heads.sort_by_key(|(id, _)| *id);
+ heads.truncate(MAX_PARA_HEADS);


Works for Snowbridge, since BridgeHub with ID 1002 will be sorted before all the other non-system parachains 👍

Ideally we would truncate before sorting. Although it sounds unlikely that someone registers so many parachains that sorting becomes a real problem. Nice thing about sorting first is that system chains will be preferred.

eskimor

Looks good! Thanks @ordian ! I would have been even more conservative by not even iterating more heads than the limit, but realistically your approach should be safe enough and is even less problematic when the limit is hit.

eskimor · 2024-06-10T19:23:49Z

polkadot/runtime/parachains/src/paras/mod.rs

+ let mut heads: Vec<(u32, Vec<u8>)> =
+ Heads::<T>::iter().map(|(id, head)| (id.into(), head.0)).collect();
+ heads.sort_by_key(|(id, _)| *id);
+ heads.truncate(MAX_PARA_HEADS);


Ideally we would truncate before sorting. Although it sounds unlikely that someone registers so many parachains that sorting becomes a real problem. Nice thing about sorting first is that system chains will be preferred.

acatangiu

Code changes look good! 👍

Regarding benchmarking, the extra_data() is computed on_initialize of each block in pallet-mmr:

polkadot-sdk/substrate/frame/merkle-mountain-range/src/lib.rs

Line 232 in f4b73bd

let data = T::LeafData::leaf_data();

It is currently using pallet "static" weight:

polkadot-sdk/substrate/frame/merkle-mountain-range/src/default_weights.rs

Line 27 in 9e56e1a

fn on_initialize(peaks: u64) -> Weight {

configured here: https://github.com/polkadot-fellows/runtimes/blob/0185f7c1740a3b6edb7f8dbb6be541cfcb26ace6/relay/polkadot/src/lib.rs#L360

To include the parachain heads processing cost too, it'd have to be covered by runtime-specific benchmarking (using a high number of registered parachains)

polkadot/runtime/parachains/src/paras/mod.rs

Co-authored-by: Adrian Catangiu <adrian@parity.io>

* master: (120 commits) network/tx: Ban peers with tx that fail to decode (#5002) Try State Hook for Bounties (#4563) [statement-distribution] Add metrics for distributed statements in V2 (#4554) added sync command (#4818) Bridges V2 refactoring backport and `pallet_bridge_messages` simplifications (#4935) xcm-executor: Improve logging (#4996) Remove usage of `sp-std` on templates (#5001) fixed cmd bot commenting not working (#5000) Explain usage of `<T: Config>` in FRAME storage + Update parachain pallet template (#4941) Expose metadata-hash feature from polkadot crate (#4886) Add `MAX_INSTRUCTIONS_TO_DECODE` to XCMv2 (#4978) add notices to the implementer's guide docs that changed for elastic scaling (#4983) `polkadot-parachain` simplifications and deduplications (#4916) Update Templates README docs (#4980) allow clear_origin in safe xcm builder (#4777) litep2p/peerstore: Fix bump last updated time (#4971) Make `tracing::log` work in the runtime (#4863) sp-core: Improve docs generated by `generate_feature_enabled_macro` (#4968) [Backport] Version bumps and prdocs reordering from 1.14.0 (#4955) Assets: can_decrease/increase for destroying asset is not successful (#3286) ...

…ritytech/polkadot-sdk into ao-paraheadrootsprovider-coretime-fix * 'ao-paraheadrootsprovider-coretime-fix' of github.com:paritytech/polkadot-sdk: Update polkadot/runtime/parachains/src/paras/mod.rs

ordian · 2024-07-15T12:56:36Z

bot bench help

ordian · 2024-07-15T12:57:21Z

bot bench substrate-pallet --pallet=pallet_mmr --runtime=westend
bot bench substrate-pallet --pallet=pallet_mmr --runtime=rococo
bot clean

ordian · 2024-07-15T13:00:20Z

bot bench polkadot-pallet --pallet=pallet_mmr --runtime=westend

ordian · 2024-07-16T11:59:34Z

bot bench polkadot-pallet --pallet=pallet_mmr --runtime=westend
bot bench polkadot-pallet --pallet=pallet_mmr --runtime=rococo
bot clean

ordian · 2024-07-17T10:31:32Z

bot bench polkadot-pallet --pallet=pallet_mmr --runtime=westend
bot bench polkadot-pallet --pallet=pallet_mmr --runtime=rococo
bot clean

command-bot · 2024-07-17T10:31:59Z

@ordian Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=pallet_mmr has finished. Result:

HttpError: Not Found
HttpError: Not Found
    at /app/node_modules/@octokit/request/dist-node/index.js:86:21
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async sendRequestWithRetries (/app/node_modules/octokit-auth-probot/node_modules/@octokit/auth-app/dist-node/index.js:466:12)
    at async Job.doExecute (/app/node_modules/bottleneck/light.js:405:18)

command-bot · 2024-07-17T10:50:02Z

@ordian Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=westend --target_dir=polkadot --pallet=pallet_mmr has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6718417 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6718417/artifacts/download.

* master: add elastic scaling MVP guide (#4663) Send PeerViewChange with high priority (#4755) [ci] Update forklift in CI image (#5032) Adjust base value for statement-distribution regression tests (#5028) [pallet_contracts] Add support for transient storage in contracts host functions (#4566) [1 / 5] Optimize logic for gossiping assignments (#4848) Remove `pallet-getter` usage from pallet-session (#4972) command-action: added scoped permissions to the github tokens (#5016) net/litep2p: Propagate ValuePut events to the network backend (#5018) rpc: add back rpc logger (#4952) Updated substrate-relay version for tests (#5017) Remove most all usage of `sp-std` (#5010) Use sp_runtime::traits::BadOrigin (#5011)

acatangiu · 2024-07-17T11:05:10Z

Benchmarking looks good! 👍

ordian · 2024-07-17T11:07:36Z

substrate/frame/merkle-mountain-range/src/benchmarking.rs

+
+ <<T as pallet::Config::<I>>::BenchmarkHelper as BenchmarkHelper>::setup();
+ for leaf in 0..(leaves - 1) {
+ Pallet::<T, I>::on_initialize((leaf as u32).into());
 }
+ }: {
+ Pallet::<T, I>::on_initialize((leaves as u32 - 1).into());


it was benchmarking leaves * on_initialize previously instead of just one iteration of on_initialize

umbrella/Cargo.toml

paritytech-cicd-pr · 2024-07-17T14:45:35Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6723623

This reverts commit 94d1a48.

ordian · 2024-07-18T09:00:04Z

Looks like merge is blocked on 1 more approval.

acatangiu · 2024-07-18T09:01:23Z

ping @eskimor

eskimor

Weight looks off. Rest looks good.

eskimor · 2024-07-19T16:04:10Z

polkadot/runtime/rococo/src/weights/pallet_mmr.rs

+ // Measured: `2140817`
+ // Estimated: `7213082`
+ // Minimum execution time: 20_387_000_000 picoseconds.
+ Weight::from_parts(223_625_477_528, 0)


Do I read this right as 223ms? This seems excessive 😨

IIRC, these number are old before I fixed the bench to measure only on_initialize instead of leaves * on_initialize. Also with testnet profile.

need to rerun the bench before the release, but that is done as part of the release AFAIK.

eskimor · 2024-07-19T16:05:19Z

polkadot/runtime/westend/src/weights/pallet_mmr.rs

+ // Measured: `1071043 + x * (39 ±0)`
+ // Estimated: `3608787 + x * (39 ±6)`
+ // Minimum execution time: 11_102_000_000 picoseconds.
+ Weight::from_parts(21_772_042_215, 0)


Here it is "only" 20. Still a lot, but 10x better than 200.

@acatangiu

Short-term addresses paritytech#4737. - [x] Resolve benchmarking I've digged into benchmarking mentioned paritytech#4737 (comment), but it seemed to me that this code is different proof/path. @acatangiu could you confirm? (btw, in this [bench](https://github.com/paritytech/polkadot-sdk/blob/b65313e81465dd730e48d4ce00deb76922618375/bridges/modules/parachains/src/benchmarking.rs#L57), where do you actually set the `fn parachains()` to a reasonable number? i've only seen 1) - [ ] Communicate to Snowfork team: This seems to be the relevant code: https://github.com/Snowfork/snowbridge/blob/1e18e010331777042aa7e8fff3c118094af856ba/relayer/cmd/parachain_head_proof.go#L95-L120 - [x] Is it preferred to iter() in some random order as suggested in paritytech#4737 (comment) or take lowest para ids instead as implemented here currently? - [x] PRDoc ## Updating Polkadot and Kusama runtimes: New weights need to be generated (`pallet_mmr`) and configs updated similar to Rococo/Westend: ```patch diff --git a/polkadot/runtime/rococo/src/lib.rs b/polkadot/runtime/rococo/src/lib.rs index 5adffbd7422..c7da339b981 100644 --- a/polkadot/runtime/rococo/src/lib.rs +++ b/polkadot/runtime/rococo/src/lib.rs @@ -1307,9 +1307,11 @@ impl pallet_mmr::Config for Runtime { const INDEXING_PREFIX: &'static [u8] = mmr::INDEXING_PREFIX; type Hashing = Keccak256; type OnNewRoot = pallet_beefy_mmr::DepositBeefyDigest<Runtime>; - type WeightInfo = (); type LeafData = pallet_beefy_mmr::Pallet<Runtime>; type BlockHashProvider = pallet_mmr::DefaultBlockHashProvider<Runtime>; + type WeightInfo = weights::pallet_mmr::WeightInfo<Runtime>; + #[cfg(feature = "runtime-benchmarks")] + type BenchmarkHelper = parachains_paras::benchmarking::mmr_setup::MmrSetup<Runtime>; } parameter_types! { @@ -1319,13 +1321,8 @@ parameter_types! { pub struct ParaHeadsRootProvider; impl BeefyDataProvider<H256> for ParaHeadsRootProvider { fn extra_data() -> H256 { - let mut para_heads: Vec<(u32, Vec<u8>)> = parachains_paras::Parachains::<Runtime>::get() - .into_iter() - .filter_map(|id| { - parachains_paras::Heads::<Runtime>::get(&id).map(|head| (id.into(), head.0)) - }) - .collect(); - para_heads.sort(); + let para_heads: Vec<(u32, Vec<u8>)> = + parachains_paras::Pallet::<Runtime>::sorted_para_heads(); binary_merkle_tree::merkle_root::<mmr::Hashing, _>( para_heads.into_iter().map(|pair| pair.encode()), ) @@ -1746,6 +1743,7 @@ mod benches { [pallet_identity, Identity] [pallet_indices, Indices] [pallet_message_queue, MessageQueue] + [pallet_mmr, Mmr] [pallet_multisig, Multisig] [pallet_parameters, Parameters] [pallet_preimage, Preimage] ``` --------- Co-authored-by: Adrian Catangiu <adrian@parity.io>

beefy: put not only lease parachain heads into mmr

1a49d4b

ordian added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Jun 10, 2024

ordian mentioned this pull request Jun 10, 2024

offchain XCMP: Merkle tree of para headers in beefy ignores Coretime chains #4737

Open

ordian added 2 commits June 10, 2024 18:43

fmt

4751d35

prdoc

c8d3742

vgeddes reviewed Jun 10, 2024

View reviewed changes

polkadot/runtime/parachains/src/paras/mod.rs Show resolved Hide resolved

vgeddes reviewed Jun 10, 2024

View reviewed changes

eskimor reviewed Jun 10, 2024

View reviewed changes

ordian and others added 4 commits June 11, 2024 10:05

fix the comment about snowbridge

96a8f75

prdoc: add westend-runtime and rococo-runtime

d6dcb4f

fix rustdocs

c6f3512

Merge branch 'master' into ao-paraheadrootsprovider-coretime-fix

003b59e

ordian marked this pull request as ready for review June 13, 2024 07:29

vgeddes approved these changes Jun 15, 2024

View reviewed changes

acatangiu approved these changes Jun 17, 2024

View reviewed changes

polkadot/runtime/parachains/src/paras/mod.rs Outdated Show resolved Hide resolved

ordian and others added 5 commits June 19, 2024 14:53

Update polkadot/runtime/parachains/src/paras/mod.rs

4036088

Co-authored-by: Adrian Catangiu <adrian@parity.io>

Merge branch 'master' into ao-paraheadrootsprovider-coretime-fix

cb69240

add benchmarking setup trait for mmr pallet

45f8fe5

Merge branch 'ao-paraheadrootsprovider-coretime-fix' of github.com:pa…

4b5fd3e

…ritytech/polkadot-sdk into ao-paraheadrootsprovider-coretime-fix * 'ao-paraheadrootsprovider-coretime-fix' of github.com:paritytech/polkadot-sdk: Update polkadot/runtime/parachains/src/paras/mod.rs

some benchmark fixes

d123500

command-bot bot deleted a comment from github-actions bot Jul 16, 2024

commit uncommitted fixes

f16916f

command-bot bot deleted a comment from github-actions bot Jul 17, 2024

ordian added 2 commits July 17, 2024 13:01

use sp_std::vec

13fd33c

acatangiu approved these changes Jul 17, 2024

View reviewed changes

ordian commented Jul 17, 2024

View reviewed changes

umbrella/Cargo.toml Outdated Show resolved Hide resolved

ordian added 4 commits July 17, 2024 13:28

ughh

ffde0ae

try fixing prdoc and feature propagation check

edceea6

regen umbrella toml

aaa5014

try another time..

659cbb4

ordian added 2 commits July 17, 2024 17:34

try removing unused dep

94d1a48

Revert "try removing unused dep"

a7d55ca

This reverts commit 94d1a48.

ordian enabled auto-merge July 17, 2024 15:46

ordian requested a review from eskimor July 18, 2024 08:59

acatangiu requested a review from serban300 July 18, 2024 09:01

eskimor approved these changes Jul 19, 2024

View reviewed changes

ordian added this pull request to the merge queue Jul 19, 2024

Merged via the queue into master with commit 7f2a99f Jul 19, 2024
160 of 164 checks passed

ordian deleted the ao-paraheadrootsprovider-coretime-fix branch July 19, 2024 16:34

ggwpez mentioned this pull request Sep 9, 2024

Parent issue for stable2409 LTS release #5583

Open

alistair-singh mentioned this pull request Sep 13, 2024

Add Para threads to MMR Root Snowfork/snowbridge#1288

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

beefy: put not only lease parachain heads into mmr #4751

beefy: put not only lease parachain heads into mmr #4751

ordian commented Jun 10, 2024 •

edited

Loading

vgeddes Jun 10, 2024

eskimor Jun 10, 2024

eskimor left a comment •

edited

Loading

eskimor Jun 10, 2024

acatangiu left a comment

ordian commented Jul 15, 2024

ordian commented Jul 15, 2024

ordian commented Jul 15, 2024

ordian commented Jul 16, 2024

ordian commented Jul 17, 2024

command-bot bot commented Jul 17, 2024

command-bot bot commented Jul 17, 2024

acatangiu commented Jul 17, 2024

ordian Jul 17, 2024

paritytech-cicd-pr commented Jul 17, 2024

ordian commented Jul 18, 2024

acatangiu commented Jul 18, 2024

eskimor left a comment

eskimor Jul 19, 2024

ordian Jul 21, 2024

eskimor Jul 19, 2024

beefy: put not only lease parachain heads into mmr #4751

beefy: put not only lease parachain heads into mmr #4751

Conversation

ordian commented Jun 10, 2024 • edited Loading

Updating Polkadot and Kusama runtimes:

vgeddes Jun 10, 2024

Choose a reason for hiding this comment

eskimor Jun 10, 2024

Choose a reason for hiding this comment

eskimor left a comment • edited Loading

Choose a reason for hiding this comment

eskimor Jun 10, 2024

Choose a reason for hiding this comment

acatangiu left a comment

Choose a reason for hiding this comment

ordian commented Jul 15, 2024

ordian commented Jul 15, 2024

ordian commented Jul 15, 2024

ordian commented Jul 16, 2024

ordian commented Jul 17, 2024

command-bot bot commented Jul 17, 2024

command-bot bot commented Jul 17, 2024

acatangiu commented Jul 17, 2024

ordian Jul 17, 2024

Choose a reason for hiding this comment

paritytech-cicd-pr commented Jul 17, 2024

ordian commented Jul 18, 2024

acatangiu commented Jul 18, 2024

eskimor left a comment

Choose a reason for hiding this comment

eskimor Jul 19, 2024

Choose a reason for hiding this comment

ordian Jul 21, 2024

Choose a reason for hiding this comment

eskimor Jul 19, 2024

Choose a reason for hiding this comment

ordian commented Jun 10, 2024 •

edited

Loading

eskimor left a comment •

edited

Loading