Skip to content

Commit 8762d74

Browse files
tmpolaczykggwpezliamaharon
authored
Add flag to see storage diff if idemptotent check fails (#78)
When the idempotent migration check fails, users see this unhelpful error message: ``` ❌ Migrations are not idempotent. Selectively remove migrations from Executive until you find the culprit. ``` This PR implements a flag to enable printing the differences in storage. It is disabled by default because users can ignore the "idempotent" warning if they want to. If the flag is disabled there is a log that suggests enabling it, improving the old message. ![Captura desde 2024-02-20 13-59-48](https://github.com/paritytech/try-runtime-cli/assets/44604217/1ae51887-6d26-411a-8874-23924b5cb536) --------- Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> Co-authored-by: Liam Aharon <liam.aharon@hotmail.com>
1 parent 7abe31b commit 8762d74

4 files changed

Lines changed: 81 additions & 7 deletions

File tree

Cargo.lock

Lines changed: 42 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ resolver = "2"
44
members = ["cli", "core"]
55

66
[workspace.package]
7-
version = "0.5.4"
7+
version = "0.6.0"
88
authors = ["Parity Technologies <admin@parity.io>"]
99
description = "Substrate's programmatic testing framework."
1010
edition = "2021"

core/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ sp-weights = { workspace = true }
5353

5454
substrate-rpc-client = { workspace = true }
5555
paris = "1.5.15"
56+
similar-asserts = "1.5.0"
5657

5758
[dev-dependencies]
5859
assert_cmd = { workspace = true }

core/src/commands/on_runtime_upgrade.rs

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
// See the License for the specific language governing permissions and
1616
// limitations under the License.
1717

18-
use std::{fmt::Debug, str::FromStr};
18+
use std::{collections::BTreeMap, fmt::Debug, str::FromStr};
1919

2020
use bytesize::ByteSize;
2121
use frame_try_runtime::UpgradeCheckSelect;
@@ -24,7 +24,7 @@ use parity_scale_codec::Encode;
2424
use sc_executor::sp_wasm_interface::HostFunctions;
2525
use sp_core::{hexdisplay::HexDisplay, Hasher};
2626
use sp_runtime::traits::{Block as BlockT, HashingFor, NumberFor};
27-
use sp_state_machine::{CompactProof, StorageProof};
27+
use sp_state_machine::{CompactProof, OverlayedChanges, StorageProof};
2828

2929
use crate::{
3030
build_executor,
@@ -69,6 +69,11 @@ pub struct Command {
6969
/// Whether to disable migration idempotency checks
7070
#[clap(long, default_value = "false", default_missing_value = "true")]
7171
pub disable_idempotency_checks: bool,
72+
73+
/// When migrations are detected as not idempotent, enabling this will output a diff of the
74+
/// storage before and after running the same set of migrations the second time.
75+
#[clap(long, default_value = "false", default_missing_value = "true")]
76+
pub print_storage_diff: bool,
7277
}
7378

7479
// Runs the `on-runtime-upgrade` command.
@@ -187,6 +192,7 @@ where
187192
colorize_string("<bold><blue>-------------------------------------------------------------------")
188193
);
189194
let (oc_pre_root, _) = overlayed_changes.storage_root(&ext.backend, ext.state_version);
195+
overlayed_changes.start_transaction();
190196
match state_machine_call_with_proof::<Block, HostFns>(
191197
&ext,
192198
&mut overlayed_changes,
@@ -202,6 +208,21 @@ where
202208
overlayed_changes.storage_root(&ext.backend, ext.state_version);
203209
if oc_pre_root != oc_post_root {
204210
log::error!("❌ Migrations are not idempotent. Selectively remove migrations from Executive until you find the culprit.");
211+
if command.print_storage_diff {
212+
log::info!("Changed storage keys:");
213+
let changes_after =
214+
collect_storage_changes_as_hex::<Block>(&overlayed_changes);
215+
overlayed_changes.rollback_transaction().expect(
216+
"Migrations must not rollback transactions that they did not open",
217+
);
218+
let changes_before =
219+
collect_storage_changes_as_hex::<Block>(&overlayed_changes);
220+
221+
similar_asserts::assert_eq!(changes_before, changes_after);
222+
} else {
223+
log::error!("Run with --print-storage-diff to see list of changed storage keys.");
224+
}
225+
205226
false
206227
} else {
207228
// Exeuction was OK and state root didn't change
@@ -366,3 +387,17 @@ fn analyse_ref_time(ref_time_results: RefTimeInfo, no_weight_warnings: bool) ->
366387
WeightSafety::ProbablySafe
367388
}
368389
}
390+
391+
fn collect_storage_changes_as_hex<Block: BlockT>(
392+
overlayed_changes: &OverlayedChanges<HashingFor<Block>>,
393+
) -> BTreeMap<String, String> {
394+
overlayed_changes
395+
.changes()
396+
.filter_map(|(key, entry)| {
397+
Some((
398+
HexDisplay::from(key).to_string(),
399+
HexDisplay::from(entry.value_ref().as_ref()?).to_string(),
400+
))
401+
})
402+
.collect()
403+
}

0 commit comments

Comments
 (0)