-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(proofs): FaultDisputeGame refund mode #85
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the change to incentive of refunding bonds, I feel like that might be a step too far. It would make a lot of sense to me though if the dispute game automatically prevented withdrawals from a game that has been blacklisted. Then the operational response to ensure the portal doesn't allow withdrawals also automatically protects bonds. We then have more time to work out how to redistribute the bonds appropriately. Having a script to easily allow refunds (or maybe even a function built into FaultDisputeGame/DelayedWETH) would also be useful, but I'd make it a manual trigger since in most cases it would be better for the bonds to be given to the honest actors even if the game didn't resolve correctly in their favour.
We propose modifying the `FaultDisputeGame` to enter a "refund mode" if the game is blacklisted. | ||
When the `FaultDisputeGame` is in this refund mode it will distribute bonds back to the users who | ||
originally deposited those bonds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The major downside of this is that it makes attacks which were previously extremely expensive because you lose your bonds very cheap because everyone gets a refund of bonds. That's a very unfortunate change in incentives which makes griefing much more likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so this is an optional mechanism that acts as a default. DelayedWETH
can still be used to remove bonds just like before, we're just optionally adding this extra mechanism that can be used in the case where removing bonds wouldn't be worth the effort.
So if someone is executing an extremely expensive attack then DelayedWETH
could still be used to hold those bonds and would therefore disincentivize such an attack just as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it just sounds like when we blacklist a game it will go into refund mode immediately which we wouldn't want. If would could build a refund script offline without it being too complex I'd be tempted to do that, but it may be easier to have it built into the contracts.
An offline script just gives us more flexibility - for example we might want to create a list of honest and dishonest actor addresses and run a script to pay out bonds appropriately or only refund actors on the honest list etc. We should write those scripts ahead of time for anything we're likely to need though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could modify this so that refund mode begins after the airgap window -- that would solve this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah if it only kicked in after the air gap if manual intervention hadn't happened before then that would make sense.
For instance, `FaultDisputeGame` instances are usually expected to be blacklisted only after they | ||
resolve incorrectly, so it may be possible for some users to begin withdrawing *before* a game is | ||
blacklisted, then the game becomes blacklisted and those same users can withdraw their original | ||
bonds. We can avoid this sort of double-counting and generally simplify the security model of the | ||
contract by preventing bond withdrawals until the game has resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of simplifying when bonds can be claimed. Having it match when the dispute game could be used withdrawals (ie 3.5 days after the game finalizes) makes a lot of sense and is much easier to reason about.
Changes that need to be made to the `FaultDisputeGame` bond unlock logic may require some small | ||
modifications to `op-challenger` and `op-dispute-mon`. We may be able to avoid changes to | ||
`op-challenger` if we move the `unlock` call into `claimCredit` and simply fail if the unlock | ||
has already been made. Need to confirm here that `op-challenger` will just retry this claim | ||
function over and over, but if that's the case then this works just fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Challenger should just keep retrying already. But I'm not concerned about the need to modify challenger - it has really good support for handling multiple different contract versions already so can tolerate behaviour changing.
`op-dispute-mon` would need to be modified slightly to account for the fact that (1) the credits | ||
inside of `DelayedWETH` will only match *after* the game has resolved and (2) the unlocks may | ||
become mismatched if refund mode is activated. Both situations can be managed but do require some | ||
changes to `op-dispute-mon`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite a few alerts in dispute-mon that will go wonky after manual intervention. We generally accept that because manual intervention is unusual and is only done when the system is in an unexpected state. So if you start from an unexpected state and do unusual things monitoring has very little chance of being able to make sense of it all. We can add dispute games to an ignore list in dispute-mon for that case when we know they're in a weird state and have been dealt with manually.
There's a fair bit of monitoring around withdrawal credits that would need to be modified but that's quite doable. dispute-mon hasn't had to deal with multiple versions as much but it has some support and is generally pretty flexible and easy to work with so I don't expect the modifications to be difficult.
handle cases where many games need to be blacklisted. If we were to adopt DD#76 alongside | ||
[DD#77](https://github.com/ethereum-optimism/design-docs/pull/77) then we could use the proposed | ||
`gameCreationValidFrom` timestamp to act as an implicit blacklist of all games created before this | ||
timestamp. The `AnchorStateRegistry.isValidGame(...)` function would return `false` if the game is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add note for flag to not go into refund mode if withdrawals have already started
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: actually, handle this in AnchorStateRegistry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're not handling this in AnchorStateRegistry
anymore right? Then we will need to track if withdrawals have started in DelayedWETH
. I suspect just tracking if someone did withdraw is the simplest thing rather than trying to track if someone could have.
Proposes a change to the FaultDisputeGame that allows it to automatically enter a "refund mode" when the game finds itself to be blacklisted.
af81441
to
e1222d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just with the confirmation that tracking when withdrawals started won't be done in AnchorStateRegistry
now.
handle cases where many games need to be blacklisted. If we were to adopt DD#76 alongside | ||
[DD#77](https://github.com/ethereum-optimism/design-docs/pull/77) then we could use the proposed | ||
`gameCreationValidFrom` timestamp to act as an implicit blacklist of all games created before this | ||
timestamp. The `AnchorStateRegistry.isValidGame(...)` function would return `false` if the game is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're not handling this in AnchorStateRegistry
anymore right? Then we will need to track if withdrawals have started in DelayedWETH
. I suspect just tracking if someone did withdraw is the simplest thing rather than trying to track if someone could have.
We don't need to track when withdrawals started in |
Proposes a change to the FaultDisputeGame that allows it to automatically enter a "refund mode" when the game finds itself to be blacklisted.