Circuit breaker remove solver #2705

sunce86 · 2024-05-08T17:29:14Z

Description

Related to #2667

POC implementation for using "Roles" safe module to grant special role to an EOA to sign and execute "removeSolver" function on behalf of the gpv2_authenticator manager/owner safe.

Need to add tests to see if this actually works.

Changes

Added Roles smart contract
Added EOA account as configuration
Implemented remove_solver function

How to test

todo

fleupold

Let's please make sure the circuit breaker is an opt-in functionality on the autopilot (ie. it's sufficiently factored as a separate background task which can be left out e.g. in local deployments).

crates/shared/src/arguments.rs

MartinquaXD

I would like to see a CircuitBreaker struct from the get go that should store the state required to remove solvers (e.g. EOA).
Ideally this struct should be able to handle all the logic around removing solvers in a background task. If for some reason (maybe it makes the most sense to decide some things in the autopilot during the competition) that struct could expose the remove_sovler function which the autopilot could then call if needed.
But overall I think we should try to leave the autopilot untouched if possible because for proper decentralization it should be possible to ban solvers solely based on on-chain data.

crates/autopilot/src/infra/blockchain/contracts.rs

crates/autopilot/src/infra/blockchain/mod.rs

crates/autopilot/src/infra/blockchain/circuit_breaker.rs

sunce86 · 2024-05-14T12:31:57Z

So, how I understand, the flow is following:

Settlement appears onchain -> settlement event is saved to our database
OnSettlementEventUpdater is triggered. It builds a domain object that will look like something like this: https://github.com/cowprotocol/services/blob/settlement-auction-struct-new/crates/autopilot/src/domain/settlement/transaction.rs#L13-L40
OnSettlementEventUpdater now needs to do 2 things: 1. update db based on this object, 2. generate violations and decide to call circuit breaker if needed
circuit breaker exposes removeSolver fn and once called, emits a transaction that removes solver in a fire and forget manner.

So, bottom line, circuit breaker's smallest responsibility unit is to execute remove solver and nothing else.
@fleupold @MartinquaXD what would be your suggestion to change in this flow? I understand we want this functionality isolated as much as possible but we decided to reuse EventHandler for events, domain::settlement::transaction::Tx for decoding settlement calldata and fetching auction data, so how should we make the circuit breaker more independent?

Edit: posting the question since afais based on the PR changes, @Mateo-mro understood that circuit breaker needs to observe onchain settlement on it's own and be completely disconnected from the EventHandler.

crates/autopilot/src/infra/blockchain/circuit_breaker.rs

MartinquaXD · 2024-05-14T12:54:09Z

so how should we make the circuit breaker more independent?

Not sure if my gut makes sense but it seems strange to fit all this logic intoOnSettlementEventUpdater. I guess the name is vague enough that technically it could also cover circuit breaking logic but it seems to me that it could end up nicer if all the rules and logic to decide if a settlement should cause a solver to get disconnected should live inside a single unit.
So far the OnSettlementEventUpdater was more like a non-judgmental observer, I think.
However, I see that monitoring events in the circuit breaker itself might not be the best idea in terms of code reuse so maybe a slightly alternative idea could be to let the OnEventSettlementUpdater monitor and parse the settlement events and the pass reasonably formed structs into the CircuitBreaker which then decides whether or not to remove the solver.

Another thing that irks me with tying the CircuitBreaker to the autopilot is that the circuit breaker would ideally also work when the autopilot is having issues or re-indexes stuff. With the OnEventSettlementUpdater calling the shots we would remove offending solvers again when we re-index settlements, no? I know that this doesn't happen often but this actually just happened a few weeks ago. (the idea mentioned above would also suffer from this issue)

sunce86 · 2024-05-14T14:09:28Z

Notes from the call:

