Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

allow try-runtime and TestExternalities to report PoV size #10372

Merged
merged 17 commits into from
Dec 4, 2021

Conversation

kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Nov 25, 2021

This PR extracts a few features from #10073, cleans them up, and exposes them so that both a normal TestExternality and try-runtime can use them.

Namely, we now allow:

  • RemoteExternalities to fetch the child-tree as well.
  • Any TestExternalities to report its PoV size as well. An example test is provided of how this can be examined. Most often you are interested in zstd compressed version of the compact proof.
  • Both of the above is now exposed to try-runtime subcommands, so theoretically you can thoroughly test a new runtime, and its migrations, and ensure that their PoV size is sane, using a combination of on-runtime-upgrade and follow-chain.

Next to paritytech/cumulus#793. this will be useful for parachain teams and making sure they perform migrations safely.

My only grumble here is that each team needs to integrate try-runtime into their own CLI. Although this is super easy and they just need to wrap one Command implementation of frame_try_runtime_cli, I would have been happier if I could make a standalone tool for this that teams can just work with, only using a .wasm runtime. In my next life, perhaps.

skip check-dependent-polkadot


Examples:

Running Polkadot Migrations using Try-runtime

2021-11-26 09:21:41 ⚠️ System declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`
2021-11-26 09:21:41 ⚠️ XcmPallet declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`
2021-11-26 09:21:41 ✅ no migration for Crowdloan
2021-11-26 09:21:41 ✅ no migration for Auctions
2021-11-26 09:21:41 ✅ no migration for Slots
2021-11-26 09:21:41 ✅ no migration for Registrar
2021-11-26 09:21:41 ✅ no migration for ParaSessionInfo
2021-11-26 09:21:41 ✅ no migration for Hrmp
2021-11-26 09:21:41 ✅ no migration for Ump
2021-11-26 09:21:41 ✅ no migration for Dmp
2021-11-26 09:21:41 ✅ no migration for Initializer
2021-11-26 09:21:41 ✅ no migration for Paras
2021-11-26 09:21:41 ✅ no migration for ParaScheduler
2021-11-26 09:21:41 ✅ no migration for ParaInherent
2021-11-26 09:21:41 ✅ no migration for ParaInclusion
2021-11-26 09:21:41 ✅ no migration for ParasShared
2021-11-26 09:21:41 ⚠️ Configuration declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
2021-11-26 09:21:41 ✅ no migration for ParachainsOrigin
2021-11-26 09:21:41 ✅ no migration for BagsList
2021-11-26 09:21:41 ✅ no migration for ElectionProviderMultiPhase
2021-11-26 09:21:41 ✅ no migration for Tips
2021-11-26 09:21:41 ✅ no migration for Bounties
2021-11-26 09:21:41 ✅ no migration for Multisig
2021-11-26 09:21:41 ✅ no migration for Proxy
2021-11-26 09:21:41 ✅ no migration for Identity
2021-11-26 09:21:41 ✅ no migration for Utility
2021-11-26 09:21:41 ⚠️ Vesting declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`
2021-11-26 09:21:41 ✅ no migration for Claims
2021-11-26 09:21:41 ✅ no migration for Treasury
2021-11-26 09:21:41 ✅ no migration for TechnicalMembership
2021-11-26 09:21:41 ✅ no migration for PhragmenElection
2021-11-26 09:21:41 ✅ no migration for TechnicalCommittee
2021-11-26 09:21:41 ✅ no migration for Council
2021-11-26 09:21:41 ✅ no migration for Democracy
2021-11-26 09:21:41 ✅ no migration for AuthorityDiscovery
2021-11-26 09:21:41 ✅ no migration for ImOnline
2021-11-26 09:21:41 ✅ no migration for Grandpa
2021-11-26 09:21:41 ✅ no migration for Session
2021-11-26 09:21:41 ✅ no migration for Historical
2021-11-26 09:21:41 ⚠️ Offences declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`
2021-11-26 09:21:41 have 0 deferred offences, applying.
2021-11-26 09:21:41 ⚠️ Staking declares internal migrations (which *might* execute). On-chain `StorageVersion(0)` vs current storage version `StorageVersion(0)`
2021-11-26 09:21:41 ✅ no migration for Authorship
2021-11-26 09:21:41 ✅ no migration for TransactionPayment
2021-11-26 09:21:41 ✅ no migration for Balances
2021-11-26 09:21:41 ✅ no migration for Indices
2021-11-26 09:21:41 ✅ no migration for Timestamp
2021-11-26 09:21:41 ✅ no migration for Babe
2021-11-26 09:21:41 ✅ no migration for Scheduler
2021-11-26 09:21:41 [7868709] 💸 👜 staking bags-list migration passes POST migrate checks ✅
2021-11-26 09:21:41 post-migration prefix of storage 'HistoricalSessions': 'Session' ==> 'Historical'
2021-11-26 09:21:41 post-migration prefix of storage 'StoredRange': 'Session' ==> 'Historical'
2021-11-26 09:21:42 proof: 80082a80ceca95881c4d1b4d3062e46e7ef26fe20d335e5828b2ca31c0ee1b941bedecc78016aa65993743068350ab64bfc9131b4e74fffbd97017ac8bd592ef542a58a08980f62cf7d5892ab918ffa81035016c74781de9022d370dfb3153574cb730027d7e80632feadd4dc9cf7c7e463ac253e615e7307b4c713740cb4c76aaa5cf84b667cd808daf8024d16b10e4ca1736a51224af066f3f47d977ae424688f78272af274a945537828068a621613de99e271a2d992bac31bda903b7ceb5b5f55e2d4a232e9472e4c4b580ed41893dda127c45d10124a971165a9446285ee26700076a483b7cffeafffb8b80ab870ca4d87364bbeaf953c1219ac78db123d3fd653b2660cab5e2e9ff96c8a2807a5f280aa7d057d6cf01fdd5047bb77ce7de8434068f4b4c7991327bcb87a2aa80e9aff8e3472b974fcdc8d790418aa9eb7ce6de8272f8eac2b07dd4ff8144fe4580dccfe9ab40e51531bb27706bf18b0260e048d6e67e724bf1d4361ab8c34d9a8c805be4dea2b3fa74d84724c468c0bfe8a85737158bbd159ec8fb28921839c35ac5805a8136adc9158ea773acadb1444120cf838c58b5d57697dc44fcf256ff4d10f180278ce9ecdb4277cda35bdec920087e7772479790233138d3a2102ddd7e4c20927f0d9ddaac822e776ae365349612c0c4f084cf841e5c65cf4db263f87153b516b927d8d067801b35806ae365...8db6329464a45d1cf0de782e8bad3c663be60812f0a2ac63464f5da3ec448c73334c07d71ef27f2c2cc16da9d1f7271475075aa8eb5c6667714426b8c41dbecf92bdedfa462b716315020000007f1c02053f0aa336633c12e81a8d45742c96715ae1a38758d7272ac9898569d512175c1c66041c477995207f6583c77f05062c96715ae1a38758d7272ac9898569d512175c1c66041c477995207f6583c77f0b002c3ea777080b002c3ea77708005101c9000000ca000000cb000000cc000000cd000000ce000000cf000000d0000000d1000000d2000000d3000000d4000000d5000000d6000000d7000000d8000000d9000000da000000db000000dc000000dd000000de000000df000000e0000000e1000000e2000000e3000000e4000000e5000000e6000000e7000000e8000000e9000000ea000000eb000000ec000000ed000000ee000000ef000000f0000000f1000000f2000000f3000000f4000000f5000000f6000000f7000000f8000000f9000000fa000000fb000000fc000000fd000000fe000000ff000000000100000101000002010000030100000401000005010000060100000701000008010000090100000a0100000b0100000c0100000d0100000e0100000f010000100100001101000012010000130100001401000015010000160100001701000018010000190100001a0100001b0100001c010000 / 81460 nodes
2021-11-26 09:21:42 proof size: 20.24 MB (20728.7021484375 KB) (21226191 bytes)
2021-11-26 09:21:42 compact proof size: 17.74 MB (18164.78125 KB) (18600736 bytes)
2021-11-26 09:21:42 zstd-compressed compact proof 5.70 MB (5833.6748046875 KB) (5973683 bytes)
2021-11-26 09:21:42 TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 4000275000000, total weight = 2000000000000 (2.0001375)

