From 34002296463d73519052fc5e34d88f4dcc8b85f9 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Sat, 26 Aug 2023 12:18:21 +0700 Subject: [PATCH] fix(catalyst-toolbox): Fix error when purpose is null. | NPG-0000 (#534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catalyst toolbox was crashing if "purpose" was Null in a registration. This fixes that, so Null is properly detected as a "Catalyst" registration. Also, it adds the pre-calculated Jormungandr chain address, because its non trivial to go from the voting key to the chain address used by Jormungandr. This lets me compare the SVE snapshot with the normal snapshot, and it shows the SVE snapshot has rounding errors which will negatively effect people with multiple registrations on the same key. Recommendation is for Audit, we publish the updated `snapshot` artifact, not the one we import into vit-ss. And then make a quick program to convert the `snapshot` artifact into a format needed for vit-ss for F10. The `utilities/snapshot-check/compare_snapshot.py` would need a couple of tweaks to be such a tool. --------- Co-authored-by: Joaquín Rosales --- containers/event-db-migrations/entry.sh | 2 +- .../src/bin/cli/snapshot/mod.rs | 6 ++ .../catalyst-toolbox/src/rewards/voters.rs | 10 +- src/catalyst-toolbox/snapshot-lib/src/lib.rs | 44 ++++++--- .../snapshot-lib/src/registration.rs | 4 +- .../snapshot-lib/src/voter_hir.rs | 16 ++- .../src/common/raw_snapshot.rs | 4 +- .../src/common/snapshot.rs | 19 +++- .../mainnet-tools/src/snapshot/convert.rs | 2 +- .../vitup/src/mode/mock/snapshot.rs | 30 +++++- utilities/snapshot-check/compare_snapshot.py | 99 +++++++++++++++++++ 11 files changed, 210 insertions(+), 26 deletions(-) create mode 100755 utilities/snapshot-check/compare_snapshot.py diff --git a/containers/event-db-migrations/entry.sh b/containers/event-db-migrations/entry.sh index cd48820fbc..f485904b84 100644 --- a/containers/event-db-migrations/entry.sh +++ b/containers/event-db-migrations/entry.sh @@ -50,7 +50,7 @@ check_env_vars() { # Iterate over the array and check if each variable is set for var in "${env_vars[@]}"; do echo "Checking $var" - if [ -z "${!var}" ]; then + if [ -z "${!var:-}" ]; then echo ">>> Error: $var is required and not set." exit 1 fi diff --git a/src/catalyst-toolbox/catalyst-toolbox/src/bin/cli/snapshot/mod.rs b/src/catalyst-toolbox/catalyst-toolbox/src/bin/cli/snapshot/mod.rs index f34664bb08..c1fb256ec2 100644 --- a/src/catalyst-toolbox/catalyst-toolbox/src/bin/cli/snapshot/mod.rs +++ b/src/catalyst-toolbox/catalyst-toolbox/src/bin/cli/snapshot/mod.rs @@ -1,3 +1,4 @@ +use chain_addr::Discrimination; use clap::Parser; use color_eyre::Report; use jcli_lib::utils::{output_file::OutputFile, output_format::OutputFormat}; @@ -45,6 +46,10 @@ pub struct SnapshotCmd { #[clap(flatten)] output_format: OutputFormat, + + /// Discrimination to use for initial addresses + #[clap(short, long, default_value = "production")] + discrimination: Discrimination, } impl SnapshotCmd { @@ -67,6 +72,7 @@ impl SnapshotCmd { self.min_stake_threshold, self.voting_power_cap, &assigner, + self.discrimination, )? .to_full_snapshot_info(); let mut out_writer = self.output.open()?; diff --git a/src/catalyst-toolbox/catalyst-toolbox/src/rewards/voters.rs b/src/catalyst-toolbox/catalyst-toolbox/src/rewards/voters.rs index 85ca48e0fe..bac76ee04a 100644 --- a/src/catalyst-toolbox/catalyst-toolbox/src/rewards/voters.rs +++ b/src/catalyst-toolbox/catalyst-toolbox/src/rewards/voters.rs @@ -150,6 +150,7 @@ mod tests { DEFAULT_SNAPSHOT_THRESHOLD.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); @@ -182,6 +183,7 @@ mod tests { DEFAULT_SNAPSHOT_THRESHOLD.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); @@ -205,6 +207,7 @@ mod tests { DEFAULT_SNAPSHOT_THRESHOLD.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); @@ -307,7 +310,7 @@ mod tests { voting_power: i.into(), reward_address, delegations, - voting_purpose: 0, + voting_purpose: Some(0), nonce: 0, }); total_stake += i; @@ -318,6 +321,7 @@ mod tests { 0.into(), Fraction::from(1u64), &|_voting_key: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); @@ -355,7 +359,7 @@ mod tests { voting_power: i.into(), reward_address, delegations, - voting_purpose: 0, + voting_purpose: Some(0), nonce: 0, }); } @@ -365,6 +369,7 @@ mod tests { 0.into(), Fraction::new(1u64, 9u64), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); @@ -390,6 +395,7 @@ mod tests { DEFAULT_SNAPSHOT_THRESHOLD.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); diff --git a/src/catalyst-toolbox/snapshot-lib/src/lib.rs b/src/catalyst-toolbox/snapshot-lib/src/lib.rs index e31e1eb6c8..a433d03f8d 100644 --- a/src/catalyst-toolbox/snapshot-lib/src/lib.rs +++ b/src/catalyst-toolbox/snapshot-lib/src/lib.rs @@ -1,3 +1,4 @@ +use chain_addr::Discrimination; pub use fraction::Fraction; use jormungandr_lib::{crypto::account::Identifier, interfaces::Value}; use registration::{ @@ -107,6 +108,7 @@ impl Snapshot { stake_threshold: Value, cap: Fraction, voting_group_assigner: &impl VotingGroupAssigner, + discrimination: Discrimination, ) -> Result { let raw_contribs = raw_snapshot .0 @@ -116,7 +118,10 @@ impl Snapshot { .filter(|reg| reg.voting_power >= std::cmp::max(stake_threshold, 1.into())) // TODO: add capability to select voting purpose for a snapshot. // At the moment Catalyst is the only one in use - .filter(|reg| reg.voting_purpose == CATALYST_VOTING_PURPOSE_TAG) + .filter(|reg| { + reg.voting_purpose.unwrap_or(CATALYST_VOTING_PURPOSE_TAG) + == CATALYST_VOTING_PURPOSE_TAG + }) .fold(BTreeMap::new(), |mut acc: BTreeMap<_, Vec<_>>, reg| { let VotingRegistration { reward_address, @@ -170,7 +175,12 @@ impl Snapshot { .map(|(k, contributions)| SnapshotInfo { hir: VoterHIR { voting_group: voting_group_assigner.assign(&k), - voting_key: k, + voting_key: k.clone(), + address: chain_addr::Address( + discrimination, + chain_addr::Kind::Account(k.to_inner().into()), + ) + .into(), voting_power: contributions.iter().map(|c| c.value).sum::().into(), }, contributions, @@ -278,14 +288,16 @@ pub mod tests { _raw, _stake_threshold.into(), Fraction::from(1u64), - &DummyAssigner + &DummyAssigner, + Discrimination::Production, ) .unwrap() == Snapshot::from_raw_snapshot( add, _stake_threshold.into(), Fraction::from(1u64), - &DummyAssigner + &DummyAssigner, + Discrimination::Production, ) .unwrap(), _additional_reg.voting_power < _stake_threshold.into() @@ -304,6 +316,7 @@ pub mod tests { threshold.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap() }) @@ -319,6 +332,7 @@ pub mod tests { 0.into(), Fraction::from(1), &|_vk: &Identifier| String::new(), + Discrimination::Production, ) .unwrap(); let total_stake = snapshot @@ -331,20 +345,22 @@ pub mod tests { #[proptest] fn test_non_catalyst_regs_are_ignored(mut _reg: VotingRegistration) { - _reg.voting_purpose = 1; + _reg.voting_purpose = Some(1); assert_eq!( Snapshot::from_raw_snapshot( vec![_reg].into(), 0.into(), Fraction::from(1u64), - &DummyAssigner + &DummyAssigner, + Discrimination::Production, ) .unwrap(), Snapshot::from_raw_snapshot( vec![].into(), 0.into(), Fraction::from(1u64), - &DummyAssigner + &DummyAssigner, + Discrimination::Production, ) .unwrap(), ) @@ -367,7 +383,7 @@ pub mod tests { voting_power: i.into(), reward_address: RewardAddress(String::new()), delegations, - voting_purpose: 0, + voting_purpose: Some(0), nonce: 0, }); } @@ -377,6 +393,7 @@ pub mod tests { 0.into(), Fraction::from(1u64), &DummyAssigner, + Discrimination::Production, ) .unwrap(); let vp_1: u64 = snapshot @@ -408,9 +425,14 @@ pub mod tests { } ]"#, ).unwrap(); - let snapshot = - Snapshot::from_raw_snapshot(raw, 0.into(), Fraction::from(1u64), &DummyAssigner) - .unwrap(); + let snapshot = Snapshot::from_raw_snapshot( + raw, + 0.into(), + Fraction::from(1u64), + &DummyAssigner, + Discrimination::Production, + ) + .unwrap(); assert_eq!( snapshot.contributions_for_voting_key( Identifier::from_hex( diff --git a/src/catalyst-toolbox/snapshot-lib/src/registration.rs b/src/catalyst-toolbox/snapshot-lib/src/registration.rs index 4c91157d54..0e4d14e659 100644 --- a/src/catalyst-toolbox/snapshot-lib/src/registration.rs +++ b/src/catalyst-toolbox/snapshot-lib/src/registration.rs @@ -55,7 +55,7 @@ pub struct VotingRegistration { pub delegations: Delegations, /// 0 = Catalyst, assumed 0 for old legacy registrations #[serde(default)] - pub voting_purpose: u64, + pub voting_purpose: Option, #[serde(default)] pub nonce: u64, @@ -341,7 +341,7 @@ mod tests { voting_power, reward_address, delegations, - voting_purpose: 0, + voting_purpose: None, nonce: 0, } }) diff --git a/src/catalyst-toolbox/snapshot-lib/src/voter_hir.rs b/src/catalyst-toolbox/snapshot-lib/src/voter_hir.rs index a0fee42510..0e049c3f5a 100644 --- a/src/catalyst-toolbox/snapshot-lib/src/voter_hir.rs +++ b/src/catalyst-toolbox/snapshot-lib/src/voter_hir.rs @@ -1,5 +1,5 @@ use ::serde::{Deserialize, Serialize}; -use jormungandr_lib::{crypto::account::Identifier, interfaces::Value}; +use jormungandr_lib::{crypto::account::Identifier, interfaces::Address, interfaces::Value}; pub type VotingGroup = String; @@ -17,6 +17,9 @@ pub struct VoterHIR { // Keep hex encoding as in CIP-36 #[serde(with = "serde")] pub voting_key: Identifier, + // Jormungandr chain address + pub address: Address, + /// Voting group this key belongs to. /// If this key belong to multiple voting groups, multiple records for the same /// key will be used. @@ -59,7 +62,18 @@ pub mod tests { fn arbitrary_with(args: Self::Parameters) -> Self::Strategy { (any::<[u8; 32]>(), args.1 .0) .prop_map(move |(key, voting_power)| VoterHIR { + // Two representations of exactly the same key. voting_key: Identifier::from_hex(&hex::encode(key)).unwrap(), + address: chain_addr::Address( + chain_addr::Discrimination::Production, + chain_addr::Kind::Account( + Identifier::from_hex(&hex::encode(key)) + .unwrap() + .to_inner() + .into(), + ), + ) + .into(), voting_power: voting_power.into(), voting_group: args.0.clone(), }) diff --git a/src/vit-servicing-station/vit-servicing-station-tests/src/common/raw_snapshot.rs b/src/vit-servicing-station/vit-servicing-station-tests/src/common/raw_snapshot.rs index 9b180233a4..e1735e3509 100644 --- a/src/vit-servicing-station/vit-servicing-station-tests/src/common/raw_snapshot.rs +++ b/src/vit-servicing-station/vit-servicing-station-tests/src/common/raw_snapshot.rs @@ -1,5 +1,6 @@ use std::convert::TryInto; +use chain_addr::Discrimination; use chain_impl_mockchain::testing::TestGen; use jormungandr_lib::interfaces::Value; use rand::Rng; @@ -60,6 +61,7 @@ impl RawSnapshotExtension for RawSnapshot { self.content.min_stake_threshold, self.content.voting_power_cap, assigner, + Discrimination::Production, )? .to_full_snapshot_info()) } @@ -200,7 +202,7 @@ impl RawSnapshotBuilder { delegation_type_count += 1; Delegations::Legacy(TestGen::identifier().into()) }, - voting_purpose: CATALYST_VOTING_PURPOSE_TAG, + voting_purpose: Some(CATALYST_VOTING_PURPOSE_TAG), nonce: 0, }) }) diff --git a/src/vit-servicing-station/vit-servicing-station-tests/src/common/snapshot.rs b/src/vit-servicing-station/vit-servicing-station-tests/src/common/snapshot.rs index 6802c0fca9..d87665b8ab 100644 --- a/src/vit-servicing-station/vit-servicing-station-tests/src/common/snapshot.rs +++ b/src/vit-servicing-station/vit-servicing-station-tests/src/common/snapshot.rs @@ -100,11 +100,20 @@ impl SnapshotBuilder { }) .take(self.contributions_count) .collect(), - hir: VoterHIR { - voting_key: TestGen::identifier().into(), - voting_group: self.groups[rng.gen_range(0, self.groups.len())] - .to_string(), - voting_power: rng.gen_range(1u64, 1_000u64).into(), + hir: { + let identifier = TestGen::identifier(); + + VoterHIR { + voting_key: identifier.clone().into(), + voting_group: self.groups[rng.gen_range(0, self.groups.len())] + .to_string(), + voting_power: rng.gen_range(1u64, 1_000u64).into(), + address: chain_addr::Address( + chain_addr::Discrimination::Production, + chain_addr::Kind::Account(identifier.into()), + ) + .into(), + } }, }) }) diff --git a/src/vit-testing/mainnet-tools/src/snapshot/convert.rs b/src/vit-testing/mainnet-tools/src/snapshot/convert.rs index ea06149501..eaeb133bfe 100644 --- a/src/vit-testing/mainnet-tools/src/snapshot/convert.rs +++ b/src/vit-testing/mainnet-tools/src/snapshot/convert.rs @@ -130,7 +130,7 @@ impl OutputExtension for SnapshotEntry { VotingDelegations::New(new) } }, - voting_purpose: *self.voting_purpose.unwrap_or(VotingPurpose::CATALYST), + voting_purpose: Some(*self.voting_purpose.unwrap_or(VotingPurpose::CATALYST)), nonce: self.nonce, }) diff --git a/src/vit-testing/vitup/src/mode/mock/snapshot.rs b/src/vit-testing/vitup/src/mode/mock/snapshot.rs index 284c36b41f..ca5b17b909 100644 --- a/src/vit-testing/vitup/src/mode/mock/snapshot.rs +++ b/src/vit-testing/vitup/src/mode/mock/snapshot.rs @@ -1,3 +1,4 @@ +use chain_addr::Discrimination; use jormungandr_lib::crypto::account::Identifier; use mainnet_tools::snapshot::MainnetWalletStateExtension; use proptest::{arbitrary::Arbitrary, prelude::*, strategy::BoxedStrategy}; @@ -35,6 +36,7 @@ impl VoterSnapshot { parameters.min_stake_threshold, parameters.voting_power_cap, &|_vk: &Identifier| String::new(), + Discrimination::Production, )? .to_full_snapshot_info(); @@ -183,20 +185,44 @@ impl Arbitrary for ArbitraryVoterHIR { if let Some(voting_group) = args { any::<([u8; 32], u64)>() .prop_map(move |(key, voting_power)| { + let identifier = Identifier::from_hex(&hex::encode(key)).unwrap(); Self(VoterHIR { - voting_key: Identifier::from_hex(&hex::encode(key)).unwrap(), + voting_key: identifier.clone(), voting_power: voting_power.into(), voting_group: voting_group.clone(), + address: chain_addr::Address( + chain_addr::Discrimination::Production, + chain_addr::Kind::Account( + identifier + .to_address(chain_addr::Discrimination::Production) + .public_key() + .unwrap() + .to_owned(), + ), + ) + .into(), }) }) .boxed() } else { any::<([u8; 32], u64, String)>() .prop_map(|(key, voting_power, voting_group)| { + let identifier = Identifier::from_hex(&hex::encode(key)).unwrap(); Self(VoterHIR { - voting_key: Identifier::from_hex(&hex::encode(key)).unwrap(), + voting_key: identifier.clone(), voting_power: voting_power.into(), voting_group, + address: chain_addr::Address( + chain_addr::Discrimination::Production, + chain_addr::Kind::Account( + identifier + .to_address(chain_addr::Discrimination::Production) + .public_key() + .unwrap() + .to_owned(), + ), + ) + .into(), }) }) .boxed() diff --git a/utilities/snapshot-check/compare_snapshot.py b/utilities/snapshot-check/compare_snapshot.py new file mode 100755 index 0000000000..6e624025ba --- /dev/null +++ b/utilities/snapshot-check/compare_snapshot.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +""" +Simple program to compare snapshots. +""" + +from __future__ import annotations + +import argparse +import sys +import json + +from pathlib import Path +from typing import Any + +def is_dir(dirpath: str | Path): + """Check if the directory is a directory.""" + real_dir = Path(dirpath) + if real_dir.exists() and real_dir.is_dir(): + return real_dir + raise argparse.ArgumentTypeError(f"{dir} is not a directory.") + + +def is_file(filename: str): + """Does the path exist and is it a file""" + real_filename = Path(filename).relative_to(".") + is_dir(real_filename.parent) + if real_filename.is_dir(): + raise argparse.ArgumentTypeError(f"{filename} is not a file.") + return real_filename + +def normalize_snapshot(snapshot) -> dict: + normalized={} + + if isinstance(snapshot, list): + for rec in snapshot: + normalized[rec["hir"]["address"]] = rec["hir"]["voting_power"] + else: + # legacy snapshot + for rec in snapshot["initial"][0]["fund"]: + normalized[rec["address"]] = rec["value"] + + return normalized + + +def analyze_snapshot(args: argparse.Namespace): + """Compare snapshots. ONLY checks if the address and voting power match.""" + snapshot = normalize_snapshot(json.loads(args.snapshot.read_text())) + compare = normalize_snapshot(json.loads(args.compare.read_text())) + + for key in snapshot: + if key not in compare: + print(f"Voter Address not found {key}"); + else: + cmp = compare.pop(key) + snap = snapshot[key] + if cmp == snap: + continue + if (cmp > snap) and int(cmp / 1000000) == snap: + continue + if (cmp < snap) and int(snap/ 1000000) == cmp: + continue + + if cmp > snap: + print(int(cmp / 1000000)) + if snap > cmp: + print(int(snap / 1000000)) + print(f"Voter {key} {snapshot[key]} != {cmp}"); + + for key in compare: + print(f"Comparison Voter Address not found {key}"); + + + +def main() -> int: + """Parse CLI arguments.""" + parser = argparse.ArgumentParser( + description="Compare fully processed snapshots." + ) + parser.add_argument( + "--snapshot", + help="Snapshot file to read.", + required=True, + type=is_file, + ) + + parser.add_argument( + "--compare", + help="Snapshot file to compare with.", + required=False, + type=is_file, + ) + + args = parser.parse_args() + analyze_snapshot(args) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) \ No newline at end of file