Circuit breaker to be basically a copy paste of OnSettlementEventUpdater with regards how it listens for new events and how it is triggered (see self.settlement_updater.schedule_update() calls)
Needs to read latest events (tx hash). Taking just the last event from database is not good as it can happen that it is already handled or it can happen that both staging and prod settle tx in the same block so we might miss. I am ok with having an in-memory cache of handled events, and when autopilot is restarted start from the latest event and build cache going forward.
Having a tx hash, build domain::settlement::transaction::Tx. Generate violations.
Decide whether to act on violations
Call removeSolver function in async function in a fire and forget manner (high maxFee, nothing fancy).

@Mateo-mro can you then work on these points (except (3) as I am still working on it). You can skip this step and just mock an intention to removeSolver (for example, call it for every event for now).

m-lord-renkse · 2024-05-15T10:14:02Z

crates/autopilot/src/arguments.rs

@@ -359,6 +365,7 @@ impl std::fmt::Display for Arguments {
            max_settlement_transaction_wait
        )?;
        writeln!(f, "s3: {:?}", s3)?;
+        writeln!(f, "circuit breaker: {:?}", circuit_breaker)?;


@sunce86 @MartinquaXD can we display the authenticator_eoa here? or should we hide it?

The EOA is actually a private key so it should be hidden.

In practice let's please KMS for this.

m-lord-renkse · 2024-05-15T10:15:17Z

crates/autopilot/src/on_settlement_event.rs

+            registry.append(&mut events);
+            (registry.len() > Self::MAX_REGISTRY_SIZE).then(|| {
+                (0..(registry.len() - Self::MAX_REGISTRY_SIZE)).for_each(|_| {
+                    registry.pop_first();
+                })
+            });
+        }


Agreed with @sunce86 to do it by fetching 4 events from the database and keeping in memory the processed ones, but we are open to change it of course.

What's more, I know this piece of code could be difficult to read (I am a .then() and .for_each() lover), I am open to change it.

crates/autopilot/src/infra/solvers/authenticator.rs

MartinquaXD · 2024-05-15T18:14:49Z

crates/autopilot/src/arguments.rs

+impl FromStr for CircuitBreaker {
+    type Err = anyhow::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {


I think providing a single string that configures 2 different things is a bit weird. Instead you could pass these things separately and make them both optional. Validating the input would then basically just be throwing an error if exactly 1 thing is set.
Also it could be that we require more arguments for the circuit breaker and adding more and more stuff to that string will become messy pretty quickly.

@MartinquaXD This comment is on non-existing code (I deleted it).

crates/autopilot/src/infra/blockchain/contracts.rs

MartinquaXD · 2024-05-15T18:21:34Z

crates/autopilot/src/on_settlement_event.rs

+
+#[allow(dead_code)]
+pub struct CircuitBreaker {
+    eth: infra::blockchain::circuit_breaker::CircuitBreaker,


Why is this called eth?

@MartinquaXD This comment is on non-existing code (I deleted it).

crates/autopilot/src/infra/solvers/authenticator.rs

MartinquaXD · 2024-05-17T11:14:31Z

crates/autopilot/src/infra/solvers/authenticator.rs

+                .unwrap()
+        };
+
+        let settlement = contracts::GPv2Settlement::at(


IMO the decision to use a custom address or a deployed address should be done at single point in the backend. For example it doesn't make a lot of sense to have 1 component use one address for the settlement contract and another component another address. Overall I think whoever instantiates the Authenticator should be responsible for picking the correct address.

Also the settlement contract doesn't really matter here. We only need the authenticator contract.

I suggested this to @Mateo-mro so that Authenticator relies less on intermediary objects built between the autopilot startup and Authenticator constructor being called -> to be more independent of the rest of the code.

For example it doesn't make a lot of sense to have 1 component use one address for the settlement contract and another component another address.

I believe this is also irrelevant for the Authenticator as a component that we envision running as a separate process etc. Choosing the right address would be the responsibility of Authenticator.

MartinquaXD · 2024-05-17T11:16:40Z

crates/autopilot/src/infra/solvers/authenticator.rs

+                .expect("authenticator address"),
+        );
+
+        let authenticator_role = contracts::Roles::at(


Also why do we effectively overriding the authenticator address but not the roles address? IMO another indicator that these dependencies should be injected.

True. IMO all contracts that the Authenticator depends on need to be received via Addresses.

but in that case shouldn't we be passing Contracts instead of Addresses? does it also make sense for the addresses which are specific for the circuit breaker? like the role or authenticator eoa?

If we want to make the component as independent as possible, it should receive raw configuration params so it can independently build whatever it needs to work properly.

With that in mind, Contracts is an internal autopilot component and shouldn't be forwarded to Authenticator. The reason being once we wish to move Authenticator out of the autopilot there would be no need to refactor constructor to not work with Contracts.

but I think that making an object depending on another object within the same crate is totally ok. In my mind, having a struct which instantiates the contracts and then those contracts are use somewhere else (in circuit breaker) would make sense 🤔

If you want to make the module as independent as possible, then we can have two builders (or new) for it, once with raw address and the other with the contract, so if we move it out of autopilot it won't require any change.

I don't want to bikeshed on this too much, since it's not essential for delivery of feature (and it also won't be too problematic to refactor this if needed). That said, I am ok with both versions.

crates/autopilot/src/infra/solvers/authenticator.rs

MartinquaXD

One last thing. The new authenticator module currently lives in the infra/solvers which seems odd. That directory holds all the code needed to talk to the solvers. /infra/blockchain seems more appropriate since it's used to send transactions to the blockchain. 🤔

m-lord-renkse · 2024-05-31T07:35:25Z

Merging since I got two approvals (counting also @sunce86 's approval here: #2705 (review))

sunce86 added the E:7.1 Ext. solvers operating driver See https://github.com/cowprotocol/pm/issues/57 for details label May 8, 2024

sunce86 self-assigned this May 8, 2024

sunce86 requested a review from a team as a code owner May 8, 2024 17:29

fleupold reviewed May 10, 2024

View reviewed changes

MartinquaXD reviewed May 10, 2024

View reviewed changes

crates/shared/src/arguments.rs Outdated Show resolved Hide resolved

sunce86 assigned m-lord-renkse May 13, 2024

sunce86 added 2 commits May 14, 2024 11:11

Initial commit

ed3b7cb

clippy

8a5c408

m-lord-renkse force-pushed the safe-contract branch from c2cd383 to 6e3acc1 Compare May 14, 2024 09:44

m-lord-renkse changed the title ~~DRAFT: Circuit breaker remove solver~~ Circuit breaker remove solver May 14, 2024

m-lord-renkse requested a review from MartinquaXD May 14, 2024 09:50

MartinquaXD reviewed May 14, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/contracts.rs Outdated Show resolved Hide resolved

m-lord-renkse force-pushed the safe-contract branch from 6e3acc1 to 311037c Compare May 14, 2024 11:58

m-lord-renkse reviewed May 14, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/mod.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed May 14, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/mod.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed May 14, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/circuit_breaker.rs Outdated Show resolved Hide resolved

m-lord-renkse reviewed May 14, 2024

View reviewed changes

crates/autopilot/src/infra/blockchain/circuit_breaker.rs Outdated Show resolved Hide resolved

m-lord-renkse force-pushed the safe-contract branch from 311037c to bbaa7cf Compare May 14, 2024 12:01

m-lord-renkse requested a review from MartinquaXD May 14, 2024 12:01

MartinquaXD reviewed May 14, 2024

View reviewed changes

m-lord-renkse force-pushed the safe-contract branch 3 times, most recently from 38ca09f to e0b47c8 Compare May 14, 2024 14:54

sunce86 removed their assignment May 14, 2024

m-lord-renkse reviewed May 15, 2024

View reviewed changes

m-lord-renkse force-pushed the safe-contract branch from 38193ef to 78f5397 Compare May 15, 2024 10:16

Rework comments

eecd7ea

squadgazzz reviewed May 17, 2024

View reviewed changes