Generic Test: Measuring the PoV of a full runtime upgrade block

// you def. need the runtime, to get the type of the block and all the transactions etc.
use xxx_runtime::*;
// fetch the latest state of your network of choice

use ext = remote_externalities::builder() ... 

//build a block with the format of that chain that only contains a runtime upgrade tx
let block = {
   let update_tx = Call::Sudo(System::SetCode(<new_code>))
   Block { extrinsics: vec![update_tx], ... } 
}

// executing this should automatically trigger both the migration, and the code change, and anything else that you put in the fake block
let (outcome, proof) = ext.execute_and_prove(|| {
  Executive::execute_block(block); 
});

// measure proof size.

This can be either a test in your top-level runtime, or it can simply be a standalone binary that pulls in the correct dependencies.

@kianenigma kianenigma added A0-please_review Pull request needs code review. B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Nov 25, 2021
@github-actions github-actions bot added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Nov 25, 2021
@kianenigma kianenigma changed the title allow try-runtime and test-externalities to report proof size allow try-runtime and test-externalities to report PoV size Nov 25, 2021
let proof = proving_ext.backend.extract_proof();

// ensure that all changes are propagated, and the recorded is clean.
proving_ext.backend.clear_recorder();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per @cheme's previous feedback, the clear recorded is not really needed here. Will double check and remove.

@kianenigma kianenigma added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 26, 2021
@kianenigma kianenigma marked this pull request as ready for review November 26, 2021 08:05
@@ -348,4 +393,32 @@ mod tests {
ext.commit_all().unwrap();
assert!(ext.backend.eq(&backend), "Both backend should be equal.");
}

