Use hourly liquidation events when constructing DLCs #2378

luckysori · 2022-07-06T17:33:46Z

The diff is misleadingly massive because when introducing the new feature in 35c4e4e I chose to simply duplicate the rollover actor. The old version is used to stay compatible with old takers. In my opinion, the correct approach would be to release different versions of xtra_libp2p_rollover as we introduce breaking changes. But this is blocked by being able to release some of the crates in our workspace (and maia): #2393.

bonomat

I might have missed it: what's the expectation of how the contract is liquidated? Is the monitoring taking care of it?

model/src/shared_protocol.rs

bonomat · 2022-07-12T00:32:31Z

xtra-libp2p-rollover/src/maker.rs

                let oracle_pk = self.oracle_pk;
                let n_payouts = self.n_payouts;
                async move {
+                    let Rates {


I think we will need to add a ttl or similar to the rates.
This is needed in case the automation died/is not functioning. Otherwise we might be rolling over with outdated rates.

I suppose you're implying that because we removed the intermediate confirmation step on the maker for rolling over, we don't check that the automaker is up before rolling over. And if it is up we understand that the offer must be up-to-date.

I would argue that the current state of affairs of assuming that the offer is up-to-date simply because the automaker is up is not much safer than not checking anything at all. Nevertheless, I do think we should introduce a safety mechanism. But I would recommend doing in a separate PR, since this one is quite big already and the situation is not significantly worse with this work.

I suppose you're implying that because we removed the intermediate confirmation step on the maker for rolling over, we don't check that the automaker is up before rolling over. And if it is up we understand that the offer must be up-to-date.

yes, this was what I thought :)

I think a follow-up PR is fine given the size of this PR. Can you make sure to create a ticket for that once this is merged please?

Done: #2531.

model/src/cfd.rs

model/src/olivia.rs

model/src/cfd.rs

luckysori · 2022-07-12T02:02:32Z

I might have missed it: what's the expectation of how the contract is liquidated? Is the monitoring taking care of it?

Yes, I think so.

My understanding is that before this PR we monitored all of olivia's attestations that we could care about. Any time we found a new relevant attestation we would attempt to decrypt a CET adaptor signature using it. If successful, we would publish the commit transaction and the CET.

With these changes we have added more attestations to monitor. But the process that occurs after finding an attestation that decrypts a CET is the same: ensure that the CFD is closed with the given CET.

luckysori · 2022-07-12T02:42:59Z

I've addressed your comments, @bonomat. Thanks for your review! I've force-pushed, but you can use the Compare button next to the force-push event on the timeline to see what I've changed compared to what you reviewed. Here's the link: https://github.com/itchysats/itchysats/compare/5cb86ff5182dab07d9cabcfe17bef2076042272d..187cb6f9fa983adfb1b7050c1c207e05014535ec.

I'm only mentioning it because I only recently discovered this. It makes force-pushing a lot safer for reviewers.

bonomat · 2022-07-12T03:47:24Z

I might have missed it: what's the expectation of how the contract is liquidated? Is the monitoring taking care of it?

Yes, I think so.

My understanding is that before this PR we monitored all of olivia's attestations that we could care about. Any time we found a new relevant attestation we would attempt to decrypt a CET adaptor signature using it. If successful, we would publish the commit transaction and the CET.

With these changes we have added more attestations to monitor. But the process that occurs after finding an attestation that decrypts a CET is the same: ensure that the CFD is closed with the given CET.

This makes sense. Do you think it would be worth adding a different monitoring agent which monitors the price movement instead? Particularly for collaboratively liquidation this could be useful.

model/src/cfd/rollover_v_1_0_0.rs

bonomat

Great work and very clean commits. It's a shame that we can't use it yet because setup_contract::new is not used. Do you want to merge this PR anyway or hold back until it is actually being used?

luckysori · 2022-07-12T05:49:07Z

It's a shame that we can't use it yet because setup_contract::new is not used. Do you want to merge this PR anyway or hold back until it is actually being used?

In case it's not clear, we can use dynamic liquidation, but only after the first rollover. I think we're okay to merge it, since it should be tested regularly ASAP, and it's useful by itself.

Also the longer we wait the more tricky rebases we'll have to do 😁

bonomat · 2022-07-12T23:33:26Z

In case it's not clear, we can use dynamic liquidation, but only after the first rollover. I think we're okay to merge it, since it should be tested regularly ASAP, and it's useful by itself.

Thanks for the clarification. I think we should get this tested soon and see what implication this has in regards to storage size and performance.

da-kami

This is great work, thanks for structuring the code into commits.
I think the scope of this PR exploded a bit.

I feel the decision to remove the rollover confirmation could have done separately to keep this contained. There are so many changes that it becomes hard to evaluate if the complete solution is still sane and no bugs sneaked in. Furthermore, rebasing this work onto the recent rollover changes will not be fun. Maybe it would be worth creating smaller PRs that build on each other next time, so some work can alrady make it into master and we don't have such a big changeset?

I will give this another review after the rebase is done - if you can easily split this work up into multiple PRs that focus on refactor / feature to change accept / actual new rollover_v2 that would help, but I am not sure it's worth the effort at this stage.

model/src/shared_protocol.rs

xtra-libp2p-rollover/src/maker.rs

da-kami · 2022-07-21T14:21:12Z

maker/src/routes.rs

+
+#[rocket::post("/rollover-config", data = "<config>")]
+#[instrument(name = "POST /rollover-config", skip(maker), err)]
+pub async fn update_rollover_configuration(


What is the motivation of this change? Because we remove the accept step we need another way to just stop accepting rollovers? What's the use case for this?

@bonomat maybe you can explain why this feature is needed. I feel we never really turned rollovers off, so I wonder if this feature is actually justified.

@bonomat I'm merging this as is. But we can remove this feature later if it's deemed unnecessary.

Sorry, I missed this comment. We might indeed not need this feature.

model/src/cfd.rs

da-kami · 2022-07-21T14:28:35Z

maker/src/cfd.rs

@@ -589,6 +589,10 @@ impl<O, T, W> Actor<O, T, W> {
        }
    }

+    // TODO: Because of this we need to have two rollover actor


I think we could actually refactor the old protocol to remove this step as well.
For me the decision if we want to handle accept in a different way is independent of the liquidation feature and could be in a separate PR.

Note to self: think about why this TODO is still here AND attempt to split this PR into:

One that removes the accept/reject automation step.

One that extracts the new crate and introduces the liquidation feature.

As promised:

Replace rollover confirmation step with configurable flag #2533 removes the maker's rollover confirmation step.

This PR introduces hourly liquidation events.

model/src/cfd.rs

model/src/cfd/rollover_v_1_0_0.rs

da-kami · 2022-07-21T14:35:33Z

model/src/payouts.rs

+    fn new(announcements: Vec<olivia::Announcement>) -> Result<Self> {
+        let announcements = announcements
+            .into_iter()
+            .sorted_by(|a, b| a.id.cmp(&b.id))


Does it make sense to sort using the complete BitMexPriceEventId or should we rather sort by the timestamp within BitMexPriceEventId?

Great comment!

Currently we have derived PartialOrd on BitMexPriceEventId and the timestamp field happens to be the first field, so we are effectively sorting by timestamp. Unless the timestamps are equal, in which case we would compare the digits. But those are always 20, given that we always construct them using BitMexPriceEventId::with_20_digits.

But this is a bad approach, since someone could nonchalantly flip the order of the fields and assume that everything is fine. So I will add a patch fixing this.

See 46e0890.

xtra-libp2p-rollover/src/v_2_0_0/maker.rs

luckysori · 2022-07-25T01:38:22Z

Do you think it would be worth adding a different monitoring agent which monitors the price movement instead? Particularly for collaboratively liquidation this could be useful.

I don't fully see what you have in mind. By monitoring olivia's attestations we are effectively monitoring price movement: each olivia attestation includes a price and we use it to select CETs that should have been "activated".

But I suppose if we add the feature of collaborative liquidation we will need to look at a regular price feed (the same one that olivia uses!).

luckysori · 2022-07-26T01:22:43Z

bors r+

2378: Use hourly liquidation events when constructing DLCs r=luckysori a=luckysori Fix #2299. The diff is misleadingly massive because when introducing the new feature in 35c4e4e I chose to simply duplicate the rollover actor. The old version is used to stay compatible with old takers. In my opinion, the correct approach would be to release different versions of `xtra_libp2p_rollover` as we introduce breaking changes. But this is blocked by being able to release some of the crates in our workspace (and `maia`): #2393. Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>

bors · 2022-07-26T01:48:21Z

Canceled.

luckysori · 2022-07-26T07:03:31Z

bors r+

I'll add the tests in a separate PR. Let's not block this any longer :D

bors · 2022-07-26T07:16:31Z

Merge conflict.

The main motivation for this change is to be able to support multiple versions of the rollover protocol at the same time, without incurring in lots of duplicated code or increased code complexity due to branching logic. Additionally, I think this should allow us to test this protocol in isolation. I think it's also worth it in terms of the overall structure of the workspace, and could lead to improved compilation times. In my view, the main downside of this approach is that the API of the rollover protocol is slightly more complicated. We can no longer rely on knowing the actors and components that we interact with, because they remain in `daemon`. Instead we depend on 2 new traits: `model::ExecuteOnCfd` and `rollover::GetAnnouncements`. The upside of making the rollover actors generic via these traits is that we can now test this protocol in isolation, as mentioned above.

This is important because deriving `PartialOrd` normally means that it looks at all the struct's fields _in order_. For this type we want to compare the `timestamp`, but do not care about the `digits`! More importantly, someone could unintentionally change this behaviour by reordering the fields. We also have to ignore the `digits` field for other derived traits, because inconsistencies there could cause trouble[1]. [1]: rust-lang/rust-clippy#1621. Co-authored-by: Daniel Karzel <daniel.karzel@coblox.tech>

The new `itchysats/rollover/2.0.0` protocol adds liquidation events to the DLC. In order to preserve backwards compatibility with takers running `itchysats/rollover/1.0.0` we run another rollover actor in parallel for the maker. This leads to a ton of duplicated code, but this technical debt is temporary and contained in the `xtra-libp2p-rollover` crate. The idea is to release all our crates to `crates.io`, so that we can eventually introduce a single cargo dependency per version of a protocol.

Identified using cargo-udeps.

It was not very intuitive to place the identity public keys and the role in the `xtra_libp2p_rollover::PunishParams` struct. Those values are not specific to the punishment mechanism of the DLC.

This code used to assume that we had a single event per CFD when constructing the corresponding DLC. After introducing liquidation events, that is no longer the case. Caveat: this code is currently not used! When we added patch 99e62ba, we chose to only use the deprecated version of this code (in `setup_contract_deprecated.rs`) for the maker and all possible takers. The idea was to then use `setup_contract::new` after adding the contract setup protocol over the new network layer. Since this hasn't happened at the time of writing this message, we cannot test the changes in this patch yet.

luckysori · 2022-07-26T07:53:20Z

bors retry

bors · 2022-07-26T07:53:22Z

Already running a review

bors · 2022-07-26T08:16:05Z

After the introduction of the `itchysats/order` protocol we can also prove that the CFD includes liquidation events from the moment it is open (not the case when we first merged #2378). When adding these tests we discovered and fixed a bug which was causing us to only generate _short_ liquidation events.

2570: Add liquidation actor tests r=luckysori a=luckysori After the introduction of the `itchysats/order` protocol we can also prove that the CFD includes liquidation events from the moment it is open (not the case when we first merged #2378). When adding these tests we discovered and fixed a bug which was causing us to only generate _short_ liquidation events. --- I'm gonna create an issue to add more tests based on the `maker_position`, because that determines the shape of the payout curve, which affects the liquidation intervals. Interestingly, here (maker going short) I only check the _long_ liquidation events, because the short liquidation events are pretty useless and hard to test: we have CETs for one specific price (as opposed to an interval for long liquidation CETs). This is because of the way our payout curve is implemented. Co-authored-by: Lucas Soriano del Pino <lucas_soriano@fastmail.com>

luckysori requested review from bonomat, da-kami and klochowicz July 6, 2022 17:33

luckysori self-assigned this Jul 6, 2022

luckysori force-pushed the liquidation-events branch 2 times, most recently from 45ea3eb to 24077e4 Compare July 7, 2022 05:09

luckysori mentioned this pull request Jul 8, 2022

Identify Protocol #2383

Closed

luckysori force-pushed the liquidation-events branch from 24077e4 to eef1ae8 Compare July 11, 2022 05:16

luckysori marked this pull request as ready for review July 11, 2022 05:50

luckysori force-pushed the liquidation-events branch 4 times, most recently from b02e9e7 to 91100f8 Compare July 12, 2022 01:10

bonomat reviewed Jul 12, 2022

View reviewed changes

luckysori force-pushed the liquidation-events branch from 91100f8 to 5cb86ff Compare July 12, 2022 01:29

luckysori force-pushed the liquidation-events branch from 5cb86ff to 187cb6f Compare July 12, 2022 02:39

luckysori requested a review from bonomat July 12, 2022 02:40

bonomat reviewed Jul 12, 2022

View reviewed changes

model/src/cfd/rollover_v_1_0_0.rs Show resolved Hide resolved

bonomat approved these changes Jul 12, 2022

View reviewed changes

luckysori mentioned this pull request Jul 20, 2022

Test the rollover protocol in isolation to capture more edge cases #2497

Closed

da-kami mentioned this pull request Jul 21, 2022

Structure tests by use case instead of one happy_path file #2505

Merged

da-kami reviewed Jul 21, 2022

View reviewed changes

luckysori mentioned this pull request Jul 25, 2022

Do not allow maker rollovers if offer is not up-to-date #2531

Closed

luckysori mentioned this pull request Jul 25, 2022

Replace rollover confirmation step with configurable flag #2533

Merged

Base automatically changed from no-rollover-confirmation to master July 26, 2022 01:46

luckysori force-pushed the liquidation-events branch from 44aec6c to f93c24b Compare July 26, 2022 01:48

luckysori force-pushed the liquidation-events branch 2 times, most recently from 637f05c to d212712 Compare July 26, 2022 07:02

luckysori and others added 9 commits July 26, 2022 17:48

Prefer txid over tx_id

b31da8a

Clarify message for IrrelevantAttestation error

ff9a7f8

Model PriceOutOfRange error explicitly

e20463d

Remove unused dependencies

1ec8541

Identified using cargo-udeps.

Simplify PunishParams

0cba5dd

It was not very intuitive to place the identity public keys and the role in the `xtra_libp2p_rollover::PunishParams` struct. Those values are not specific to the punishment mechanism of the DLC.

luckysori force-pushed the liquidation-events branch from d212712 to f47bcff Compare July 26, 2022 07:49

bors bot merged commit dcb9d1a into master Jul 26, 2022

bors bot deleted the liquidation-events branch July 26, 2022 08:16

luckysori mentioned this pull request Jul 28, 2022

Add liquidation actor tests #2570

Merged

luckysori mentioned this pull request Aug 15, 2022

"Dynamic" liquidation #558

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hourly liquidation events when constructing DLCs #2378

Use hourly liquidation events when constructing DLCs #2378

luckysori commented Jul 6, 2022 •

edited

bonomat left a comment

bonomat Jul 12, 2022

luckysori Jul 12, 2022

bonomat Jul 12, 2022

luckysori Jul 25, 2022

luckysori commented Jul 12, 2022

luckysori commented Jul 12, 2022

bonomat commented Jul 12, 2022

bonomat left a comment

luckysori commented Jul 12, 2022

bonomat commented Jul 12, 2022

da-kami left a comment

da-kami Jul 21, 2022

da-kami Jul 22, 2022

luckysori Jul 26, 2022

bonomat Jul 26, 2022

da-kami Jul 21, 2022

luckysori Jul 22, 2022

luckysori Jul 25, 2022

da-kami Jul 21, 2022

luckysori Jul 25, 2022 •

edited

luckysori Jul 25, 2022

luckysori commented Jul 25, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

bors bot commented Jul 26, 2022

Use hourly liquidation events when constructing DLCs #2378

Use hourly liquidation events when constructing DLCs #2378

Conversation

luckysori commented Jul 6, 2022 • edited

bonomat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori commented Jul 12, 2022

luckysori commented Jul 12, 2022

bonomat commented Jul 12, 2022

bonomat left a comment

Choose a reason for hiding this comment

luckysori commented Jul 12, 2022

bonomat commented Jul 12, 2022

da-kami left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori Jul 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luckysori commented Jul 25, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

luckysori commented Jul 26, 2022

bors bot commented Jul 26, 2022

bors bot commented Jul 26, 2022

luckysori commented Jul 6, 2022 •

edited

luckysori Jul 25, 2022 •

edited