Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM] Reserve OutsiderBond for report, when the oracle fails to report #903

Merged
merged 36 commits into from
Jan 30, 2023
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
5dd2847
wip
Chralt98 Dec 9, 2022
1e80b1d
update dependent types
Chralt98 Dec 12, 2022
a0a2f8f
reward the outsider with oracle bond
Chralt98 Dec 12, 2022
a41ff0b
slash in admin_destroy_market
Chralt98 Dec 12, 2022
ad08af4
prepare storage migration
Chralt98 Dec 12, 2022
bf880e0
test new functionality
Chralt98 Dec 12, 2022
1fe1d36
revert storage changes
Chralt98 Dec 16, 2022
155029a
Merge branch 'main' into chralt98-outsider-bond
Chralt98 Jan 5, 2023
20aaaa6
add outsider bond to market storage
Chralt98 Jan 5, 2023
80167be
AddOutsiderBond migration, defensive outsider bond
Chralt98 Jan 5, 2023
89d75f8
use storage alias to avoid migration mistake
Chralt98 Jan 9, 2023
280b4fa
cargo fmt
Chralt98 Jan 9, 2023
76fadc6
log warning in impl_repatriate_bond
Chralt98 Jan 10, 2023
2399ceb
reconfigure outsider bond
Chralt98 Jan 10, 2023
57a3949
Update zrml/prediction-markets/src/lib.rs
Chralt98 Jan 10, 2023
4f90a62
bump market commons version
Chralt98 Jan 10, 2023
a8e20fa
Merge branch 'chralt98-outsider-bond' of github.com:zeitgeistpm/zeitg…
Chralt98 Jan 10, 2023
02eb35b
Apply suggestions from code review
Chralt98 Jan 10, 2023
9e42f0d
use macro for is_bond_pending
Chralt98 Jan 10, 2023
bf6920c
restructure resolve_reported_market
Chralt98 Jan 10, 2023
eaab9bd
improve tests
Chralt98 Jan 11, 2023
b85409c
improve sentinel reserve
Chralt98 Jan 11, 2023
0e44285
Merge branch 'main' into chralt98-outsider-bond
Chralt98 Jan 11, 2023
8203234
fix after merge
Chralt98 Jan 11, 2023
51aa4e4
cargo fmt
Chralt98 Jan 11, 2023
384ace5
Merge branch 'main' into chralt98-outsider-bond
Chralt98 Jan 19, 2023
9a192fc
fix after merge
Chralt98 Jan 19, 2023
758aca3
Merge branch 'main' into chralt98-outsider-bond
Chralt98 Jan 20, 2023
71356aa
remove storage read write count
Chralt98 Jan 24, 2023
745cd30
simplify assert for base asset
Chralt98 Jan 24, 2023
e7df060
remove old migration
Chralt98 Jan 24, 2023
e5633ec
add macro fucntion doc comment
Chralt98 Jan 24, 2023
3d5543e
correct comment
Chralt98 Jan 24, 2023
c1dfa81
cargo fmt
Chralt98 Jan 24, 2023
a5f184a
Update zrml/prediction-markets/src/lib.rs
Chralt98 Jan 24, 2023
4a814c6
Merge branch 'main' into chralt98-outsider-bond
Chralt98 Jan 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions primitives/src/constants/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ parameter_types! {
pub const MaxSubsidyPeriod: Moment = 2_678_400_000;
pub const MaxMarketPeriod: Moment = u64::MAX / 2;
pub const OracleBond: Balance = 50 * CENT;
pub const OutsiderBond: Balance = 25 * CENT;
pub const PmPalletId: PalletId = PalletId(*b"zge/pred");
pub const ValidityBond: Balance = 50 * CENT;
pub const MinDisputeDuration: BlockNumber = 2;
Expand Down
7 changes: 5 additions & 2 deletions primitives/src/market.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ impl<AI, BA> Bond<AI, BA> {
pub struct MarketBonds<AI, BA> {
pub creation: Option<Bond<AI, BA>>,
pub oracle: Option<Bond<AI, BA>>,
pub outsider: Option<Bond<AI, BA>>,
}

impl<AI: Ord, BA: frame_support::traits::tokens::Balance> MarketBonds<AI, BA> {
Expand All @@ -92,14 +93,16 @@ impl<AI: Ord, BA: frame_support::traits::tokens::Balance> MarketBonds<AI, BA> {
Some(bond) if bond.who == *who => bond.value,
_ => BA::zero(),
};
value_or_default(&self.creation).saturating_add(value_or_default(&self.oracle))
value_or_default(&self.creation)
.saturating_add(value_or_default(&self.oracle))
.saturating_add(value_or_default(&self.outsider))
}
}

// Used primarily for testing purposes.
impl<AI, BA> Default for MarketBonds<AI, BA> {
fn default() -> Self {
MarketBonds { creation: None, oracle: None }
MarketBonds { creation: None, oracle: None, outsider: None }
}
}

Expand Down
3 changes: 3 additions & 0 deletions runtime/battery-station/src/parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ parameter_types! {
/// (Slashable) The orcale bond. Slashed in case the final outcome does not match the
/// outcome the oracle reported.
pub const OracleBond: Balance = 50 * CENT;
/// (Slashable) A bond for an outcome reporter, who is not the oracle.
/// Slashed in case the final outcome does not match the outcome by the outsider.
pub const OutsiderBond: Balance = 25 * CENT;
maltekliemann marked this conversation as resolved.
Show resolved Hide resolved
/// Pallet identifier, mainly used for named balance reserves.
pub const PmPalletId: PalletId = PM_PALLET_ID;
/// (Slashable) A bond for creation markets that do not require approval. Slashed in case
Expand Down
5 changes: 3 additions & 2 deletions runtime/common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ macro_rules! decl_common_types {
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
zrml_prediction_markets::migrations::RecordBonds<Runtime>,
zrml_prediction_markets::migrations::AddOutsiderBond<Runtime>,
>;

#[cfg(not(feature = "parachain"))]
Expand All @@ -68,7 +68,7 @@ macro_rules! decl_common_types {
frame_system::ChainContext<Runtime>,
Runtime,
AllPalletsWithSystem,
zrml_prediction_markets::migrations::RecordBonds<Runtime>,
zrml_prediction_markets::migrations::AddOutsiderBond<Runtime>,
>;

pub type Header = generic::Header<BlockNumber, BlakeTwo256>;
Expand Down Expand Up @@ -1002,6 +1002,7 @@ macro_rules! impl_config_traits {
type MaxEditReasonLen = MaxEditReasonLen;
type MaxRejectReasonLen = MaxRejectReasonLen;
type OracleBond = OracleBond;
type OutsiderBond = OutsiderBond;
type PalletId = PmPalletId;
type RejectOrigin = EnsureRootOrHalfAdvisoryCommittee;
type RequestEditOrigin = EitherOfDiverse<
Expand Down
3 changes: 3 additions & 0 deletions runtime/zeitgeist/src/parameters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,9 @@ parameter_types! {
/// (Slashable) The orcale bond. Slashed in case the final outcome does not match the
/// outcome the oracle reported.
pub const OracleBond: Balance = 200 * BASE;
/// (Slashable) A bond for an outcome reporter, who is not the oracle.
/// Slashed in case the final outcome does not match the outcome by the outsider.
pub const OutsiderBond: Balance = 100 * BASE;
maltekliemann marked this conversation as resolved.
Show resolved Hide resolved
/// Pallet identifier, mainly used for named balance reserves. DO NOT CHANGE.
pub const PmPalletId: PalletId = PM_PALLET_ID;
/// (Slashable) A bond for creation markets that do not require approval. Slashed in case
Expand Down
2 changes: 1 addition & 1 deletion zrml/court/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const DEFAULT_MARKET: MarketOf<Runtime> = Market {
resolved_outcome: None,
status: MarketStatus::Closed,
scoring_rule: ScoringRule::CPMM,
bonds: MarketBonds { creation: None, oracle: None },
bonds: MarketBonds { creation: None, oracle: None, outsider: None },
};
const DEFAULT_SET_OF_JURORS: &[(u128, Juror)] = &[
(7, Juror { status: JurorStatus::Ok }),
Expand Down
2 changes: 1 addition & 1 deletion zrml/market-commons/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const MARKET_DUMMY: Market<AccountIdTest, Balance, BlockNumber, Moment> = Market
resolved_outcome: None,
scoring_rule: ScoringRule::CPMM,
status: MarketStatus::Disputed,
bonds: MarketBonds { creation: None, oracle: None },
bonds: MarketBonds { creation: None, oracle: None, outsider: None },
};

#[test]
Expand Down
132 changes: 124 additions & 8 deletions zrml/prediction-markets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ mod pallet {
pallet_prelude::{ConstU32, StorageMap, StorageValue, ValueQuery},
storage::{with_transaction, TransactionOutcome},
traits::{
Currency, EnsureOrigin, Get, Hooks, Imbalance, IsType, NamedReservableCurrency,
OnUnbalanced, StorageVersion,
tokens::BalanceStatus, Currency, EnsureOrigin, Get, Hooks, Imbalance, IsType,
NamedReservableCurrency, OnUnbalanced, StorageVersion,
},
transactional,
weights::Pays,
Expand Down Expand Up @@ -190,6 +190,45 @@ mod pallet {
};
}

macro_rules! impl_repatriate_bond {
($fn_name:ident, $bond_type:ident) => {
/// Settle the $bond_type bond by repatriating it to free balance of beneficiary.
///
/// This function **should** only be called if the bond is not yet settled, and calling
/// it if the bond is settled is most likely a logic error. If the bond is already
/// settled, storage is not changed, a warning is raised and `Ok(())` is returned.
fn $fn_name(
market_id: &MarketIdOf<T>,
beneficiary: &T::AccountId,
) -> Result<BalanceOf<T>, DispatchError> {
let market = <zrml_market_commons::Pallet<T>>::market(market_id)?;
let bond = market.bonds.$bond_type.as_ref().ok_or(Error::<T>::MissingBond)?;
if bond.is_settled {
let warning = format!(
"Attempting to settle the {} bond of market {:?} multiple times",
stringify!($bond_type),
market_id,
);
log::warn!("{}", warning);
debug_assert!(false, "{}", warning);
return Ok(Zero::zero());
}
let missing = <CurrencyOf<T>>::repatriate_reserved_named(
&Self::reserve_id(),
&bond.who,
beneficiary,
bond.value,
BalanceStatus::Free,
)?;
<zrml_market_commons::Pallet<T>>::mutate_market(market_id, |m| {
m.bonds.$bond_type = Some(Bond { is_settled: true, ..bond.clone() });
Ok(())
})?;
Ok(missing)
}
};
}

#[pallet::call]
impl<T: Config> Pallet<T> {
/// Destroy a market, including its outcome assets, market account and pool account.
Expand Down Expand Up @@ -238,6 +277,11 @@ mod pallet {
Self::slash_oracle_bond(&market_id, None)?;
}
}
if let Some(bond) = market.bonds.outsider {
if !bond.is_settled {
Self::slash_outsider_bond(&market_id, None)?;
}
}

if market_status == MarketStatus::Proposed {
MarketIdsForEdit::<T>::remove(market_id);
Expand Down Expand Up @@ -692,10 +736,12 @@ mod pallet {
MarketCreation::Advised => MarketBonds {
creation: Some(Bond::new(sender.clone(), T::AdvisoryBond::get())),
oracle: Some(Bond::new(sender.clone(), T::OracleBond::get())),
..Default::default()
},
MarketCreation::Permissionless => MarketBonds {
creation: Some(Bond::new(sender.clone(), T::ValidityBond::get())),
oracle: Some(Bond::new(sender.clone(), T::OracleBond::get())),
..Default::default()
},
};

Expand Down Expand Up @@ -1220,13 +1266,26 @@ mod pallet {
}
}

let sender_is_oracle = sender == market.oracle;
let origin_has_permission = T::ResolveOrigin::ensure_origin(origin).is_ok();
let sender_is_outsider = !sender_is_oracle && !origin_has_permission;

if should_check_origin {
let sender_is_oracle = sender == market.oracle;
let origin_has_permission = T::ResolveOrigin::ensure_origin(origin).is_ok();
ensure!(
sender_is_oracle || origin_has_permission,
Error::<T>::ReporterNotOracle
);
} else if sender_is_outsider {
let outsider_bond = T::OutsiderBond::get();

market.bonds.outsider = Some(Bond::new(sender.clone(), outsider_bond));

T::AssetManager::reserve_named(
&Self::reserve_id(),
Asset::Ztg,
&sender,
outsider_bond,
)?;
}

market.report = Some(market_report.clone());
Expand Down Expand Up @@ -1528,6 +1587,9 @@ mod pallet {
#[pallet::constant]
type MaxEditReasonLen: Get<u32>;

#[pallet::constant]
type OutsiderBond: Get<BalanceOf<Self>>;

/// The module identifier.
#[pallet::constant]
type PalletId: Get<PalletId>;
Expand Down Expand Up @@ -1930,8 +1992,11 @@ mod pallet {
impl<T: Config> Pallet<T> {
impl_unreserve_bond!(unreserve_creation_bond, creation);
impl_unreserve_bond!(unreserve_oracle_bond, oracle);
impl_unreserve_bond!(unreserve_outsider_bond, outsider);
impl_slash_bond!(slash_creation_bond, creation);
impl_slash_bond!(slash_oracle_bond, oracle);
impl_slash_bond!(slash_outsider_bond, outsider);
impl_repatriate_bond!(repatriate_oracle_bond, oracle);

pub fn outcome_assets(
market_id: MarketIdOf<T>,
Expand Down Expand Up @@ -2347,6 +2412,12 @@ mod pallet {
err
);
}

maltekliemann marked this conversation as resolved.
Show resolved Hide resolved
if let Some(bond) = &market.bonds.outsider {
if !bond.is_settled {
Self::unreserve_outsider_bond(market_id)?;
}
}
}

Ok(report.outcome.clone())
Expand Down Expand Up @@ -2390,11 +2461,56 @@ mod pallet {
// If the oracle reported right, return the OracleBond, otherwise slash it to
// pay the correct reporters.
let mut overall_imbalance = NegativeImbalanceOf::<T>::zero();
if report.by == market.oracle && report.outcome == resolved_outcome {
Self::unreserve_oracle_bond(market_id)?;

let report_by_oracle = report.by == market.oracle;
let is_correct = report.outcome == resolved_outcome;

// TODO: this wrapper (if let Some and !settled) can be removed
// TODO: if there are only markets with outsiders, who all reserved OutsiderBond
Chralt98 marked this conversation as resolved.
Show resolved Hide resolved
let outsider_bond_pending = || -> bool {
if let Some(bond) = &market.bonds.outsider {
if !bond.is_settled {
return true;
}
}
false
};

let unreserve_outsider = || -> DispatchResult {
if outsider_bond_pending() {
Chralt98 marked this conversation as resolved.
Show resolved Hide resolved
Self::unreserve_outsider_bond(market_id)?;
}
Ok(())
};

let slash_outsider = || -> Result<NegativeImbalanceOf<T>, DispatchError> {
if outsider_bond_pending() {
let imbalance = Self::slash_outsider_bond(market_id, None)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about the use of the question mark operator here. I know you didn't start this, but I'm just making an observation. I think we're not handling failure to resolve very well. Right now, if any of these question mark operators actually cause an error, the market fails to resolve and becomes a zombie market, right? This essentially means that every application of the question mark operator here is not a runtime error, but a logic error. If any of these trigger, we've made a mistake. Seems scary.

We may or may not have some markets with outsider reports but without outsider bonds. This is fine because slash_outsider_bond is only called if it is actually pending (we'll log a warning, but that can be disregarded).

Not really sure what I'm getting at here, just pointing this out. 🤷‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. This is essentially also my concern about resolutions. There are many places inside on_resolution where the code potentially could throw an error. That's why we used the approach of is_outsider_bond_pending here and are logging the warning. Whenever we add some error case to slash_outsider_bond it is not covered by is_outsider_bond_pending anymore. But with the current solution there is no way slash_outsider_bond could fail, because the assumptions are ensured in the if statement above.

This is covered by the mentioned if statement and the market existence is covered by the resolution_manager here.

return Ok(imbalance);
}
Ok(NegativeImbalanceOf::<T>::zero())
};

if report_by_oracle {
if is_correct {
Self::unreserve_oracle_bond(market_id)?;
} else {
let negative_imbalance = Self::slash_oracle_bond(market_id, None)?;
overall_imbalance.subsume(negative_imbalance);
}
} else {
let imbalance = Self::slash_oracle_bond(market_id, None)?;
overall_imbalance.subsume(imbalance);
// outsider report
if is_correct {
// reward outsider reporter with oracle bond
let missing = Self::repatriate_oracle_bond(market_id, &report.by)?;
maltekliemann marked this conversation as resolved.
Show resolved Hide resolved
debug_assert!(missing.is_zero(), "Could not deduct all of the value.");
unreserve_outsider()?;
} else {
let oracle_imbalance = Self::slash_oracle_bond(market_id, None)?;
let outsider_imbalance = slash_outsider()?;
Comment on lines +2584 to +2585
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see one scenario which bothers me. What if the market was misconfigured and the market ends too soon so that there's no outcome to report. The oracle solves this problem by not reporting (seems like the best course of action available to the oracle). Then some outsider ignorantly reports something, a dispute ensues and in the end, both are slashed. Seems unfair to the oracle, which always acted with good intent.

What I'm getting at is that this setup is incentivizing the oracle in this situation to report a random outcome in the hope of guessing the correct outcome so that it doesn't get slashed. Even worse, if the oracle is smart, it will probably "guess" the outcome with the highest prediction.

I don't think that there's anything we can do about this here. The problem is, of course, that we don't have a good method of handling markets that resolve too early. What's annoying about this particular situation is that even if our dispute system has a means of delaying the resolution of a market, we still need to trigger a dispute in order to delay the market. But the dispute can only be triggered if someone has reported.

So the request to delay would actually have to come before an outsider can report. Which brings me back to my original suggestion in ZIP-0: "Immediately initiate dispute if the oracle fails to submit a report [in time]" (https://hackmd.io/i326HLsNQTWG8NcIriTwdw#Immediately-initiate-dispute-if-the-oracle-fails-to-submit-a-report).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. T

What if the market was misconfigured and the market ends too soon so that there's no outcome to report. The oracle solves this problem by not reporting.

The implementation before this PR had this problem too. The oracle had oracle_duration time to wait until an outsider could ignorantly report an outcome.

Seems unfair to the oracle, which always acted with good intent.

So the main problem is how to recognise that a market ended too soon. If we could find a solution for this, we could extend the oracle duration.

What I'm getting at is that this setup is incentivizing the oracle in this situation to report a random outcome in the hope of guessing the correct outcome so that it doesn't get slashed.

I see. Yeah, we should better find a way of making the oracle duration more flexible as described above.

What's annoying about this particular situation is that even if our dispute system has a means of delaying the resolution of a market, we still need to trigger a dispute in order to delay the market. But the dispute can only be triggered if someone has reported.

Exactly! We should find a way to delay the market beforehand. So that we have both worlds: Allow a delay by the MDM and by some force which knows when a market couldn't have a final outcome at this time.

Ok, let me brainstorm: your approach (initiate a dispute) suits it well I think. But how to determine whether an oracle forgot to submit a report and an oracle which by purpose did not report because the final outcome was not present?

So we need to either unreserve or slash the oracle bond based on that. When we say: always unreserve the OracleBond when the oracle did not report in time, then the oracle does not get slashed even in the case that the final outcome was present and the oracle just forgot to submit it or worse: by intention.

Another approach: Just let the oracle delay the oracle duration itself. If the advisory committee disagrees on the delay, then the oracle bond gets slashed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how to determine whether an oracle forgot to submit a report and an oracle which by purpose did not report because the final outcome was not present?

I think this boils down to whether or not the dispute was justified. In other words:

  • If the report period ends, the oracle didn't report and the MDM decides that the report needs to be delayed, then the oracle acted correctly and is not punished.
  • If the report period ends, the oracle didn't report and the MDM decides that the market should resolve to outcome A, then the oracle is punished.

But this results in problems if the outcome becomes known between the end of the report period and the end of the dispute. I'm okay with the oracle getting punished in this situations. There are always going to be loopholes and people thinking they've been treated unfairly.

Another option would be to let the MDM decide whether or not to punish the oracle.

Just let the oracle delay the oracle duration itself. If the advisory committee disagrees on the delay, then the oracle bond gets slashed.

That's kind of like raising a dispute with authorized as MDM, right? For advised markets, this would be perfectly fine. In that case, it's the AC's fault that the market wasn't correctly configured. For permissionless markets, this puts too much power in the AC's hands. In this case, I would really prefer a dispute.

I think this warrants it's own ZIP. It definitely doesn't belong into this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the MDM decides that the report needs to be delayed

How would you let the MDM decide whether a report needs to be delayed? In case of authorized it's clear, that the advisory committee could call a new extrinsic which determines that. We need to properly define new requirements here for authorized and court. How does the delay agreement look like in detail. Who decides how, that a report delay is justified. It feels pretty general especially in the case of court that the MDM just decides. Who, how, when, what?

Yeah, I agree, it seems like a whole own ZIP.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to keep in mind with this is that the oracle should propose an outcome. Otherwise I could as market creator just set the oracle duration to the minimum and just wait with my oracle outcome submit until the oracle duration is over to eventually get my bond back. Market creators are then incentivised to set the periods incorrectly so that it's justified that the market ended too soon.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall_imbalance.subsume(oracle_imbalance);
overall_imbalance.subsume(outsider_imbalance);
}
}

for (i, dispute) in disputes.iter().enumerate() {
Expand Down
Loading