#[test]
fn execute_and_generate_proof_works() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make this test such that I would make two exts with the same initial data, read 1 key from one and 3 keys from the other one, and expect the proof of the latter to be bigger than the former, but this failed. Any idea why @cheme?

Copy link
Contributor

@cheme cheme Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If values are small, they can be encoded in a same node (inline encoding) and the proof will be the same length.
Using values of length 32 or more is better for testing this kind of things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a bad test.

Convert the proof back to a db and check that it contains the expected values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the backing memory db contains the same data trie nodes of proof, and thei hash, not sure what is there to check. If you want me to improve the test, doing in on the proof itself is enough.

I am looking into checking the proof itself, but it sounds a bit like an overkill. It is not the scope of this test to make sure the proof recorded is doing its job well, just that something is recorded.

Inserting large values (32+ bytes) does help indeed.

Copy link
Contributor Author

@kianenigma kianenigma Nov 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking a bit into example code, I presume making sure that the proof (as memory-db or as-is) contains the root is a reasonable check to do, aka using create_proof_check_backend.

primitives/state-machine/src/ext.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/testing.rs Outdated Show resolved Hide resolved
primitives/storage/src/lib.rs Show resolved Hide resolved
primitives/storage/src/lib.rs Show resolved Hide resolved
utils/frame/remote-externalities/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Show resolved Hide resolved
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
assert!(to_delete.len() > 0);

for d in to_delete {
use std::os::unix::fs::MetadataExt;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens with this test on non-unix systems? :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my problem, these tests don't run on CI either, they are only for me, and I run unix :D

primitives/state-machine/src/testing.rs Outdated Show resolved Hide resolved
primitives/state-machine/src/testing.rs Outdated Show resolved Hide resolved
@@ -348,4 +393,32 @@ mod tests {
ext.commit_all().unwrap();
assert!(ext.backend.eq(&backend), "Both backend should be equal.");
}

#[test]
fn execute_and_generate_proof_works() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a bad test.

Convert the proof back to a db and check that it contains the expected values.

utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/try-runtime/cli/src/lib.rs Outdated Show resolved Hide resolved
utils/frame/remote-externalities/src/lib.rs Outdated Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

@bkchr @cheme thanks for the feedback. your comments are all addressed.

@@ -1019,4 +1023,47 @@ mod tests {
panic!("Hey, I'm an error");
});
}

#[test]
fn execute_and_generate_proof_works() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a different crate would make more sense, but I am ok with this one.

@kianenigma kianenigma changed the title allow try-runtime and test-externalities to report PoV size allow try-runtime and TestExternalities to report PoV size Dec 1, 2021
@stechu
Copy link
Contributor

stechu commented Dec 2, 2021

@bkchr @kianenigma what is the "reasonable" PoV size range in your view?

@kianenigma
Copy link
Contributor Author

@bkchr @kianenigma what is the "reasonable" PoV size range in your view?

It depend on how much storage is being read. I don't think there's any reasonable range.

log::info!(
target: LOG_TARGET,
"proof: {} / {} nodes",
HexDisplay::from(&proof_nodes.iter().flatten().cloned().collect::<Vec<_>>()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that to much to be displayed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Display for HexDisplay trims stuff relatively nicely.

);

let mut child_kv = vec![];
for prefixed_top_key in child_bearing_top_keys {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got really confused, why don't you just call the child storage roots? :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed this instance. Generally speaking, the jargon here is super confusing. Kinda hard to specify what node is in what tree. I though child_bearing_top_keys is pretty accurate actually.


info!(target: LOG_TARGET, "injecting a total of {} top keys", top_kv.len());
for (k, v) in top_kv {
// skip writing the child root data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

@kianenigma kianenigma Dec 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it is computed (re)automatically.

primitives/state-machine/src/testing.rs Show resolved Hide resolved
@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Head SHA changed from c519d98 to ee94f32

@kianenigma
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 6d09a45 into master Dec 4, 2021
@paritytech-processbot paritytech-processbot bot deleted the kiz-pov-test-ext branch December 4, 2021 06:11
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…ech#10372)

* allow try-runtime and test-externalities to report proof size

* self review

* fix test

* Fix humanized dispaly of bytes

* Fix some test

* Fix some review grumbles

* last of the review comments

* fmt

* remove unused import

* move test

* fix import

* Update primitives/state-machine/src/testing.rs

Co-authored-by: Bastian Köcher <[email protected]>

* last touches

* fix

Co-authored-by: Bastian Köcher <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…ech#10372)

* allow try-runtime and test-externalities to report proof size

* self review

* fix test

* Fix humanized dispaly of bytes

* Fix some test

* Fix some review grumbles

* last of the review comments

* fmt

* remove unused import

* move test

* fix import

* Update primitives/state-machine/src/testing.rs

Co-authored-by: Bastian Köcher <[email protected]>

* last touches

* fix

Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants