Is it expected that: state_machine_call_with_proof() does not return a proof when running with option checks_none

simonsso commented 1 year ago

In core/src/commands/on_runtime_upgrade.rs there are two calls to state_machine_call_with_proof() in the later is run without checks UpgradeCheckSelect::None.encode().as_ref(),

Is it expected behavior for the seconds call to return an Empty Trie, when running with pre and post check the proof Trie has the length 45.

This is related to the question I posted yesterday on stackoverflow: https://substrate.stackexchange.com/questions/10351/try-runtime-failed-to-generate-compact-proof-trieerrorinvalidstateroot

simonsso commented 12 months ago

@liamaharon is this a normal behavior, is my data a weird corner case?

liamaharon commented 12 months ago

Nope, that is a corner case bug. Thanks for reporting.

simonsso commented 12 months ago

Files to reproduce can be downloaded from here: https://github.com/simonsso/try-runtime-cli/releases/download/ticket-53/Ticket-53.tar.bz2

Steps to reproduce:

simson@tama:~/work/try-runtime-cli$ cargo run -- --runtime ticket-53/runtime_eden.wasm  on-runtime-upgrade snap -p ticket-53/snap

Wrong file hold on updating

liamaharon commented 12 months ago

Hey @simonsso, I'm actually unable to replicate this on any Polkadot testnet or mainnets.

I wonder if it's an issue with how the snapshot or runtime is being created.

Could you please share how I can compile that runtime file locally and download the snapshot?

simonsso commented 12 months ago

@liamaharon data from https://github.com/simonsso/try-runtime-cli/releases/download/ticket-53/Ticket-53.tar.bz2 (it is updated now)

simson@tama:~/nodle/work/try-runtime-cli$ md5sum ticket-53/runtime_eden.wasm 
244d495549c13910223f5d5a1a2405ef  ticket-53/runtime_eden.wasm

branches Same result on v0.3.4 and main (commit 7f66e57a1bed388622f06fc3ddc9f124d46c7826)

simson@tama:~/nodle/work/try-runtime-cli$ cargo run -- --runtime ticket-53/runtime_eden.wasm  on-runtime-upgrade --checks=none snap -p ticket-53/snap
    Finished dev [unoptimized + debuginfo] target(s) in 0.36s
     Running `target/debug/try-runtime --runtime ticket-53/runtime_eden.wasm on-runtime-upgrade --checks=none snap -p ticket-53/snap`
[2023-11-07T03:56:43Z INFO  remote-ext] Loading snapshot from "ticket-53/snap"
✅ Loaded snapshot (2.08s)
[2023-11-07T03:56:46Z INFO  remote-ext] initialized state externalities with storage root 0x96461e4db4dbe53604e2f67063595d0f201a59e3635828680666f9aec6d6d259 and state_version V0
[2023-11-07T03:56:46Z INFO  try-runtime::cli] Original runtime [Name: RuntimeString::Owned("nodle-para")] [Version: 23] [Code hash: 0xae58...e0f9]
[2023-11-07T03:56:47Z INFO  try-runtime::cli] New runtime      [Name: RuntimeString::Owned("nodle-para")] [Version: 25] [Code hash: 0xdd47...f334]
[2023-11-07T03:56:47Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 Running TryRuntime_on_runtime_upgrade with checks: None
[2023-11-07T03:56:58Z INFO  runtime::frame-support] ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
[2023-11-07T03:56:58Z ERROR try-runtime::cli] failed to generate compact proof: TrieError(InvalidStateRoot(0x868e46de038c23f89652a0763c83831c852728f5d1e42ea98181364ff6c6ce22))
[2023-11-07T03:56:58Z INFO  try-runtime::cli] PoV size (zstd-compressed compact proof): 10 B. For parachains, it's your responsibility to verify that a PoV of this size fits within any relaychain constraints.
[2023-11-07T03:56:58Z INFO  try-runtime::cli] Consumed ref_time: 0s (0.00% of max 0.5s)
[2023-11-07T03:56:58Z INFO  try-runtime::cli] ✅ TryRuntime_on_runtime_upgrade executed without errors or weight safety warnings. Please note this does not guarantee a successful runtime upgrade. Always test your runtime upgrade with recent state, and ensure that the weight usage of your migrations will not drastically differ between testing and actual on-chain execution.

same result on main with this data

Link to the source of the offending build: https://github.com/NodleCode/chain/tree/15ac86030b20b6566414b54f122ae9018e9c7bd5 https://github.com/NodleCode/chain/actions/runs/6701038325

simonsso commented 12 months ago

@liamaharon So I think there is something very fishy I was doing in the build above.

liamaharon commented 12 months ago

Yeah I was unable to reproduce this using try-runtime v0.3.5, a runtime built from the Nodle master branch (dede3e55dd85ef6b1e52ca82d0fce90f99a49e66) and the snapshot provided above.

Seems like a problem with your code downstream?

simonsso commented 12 months ago

@liamaharon did you try with the wasm I shared?

I got this problem while working with the migrations and it is not yet merged on master, I created a branch where I can reproduce it: https://github.com/NodleCode/chain/tree/iso/patch-1

Seems like a problem upstream?

Yes our migrations have been lagging behind, so I want to make this properly now

liamaharon commented 12 months ago

I was able to reproduce with the runtime you provided in the .tar.

I suggest bisecting the commit history to find where things broke.

simonsso commented 12 months ago

Still working on same branch as above: This https://github.com/NodleCode/chain/commit/87ee3fa2664c9c71cb5994db70139acdbffbd01d commit removes the error for me, so what in this input has changed?

simson@tama:~/nodle/chain$ ./scripts/try_runtime.sh 
    Finished release [optimized] target(s) in 0.38s
⚡ Found 3 strongly connected components which includes at least one cycle each
cycle(001) ∈ α: ApprovalVoting ~~{"DisputeCoordinatorMessage"}~~> DisputeCoordinator ~~{"ApprovalVotingMessage"}~~>  *
cycle(002) ∈ β: CandidateBacking ~~{"StatementDistributionMessage"}~~> StatementDistribution ~~{"CandidateBackingMessage"}~~>  *
cycle(003) ∈ γ: NetworkBridgeRx ~~{"GossipSupportMessage"}~~> GossipSupport ~~{"NetworkBridgeRxMessage"}~~>  *
   Compiling runtime-eden v2.3.1 (/home/simson/nodle/chain/runtimes/eden)
warning: unused imports: `AccountId`, `Balance`, `Decode`, `Encode`, `frame_support::Blake2_128Concat`, `pallet_uniques::CollectionDetails`
  --> runtimes/eden/src/migrations.rs:9:10
   |
9  |     codec::{Decode, Encode},
   |             ^^^^^^  ^^^^^^
10 |     frame_support::Blake2_128Concat,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
11 |     pallet_uniques::CollectionDetails,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
12 |     primitives::{AccountId, Balance},
   |                  ^^^^^^^^^  ^^^^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused variable: `state`
  --> runtimes/eden/src/migrations.rs:67:18
   |
67 |     fn post_upgrade(state: Vec<u8>) -> Result<(), TryRuntimeError> {
   |                     ^^^^^ help: if this is intentional, prefix it with an underscore: `_state`
   |
   = note: `#[warn(unused_variables)]` on by default

warning: `runtime-eden` (lib) generated 2 warnings (run `cargo fix --lib -p runtime-eden` to apply 2 suggestions)
   Compiling nodle-parachain v2.3.1 (/home/simson/nodle/chain/node)
    Finished release [optimized] target(s) in 50.49s
[2023-11-07T08:22:58Z INFO  remote-ext] Loading snapshot from "snapshots/eden2"
✅ Loaded snapshot (0.71s)
[2023-11-07T08:22:59Z INFO  remote-ext] initialized state externalities with storage root 0x96461e4db4dbe53604e2f67063595d0f201a59e3635828680666f9aec6d6d259 and state_version V0
[2023-11-07T08:22:59Z INFO  try-runtime::cli] Original runtime [Name: RuntimeString::Owned("nodle-para")] [Version: 23] [Code hash: 0xae58...e0f9]
[2023-11-07T08:22:59Z INFO  try-runtime::cli] New runtime      [Name: RuntimeString::Owned("nodle-para")] [Version: 25] [Code hash: 0x4408...264a]
[2023-11-07T08:23:00Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 Running TryRuntime_on_runtime_upgrade with checks: PreAndPost
[2023-11-07T08:23:01Z INFO  runtime::multisig] [3664149] ✍️ Number of calls to refund and delete: 0
[2023-11-07T08:23:01Z INFO  runtime::frame-support] ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
[2023-11-07T08:23:01Z INFO  try_runtime_core::commands::on_runtime_upgrade] 🔬 TryRuntime_on_runtime_upgrade succeeded! Running it again without checks for weight measurements.
[2023-11-07T08:23:01Z INFO  runtime::frame-support] ⚠️ PolkadotXcm declares internal migrations (which *might* execute). On-chain `StorageVersion(1)` vs current storage version `StorageVersion(1)`
[2023-11-07T08:23:01Z INFO  try-runtime::cli] PoV size (zstd-compressed compact proof): 528 B. For parachains, it's your responsibility to verify that a PoV of this size fits within any relaychain constraints.
[2023-11-07T08:23:01Z INFO  try-runtime::cli] Consumed ref_time: 0s (0.00% of max 0.5s)
[2023-11-07T08:23:01Z INFO  try-runtime::cli] ✅ TryRuntime_on_runtime_upgrade executed without errors or weight safety warnings. Please note this does not guarantee a successful runtime upgrade. Always test your runtime upgrade with recent state, and ensure that the weight usage of your migrations will not drastically differ between testing and actual on-chain execution.

simonsso commented 12 months ago

This NodleCode/chain@87ee3fa commit removes the error for me

@liamaharon was above helpful in understanding this behavior?

liamaharon commented 12 months ago

It might be something to do with the state you're using being incompatible with the new runtime you are compiling.

If you're using an older runtime (it's working on master), maybe try also using an older state snapshot.

liamaharon commented 11 months ago

Hey @simonsso any update here?

simonsso commented 11 months ago

@liamaharon I get other errors now in my integration. I think we can close this investigation and leave it here as a clue if this becomes a problem again in the future

paritytech / try-runtime-cli

Is it expected that: state_machine_call_with_proof() does not return a proof when running with option checks_none #53