Skip to content

Commit

Permalink
fix(catalyst-toolbox): Fix error when purpose is null. | NPG-0000 (#534)
Browse files Browse the repository at this point in the history
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 <joaquin.rosales@iohk.io>
  • Loading branch information
stevenj and saibatizoku authored Aug 26, 2023
1 parent c2f8299 commit 3400229
Show file tree
Hide file tree
Showing 11 changed files with 210 additions and 26 deletions.
2 changes: 1 addition & 1 deletion containers/event-db-migrations/entry.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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 {
Expand All @@ -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()?;
Expand Down
10 changes: 8 additions & 2 deletions src/catalyst-toolbox/catalyst-toolbox/src/rewards/voters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ mod tests {
DEFAULT_SNAPSHOT_THRESHOLD.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand Down Expand Up @@ -182,6 +183,7 @@ mod tests {
DEFAULT_SNAPSHOT_THRESHOLD.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand All @@ -205,6 +207,7 @@ mod tests {
DEFAULT_SNAPSHOT_THRESHOLD.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand Down Expand Up @@ -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;
Expand All @@ -318,6 +321,7 @@ mod tests {
0.into(),
Fraction::from(1u64),
&|_voting_key: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand Down Expand Up @@ -355,7 +359,7 @@ mod tests {
voting_power: i.into(),
reward_address,
delegations,
voting_purpose: 0,
voting_purpose: Some(0),
nonce: 0,
});
}
Expand All @@ -365,6 +369,7 @@ mod tests {
0.into(),
Fraction::new(1u64, 9u64),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand All @@ -390,6 +395,7 @@ mod tests {
DEFAULT_SNAPSHOT_THRESHOLD.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();

Expand Down
44 changes: 33 additions & 11 deletions src/catalyst-toolbox/snapshot-lib/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use chain_addr::Discrimination;
pub use fraction::Fraction;
use jormungandr_lib::{crypto::account::Identifier, interfaces::Value};
use registration::{
Expand Down Expand Up @@ -107,6 +108,7 @@ impl Snapshot {
stake_threshold: Value,
cap: Fraction,
voting_group_assigner: &impl VotingGroupAssigner,
discrimination: Discrimination,
) -> Result<Self, Error> {
let raw_contribs = raw_snapshot
.0
Expand All @@ -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,
Expand Down Expand Up @@ -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::<u64>().into(),
},
contributions,
Expand Down Expand Up @@ -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()
Expand All @@ -304,6 +316,7 @@ pub mod tests {
threshold.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap()
})
Expand All @@ -319,6 +332,7 @@ pub mod tests {
0.into(),
Fraction::from(1),
&|_vk: &Identifier| String::new(),
Discrimination::Production,
)
.unwrap();
let total_stake = snapshot
Expand All @@ -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(),
)
Expand All @@ -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,
});
}
Expand All @@ -377,6 +393,7 @@ pub mod tests {
0.into(),
Fraction::from(1u64),
&DummyAssigner,
Discrimination::Production,
)
.unwrap();
let vp_1: u64 = snapshot
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions src/catalyst-toolbox/snapshot-lib/src/registration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u64>,

#[serde(default)]
pub nonce: u64,
Expand Down Expand Up @@ -341,7 +341,7 @@ mod tests {
voting_power,
reward_address,
delegations,
voting_purpose: 0,
voting_purpose: None,
nonce: 0,
}
})
Expand Down
16 changes: 15 additions & 1 deletion src/catalyst-toolbox/snapshot-lib/src/voter_hir.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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.
Expand Down Expand Up @@ -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(),
})
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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,
})
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
},
})
})
Expand Down
2 changes: 1 addition & 1 deletion src/vit-testing/mainnet-tools/src/snapshot/convert.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
})
Expand Down
Loading

0 comments on commit 3400229

Please sign in to comment.