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

Conversation

Chralt98
Copy link
Member

@Chralt98 Chralt98 commented Dec 9, 2022

Fixes #868. Fixes #698.

@Chralt98 Chralt98 added the s:in-progress The pull requests is currently being worked on label Dec 9, 2022
@Chralt98 Chralt98 added this to the v0.3.8 milestone Dec 9, 2022
@Chralt98 Chralt98 self-assigned this Dec 9, 2022
@Chralt98 Chralt98 added s:on-hold Work on the pull request has been paused and removed s:in-progress The pull requests is currently being worked on labels Dec 13, 2022
@Chralt98
Copy link
Member Author

Waiting for #892 to use the approach for tracking bonds.

@Chralt98 Chralt98 modified the milestones: v0.3.8, v0.3.9 Jan 5, 2023
@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:on-hold Work on the pull request has been paused labels Jan 5, 2023
runtime/battery-station/src/parameters.rs Outdated Show resolved Hide resolved
runtime/zeitgeist/src/parameters.rs Outdated Show resolved Hide resolved
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews i:config-values 💻 New/changed config values and removed s:in-progress The pull requests is currently being worked on labels Jan 5, 2023
@Chralt98 Chralt98 marked this pull request as ready for review January 5, 2023 13:34
@Chralt98 Chralt98 removed the s:review-needed The pull request requires reviews label Jan 6, 2023
Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

Yeah, nice. Couple o' nitpicks, but nothing really problematic.

Comment on lines +2578 to +2579
let oracle_imbalance = Self::slash_oracle_bond(market_id, None)?;
let outsider_imbalance = slash_outsider()?;
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.

zrml/prediction-markets/src/migrations.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/migrations.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/tests.rs Outdated Show resolved Hide resolved
zrml/prediction-markets/src/migrations.rs Show resolved Hide resolved
zrml/prediction-markets/src/tests.rs Show resolved Hide resolved
@maltekliemann
Copy link
Member

Just realized that you should also document the new functions.

@maltekliemann maltekliemann added s:revision-needed The pull requests must be revised and removed s:review-needed The pull request requires reviews labels Jan 23, 2023
@maltekliemann
Copy link
Member

About the outsider bond. Anything >= 2 * OracleBond::get() should be fine to prevent dumb guessing, but the question is if it should be even higher. Disputes currently cost ten times of the oracle bond.

@Chralt98
Copy link
Member Author

Chralt98 commented Jan 24, 2023

Just realized that you should also document the new functions.

I only see the macro function "impl_is_bond_pending" which comes to my mind. Which functions do you exactly mean? 3d5543e

@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:revision-needed The pull requests must be revised labels Jan 24, 2023
maltekliemann
maltekliemann previously approved these changes Jan 24, 2023
zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved

let slash_outsider = || -> Result<NegativeImbalanceOf<T>, DispatchError> {
if Self::is_outsider_bond_pending(market_id, market, true) {
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.

Co-authored-by: Malte Kliemann <mail@maltekliemann.com>
@vivekvpandya vivekvpandya added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jan 30, 2023
@Chralt98 Chralt98 merged commit 0fd31bf into main Jan 30, 2023
@Chralt98 Chralt98 deleted the chralt98-outsider-bond branch January 30, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i:config-values 💻 New/changed config values s:accepted This pull request is ready for merge
Projects
None yet
4 participants