From ade3230a54643b760f8f72174781e34ad95e4aa3 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Thu, 19 Oct 2023 14:17:57 +0200 Subject: [PATCH 1/8] Remove TODO --- contracts/consumer/virtual-staking/src/multitest.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/contracts/consumer/virtual-staking/src/multitest.rs b/contracts/consumer/virtual-staking/src/multitest.rs index a1cf8ce6..ee0e3152 100644 --- a/contracts/consumer/virtual-staking/src/multitest.rs +++ b/contracts/consumer/virtual-staking/src/multitest.rs @@ -65,7 +65,6 @@ fn setup<'a>(app: &'a App, args: SetupArgs<'a>) -> SetupResponse<'a> { } } -// TODO: Redundant test. Remove it. #[test] fn instantiation() { let app = App::default(); From 8e8054d5365fb846cc8e6afcded4b4637ec5827f Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Thu, 19 Oct 2023 14:26:07 +0200 Subject: [PATCH 2/8] Use specific boolean flag for robustness --- contracts/consumer/converter/src/contract.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/contracts/consumer/converter/src/contract.rs b/contracts/consumer/converter/src/contract.rs index 6add9b99..de5e9a1f 100644 --- a/contracts/consumer/converter/src/contract.rs +++ b/contracts/consumer/converter/src/contract.rs @@ -363,6 +363,7 @@ impl ConverterApi for ConverterContract<'_> { let channel = IBC_CHANNEL.load(ctx.deps.storage)?; let mut event = Event::new("valset_update"); + let mut is_empty = true; if !additions.is_empty() { event = event.add_attribute( @@ -373,9 +374,11 @@ impl ConverterApi for ConverterContract<'_> { .collect::>() .join(","), ); + is_empty = false; } if !removals.is_empty() { event = event.add_attribute("removals", removals.join(",")); + is_empty = false; } if !updated.is_empty() { event = event.add_attribute( @@ -386,18 +389,22 @@ impl ConverterApi for ConverterContract<'_> { .collect::>() .join(","), ); + is_empty = false; } if !jailed.is_empty() { event = event.add_attribute("jailed", jailed.join(",")); + is_empty = false; } if !unjailed.is_empty() { event = event.add_attribute("unjailed", unjailed.join(",")); + is_empty = false; } if !tombstoned.is_empty() { event = event.add_attribute("tombstoned", tombstoned.join(",")); + is_empty = false; } let mut resp = Response::new(); - if !event.attributes.is_empty() { + if !is_empty { let valset_msg = valset_update_msg( &ctx.env, &channel, From 991483bb118cd86613e7a799e0e3763c4a5e7854 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Thu, 19 Oct 2023 14:30:21 +0200 Subject: [PATCH 3/8] Rename tombstoned / jailed fields for clarity --- .../consumer/virtual-staking/src/contract.rs | 26 +++++++++---------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index 22e1b432..618ebd96 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -41,10 +41,10 @@ pub struct VirtualStakingContract<'a> { pub bonded: Item<'a, Vec<(String, Uint128)>>, /// This is what validators have been fully unbonded due to tombstoning // The list will be cleared after processing in handle_epoch. - pub tombstoned: Item<'a, Vec>, + pub tombstone_requests: Item<'a, Vec>, /// This is what validators have been slashed due to jailing. // The list will be cleared after processing in handle_epoch. - pub jailed: Item<'a, Vec>, + pub jail_requests: Item<'a, Vec>, } #[cfg_attr(not(feature = "library"), sylvia::entry_points)] @@ -58,8 +58,8 @@ impl VirtualStakingContract<'_> { config: Item::new("config"), bond_requests: Map::new("bond_requests"), bonded: Item::new("bonded"), - tombstoned: Item::new("tombstoned"), - jailed: Item::new("jailed"), + tombstone_requests: Item::new("tombstoned"), + jail_requests: Item::new("jailed"), } } @@ -75,8 +75,8 @@ impl VirtualStakingContract<'_> { self.config.save(ctx.deps.storage, &config)?; // initialize these to no one, so no issue when reading for the first time self.bonded.save(ctx.deps.storage, &vec![])?; - self.tombstoned.save(ctx.deps.storage, &vec![])?; - self.jailed.save(ctx.deps.storage, &vec![])?; + self.tombstone_requests.save(ctx.deps.storage, &vec![])?; + self.jail_requests.save(ctx.deps.storage, &vec![])?; VALIDATOR_REWARDS_BATCH.init(ctx.deps.storage)?; set_contract_version(ctx.deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; @@ -127,8 +127,8 @@ impl VirtualStakingContract<'_> { // Make current bonded mutable let mut current = bonded; // Process tombstoning (unbonded) and jailing (slashed) over bond_requests and current - let tombstoned = self.tombstoned.load(deps.storage)?; - let jailed = self.jailed.load(deps.storage)?; + let tombstoned = self.tombstone_requests.load(deps.storage)?; + let jailed = self.jail_requests.load(deps.storage)?; if !tombstoned.is_empty() || !jailed.is_empty() { let ratios = TokenQuerier::new(&deps.querier).slash_ratio()?; let slash_ratio_double_sign = Decimal::from_str(&ratios.slash_fraction_double_sign)?; @@ -142,8 +142,8 @@ impl VirtualStakingContract<'_> { slash_ratio_downtime, )?; // Clear up both lists - self.tombstoned.save(deps.storage, &vec![])?; - self.jailed.save(deps.storage, &vec![])?; + self.tombstone_requests.save(deps.storage, &vec![])?; + self.jail_requests.save(deps.storage, &vec![])?; } // calculate what the delegations should be when we are done @@ -222,16 +222,14 @@ impl VirtualStakingContract<'_> { unjailed: &[String], tombstoned: &[String], ) -> Result, ContractError> { - let _ = (removals, updated, unjailed); - // Account for tombstoned validators. Will be processed in handle_epoch - self.tombstoned.update(deps.storage, |mut old| { + self.tombstone_requests.update(deps.storage, |mut old| { old.extend_from_slice(tombstoned); Ok::<_, ContractError>(old) })?; // Account for jailed validators. Will be processed in handle_epoch - self.jailed.update(deps.storage, |mut old| { + self.jail_requests.update(deps.storage, |mut old| { old.extend_from_slice(jailed); Ok::<_, ContractError>(old) })?; From f046ea081c40e4571c47b749ee9ba5f42ad99a23 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Thu, 19 Oct 2023 14:45:39 +0200 Subject: [PATCH 4/8] Update / adjust comments --- contracts/consumer/virtual-staking/src/contract.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index 618ebd96..74ffa052 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -39,11 +39,11 @@ pub struct VirtualStakingContract<'a> { // `bonded` could be a Map like `bond_requests`, but the only time we use it is to read / write the entire list in bulk (in handle_epoch), // never accessing one element. Reading 100 elements in an Item is much cheaper than ranging over a Map with 100 entries. pub bonded: Item<'a, Vec<(String, Uint128)>>, - /// This is what validators have been fully unbonded due to tombstoning - // The list will be cleared after processing in handle_epoch. + /// This is what validators have been requested to be slashed due to tombstoning. + // The list will be cleared after processing in `handle_epoch`. pub tombstone_requests: Item<'a, Vec>, - /// This is what validators have been slashed due to jailing. - // The list will be cleared after processing in handle_epoch. + /// This is what validators have been requested to be slashed due to jailing. + // The list will be cleared after processing in `handle_epoch`. pub jail_requests: Item<'a, Vec>, } From 2e7c1ebd5ed581f38286934a24fafa7c57e7fcb6 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Fri, 20 Oct 2023 11:11:34 +0200 Subject: [PATCH 5/8] Maintain an inactive list of validators --- .../consumer/virtual-staking/src/contract.rs | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index 74ffa052..21e92473 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -45,6 +45,10 @@ pub struct VirtualStakingContract<'a> { /// This is what validators have been requested to be slashed due to jailing. // The list will be cleared after processing in `handle_epoch`. pub jail_requests: Item<'a, Vec>, + /// This is what validators are inactive because of tombstoning, jailing or removal (unbonded). + // `inactive` could be a Map like `bond_requests`, but the only time we use it is to read / write the entire list in bulk (in handle_epoch), + // never accessing one element. Reading 100 elements in an Item is much cheaper than ranging over a Map with 100 entries. + pub inactive: Item<'a, Vec>, } #[cfg_attr(not(feature = "library"), sylvia::entry_points)] @@ -60,6 +64,7 @@ impl VirtualStakingContract<'_> { bonded: Item::new("bonded"), tombstone_requests: Item::new("tombstoned"), jail_requests: Item::new("jailed"), + inactive: Item::new("inactive"), } } @@ -77,6 +82,7 @@ impl VirtualStakingContract<'_> { self.bonded.save(ctx.deps.storage, &vec![])?; self.tombstone_requests.save(ctx.deps.storage, &vec![])?; self.jail_requests.save(ctx.deps.storage, &vec![])?; + self.inactive.save(ctx.deps.storage, &vec![])?; VALIDATOR_REWARDS_BATCH.init(ctx.deps.storage)?; set_contract_version(ctx.deps.storage, CONTRACT_NAME, CONTRACT_VERSION)?; @@ -127,21 +133,28 @@ impl VirtualStakingContract<'_> { // Make current bonded mutable let mut current = bonded; // Process tombstoning (unbonded) and jailing (slashed) over bond_requests and current - let tombstoned = self.tombstone_requests.load(deps.storage)?; - let jailed = self.jail_requests.load(deps.storage)?; - if !tombstoned.is_empty() || !jailed.is_empty() { + let tombstone = self.tombstone_requests.load(deps.storage)?; + let jail = self.jail_requests.load(deps.storage)?; + if !tombstone.is_empty() || !jail.is_empty() { let ratios = TokenQuerier::new(&deps.querier).slash_ratio()?; let slash_ratio_double_sign = Decimal::from_str(&ratios.slash_fraction_double_sign)?; let slash_ratio_downtime = Decimal::from_str(&ratios.slash_fraction_downtime)?; self.adjust_slashings( deps.storage, &mut current, - tombstoned, - jailed, + &tombstone, + &jail, slash_ratio_double_sign, slash_ratio_downtime, )?; - // Clear up both lists + // Update inactive list + self.inactive.update(deps.storage, |mut old| { + old.extend_from_slice(&tombstone); + old.extend_from_slice(&jail); + old.dedup(); + Ok::<_, ContractError>(old) + })?; + // Clear up both requests self.tombstone_requests.save(deps.storage, &vec![])?; self.jail_requests.save(deps.storage, &vec![])?; } @@ -173,13 +186,13 @@ impl VirtualStakingContract<'_> { &self, storage: &mut dyn Storage, current: &mut [(String, Uint128)], - tombstones: Vec, - jailing: Vec, + tombstones: &[String], + jailing: &[String], slash_ratio_tombstoning: Decimal, slash_ratio_jailing: Decimal, ) -> StdResult<()> { - let tombstones: BTreeSet<_> = tombstones.into_iter().collect(); - let jailing: BTreeSet<_> = jailing.into_iter().collect(); + let tombstones: BTreeSet<_> = tombstones.iter().collect(); + let jailing: BTreeSet<_> = jailing.iter().collect(); // this is linear over current, but better than turn it in to a map for (validator, prev) in current { @@ -234,6 +247,18 @@ impl VirtualStakingContract<'_> { Ok::<_, ContractError>(old) })?; + // Update inactive list. + // We ignore `unjailed` as it's not clear they make the validator active again or not. + if !removals.is_empty() || !additions.is_empty() { + self.inactive.update(deps.storage, |mut old| { + // Add removals + old.extend_from_slice(removals); + // Filter additions + old.retain(|v| !additions.iter().any(|a| a.address == *v)); + old.dedup(); + Ok::<_, ContractError>(old) + })?; + } // Send all updates to the Converter. let cfg = self.config.load(deps.storage)?; let msg = converter_api::ExecMsg::ValsetUpdate { From 3781ce1614057af81cfb7924ffe772af690c448f Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Fri, 20 Oct 2023 11:46:42 +0200 Subject: [PATCH 6/8] Filter out inactive validators from rewards gathering --- .../consumer/virtual-staking/src/contract.rs | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index 21e92473..2f29ae82 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -1,5 +1,5 @@ use std::cmp::Ordering; -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::str::FromStr; use cosmwasm_std::{ @@ -116,7 +116,8 @@ impl VirtualStakingContract<'_> { ) -> Result, ContractError> { // withdraw rewards let bonded = self.bonded.load(deps.storage)?; - let withdraw = withdraw_reward_msgs(deps.branch(), &bonded); + let inactive = self.inactive.load(deps.storage)?; + let withdraw = withdraw_reward_msgs(deps.branch(), &bonded, &inactive); let resp = Response::new().add_submessages(withdraw); let bond = @@ -236,16 +237,20 @@ impl VirtualStakingContract<'_> { tombstoned: &[String], ) -> Result, ContractError> { // Account for tombstoned validators. Will be processed in handle_epoch - self.tombstone_requests.update(deps.storage, |mut old| { - old.extend_from_slice(tombstoned); - Ok::<_, ContractError>(old) - })?; + if !tombstoned.is_empty() { + self.tombstone_requests.update(deps.storage, |mut old| { + old.extend_from_slice(tombstoned); + Ok::<_, ContractError>(old) + })?; + } // Account for jailed validators. Will be processed in handle_epoch - self.jail_requests.update(deps.storage, |mut old| { - old.extend_from_slice(jailed); - Ok::<_, ContractError>(old) - })?; + if !jailed.is_empty() { + self.jail_requests.update(deps.storage, |mut old| { + old.extend_from_slice(jailed); + Ok::<_, ContractError>(old) + })?; + } // Update inactive list. // We ignore `unjailed` as it's not clear they make the validator active again or not. @@ -461,7 +466,14 @@ impl<'a> ValidatorRewardsBatch<'a> { fn withdraw_reward_msgs( deps: DepsMut, bonded: &[(String, Uint128)], + inactive: &[String], ) -> Vec> { + // Filter out inactive validators + let inactive = inactive.iter().collect::>(); + let bonded = bonded + .iter() + .filter(|(v, _)| !inactive.contains(v)) + .collect::>(); // We need to make a list, so we know where to send the rewards later (reversed, so we can pop off the top) let targets = bonded .iter() From 98c2f3f0d3610b9b04af8f78c1dd4f9f8febecb7 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Fri, 20 Oct 2023 11:49:13 +0200 Subject: [PATCH 7/8] Fix / adapt tests --- .../consumer/virtual-staking/src/contract.rs | 65 ++++++++++++++----- 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index 2f29ae82..e626db88 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -731,17 +731,31 @@ mod tests { ] ); - // FIXME: Subsequent rewards msgs could be removed while validator is jailed / inactive + // Subsequent rewards msgs are removed while validator is jailed / inactive + contract.hit_epoch(deps.as_mut()).assert_rewards(&["val2"]); + + // Unjail does nothing + contract.unjail(deps.as_mut(), "val1"); contract .hit_epoch(deps.as_mut()) - .assert_rewards(&["val1", "val2"]); // But rewards are still being gathered + .assert_bond(&[]) // No bond msgs after unjailing + .assert_unbond(&[]) // No unbond msgs after unjailing + .assert_rewards(&["val2"]); + // Removal does nothing (already removed) + contract.remove_val(deps.as_mut(), "val1"); + contract + .hit_epoch(deps.as_mut()) + .assert_bond(&[]) // No bond msgs after unjailing + .assert_unbond(&[]) // No unbond msgs after unjailing + .assert_rewards(&["val2"]); - contract.unjail(deps.as_mut(), "val1"); + // Addition restores the validator to the active set + contract.add_val(deps.as_mut(), "val1"); contract .hit_epoch(deps.as_mut()) .assert_bond(&[]) // No bond msgs after unjailing .assert_unbond(&[]) // No unbond msgs after unjailing - .assert_rewards(&["val1", "val2"]); // Rewards are gathered + .assert_rewards(&["val1", "val2"]); } #[test] @@ -808,7 +822,8 @@ mod tests { let bonded = contract.bonded.load(deps.as_ref().storage).unwrap(); assert_eq!(bonded, [("val1".to_string(), Uint128::new(0)),]); - contract.hit_epoch(deps.as_mut()).assert_rewards(&["val1"]); + // No rewards gatherings after the first one for the jailed validator + contract.hit_epoch(deps.as_mut()).assert_rewards(&[]); // Unjail over unbonded has no effect contract.unjail(deps.as_mut(), "val1"); @@ -816,7 +831,7 @@ mod tests { .hit_epoch(deps.as_mut()) .assert_bond(&[]) // No bond msgs after unjailing .assert_unbond(&[]) // No unbond msgs after unjailing - .assert_rewards(&["val1"]); + .assert_rewards(&[]); let bonded = contract.bonded.load(deps.as_ref().storage).unwrap(); assert_eq!(bonded, [("val1".to_string(), Uint128::new(0)),]); @@ -838,8 +853,16 @@ mod tests { .assert_rewards(&[]); contract.remove_val(deps.as_mut(), "val1"); - // FIXME: Subsequent rewards msgs could be removed while validator is inactive - contract.hit_epoch(deps.as_mut()).assert_rewards(&["val1"]); + // Subsequent rewards msgs are removed while validator is inactive + contract.hit_epoch(deps.as_mut()).assert_rewards(&[]); + + // Addition restores the validator to the active set + contract.add_val(deps.as_mut(), "val1"); + contract + .hit_epoch(deps.as_mut()) + .assert_bond(&[]) // No bond msgs after unjailing + .assert_unbond(&[]) // No unbond msgs after unjailing + .assert_rewards(&["val1"]); // Rewards are being gathered again } #[test] @@ -878,10 +901,8 @@ mod tests { ] ); - // FIXME: Subsequent rewards msgs could be removed after validator is tombstoned - contract - .hit_epoch(deps.as_mut()) - .assert_rewards(&["val1", "val2"]); + // Subsequent rewards msgs are removed after validator is tombstoned + contract.hit_epoch(deps.as_mut()).assert_rewards(&["val2"]); } #[test] @@ -920,8 +941,8 @@ mod tests { ] ); - // FIXME: Subsequent rewards msgs could be removed after validator is tombstoned - contract.hit_epoch(deps.as_mut()).assert_rewards(&["val1"]); + // Subsequent rewards msgs are removed after validator is tombstoned + contract.hit_epoch(deps.as_mut()).assert_rewards(&[]); } #[test] @@ -956,8 +977,8 @@ mod tests { // FIXME: Remove zero amounts assert_eq!(bonded, [("val1".to_string(), Uint128::new(0)),]); - // FIXME: Subsequent rewards msgs could be removed after validator is tombstoned - contract.hit_epoch(deps.as_mut()).assert_rewards(&["val1"]); + // Subsequent rewards msgs are removed after validator is tombstoned + contract.hit_epoch(deps.as_mut()).assert_rewards(&[]); } #[test] @@ -1133,6 +1154,7 @@ mod tests { fn jail(&self, deps: DepsMut, val: &str); fn unjail(&self, deps: DepsMut, val: &str); fn tombstone(&self, deps: DepsMut, val: &str); + fn add_val(&self, deps: DepsMut, val: &str); fn remove_val(&self, deps: DepsMut, val: &str); } @@ -1228,6 +1250,17 @@ mod tests { .unwrap(); } + fn add_val(&self, deps: DepsMut, val: &str) { + let val = cosmwasm_std::Validator { + address: val.to_string(), + commission: Default::default(), + max_commission: Default::default(), + max_change_rate: Default::default(), + }; + self.handle_valset_update(deps, &[val], &[], &[], &[], &[], &[]) + .unwrap(); + } + fn remove_val(&self, deps: DepsMut, val: &str) { self.handle_valset_update(deps, &[], &[val.to_string()], &[], &[], &[], &[]) .unwrap(); From 40f88a3e32b0dc90ee18eb310bf2b91235ee5570 Mon Sep 17 00:00:00 2001 From: Mauro Lacy Date: Fri, 20 Oct 2023 11:54:01 +0200 Subject: [PATCH 8/8] Filter out zero amounts --- contracts/consumer/virtual-staking/src/contract.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/consumer/virtual-staking/src/contract.rs b/contracts/consumer/virtual-staking/src/contract.rs index e626db88..6bc1874e 100644 --- a/contracts/consumer/virtual-staking/src/contract.rs +++ b/contracts/consumer/virtual-staking/src/contract.rs @@ -472,7 +472,7 @@ fn withdraw_reward_msgs( let inactive = inactive.iter().collect::>(); let bonded = bonded .iter() - .filter(|(v, _)| !inactive.contains(v)) + .filter(|(validator, amount)| !amount.is_zero() && !inactive.contains(validator)) .collect::>(); // We need to make a list, so we know where to send the rewards later (reversed, so we can pop off the top) let targets = bonded