Remove auction_transaction #2283

sunce86 · 2024-01-15T11:13:46Z

Description

Exchanges tx_from/tx_nonce for auction_id which can be directly encoded from the settlement transaction since we shipped colocation. This allows us to remove an entire database table and a bunch of other complicated code.

I'm not yet removing the old columns in case something goes bad with this change.

Changes

Removed auction_transaction table.
Redesigned settlements table to contain auction id

How to test

db tests and e2e tests cover the new functionality.
Locally I've tested the db migration file like this:

First comment out the V_58 migration file and apply old schema.
Insert 10 settlements, of which first 5 have auction_transaction record (except one settlement (block_number 3), to simulate the existence of settlement from other environment staging/prod)
Then, manually apply migration script V_58
Observed the settlements table,auction_id were populated as expected.

fleupold

I like the change of the table turning Option<from/nonce> to an Option<AuctionID>. In fact this could probably be its own PR.

However, I think we should explore populating the auction id field already in the event handler that inserts the original settlement (it should have access to all required information). This way we don't need to create the extra "auction_kind" column which I find quite confusing (as the db values don't match the values on the struct) and don't run into the issue of first requiring one event handler to run before the other one does.

I'm not sure what this means for transitioning, but we could for a while have both systems run in parallel (and assert that the upsert doesn't really change any values other than at the moment of the release when it may be catching up with old auctions).

fleupold · 2024-01-22T19:23:45Z

crates/autopilot/src/on_settlement_event_updater.rs

+
+            // Wait a bit more to not race with the event indexer.
+            // Otherwise we might miss event and have to wait for next block to retrigger
+            // loop.
+            tokio::time::sleep(std::time::Duration::from_secs(1)).await;


Relying on a fixed time sleep here is super hacky. I think having one block delay wouldn't be so bad, but if it's a must I think we should combine the two event updater (which one is the other one?) and make sure they are processing events in the order we want them to.

fleupold · 2024-01-22T19:27:39Z

crates/autopilot/src/on_settlement_event_updater.rs

-                Ok(None)
+            Some(score) => {
+                if score.winner.0 != tx_from.0 {
+                    tracing::warn!(


Should this and not being able to recover auction_id from calldata be error logs now (or even return an error)?

fleupold · 2024-01-22T19:34:16Z

database/sql/V058__drop_auction_transaction.sql

+CREATE TYPE AuctionKind AS ENUM ('valid', 'invalid', 'unprocessed');
+
+ALTER TABLE settlements
+    ADD COLUMN auction_kind AuctionKind NOT NULL DEFAULT 'unprocessed',


Are we relying on this default anywhere? If not, can we drop it (I'd rather us use as little fancy SQL features as possible and set all values explicitly in code)

fleupold · 2024-01-22T19:44:05Z

crates/autopilot/src/on_settlement_event_updater.rs

    sqlx::PgConnection,
    web3::types::Transaction,
 };

+#[derive(Debug, Copy, Clone)]
+pub enum AuctionKind {


I'm not a fan of this type. It has the same entropy as an Option<AuctionId> with the added complication that in the database it exists in three flavours (including unprocessed)

sunce86 · 2024-01-22T20:54:34Z

However, I think we should explore populating the auction id field already in the event handler that inserts the original settlement (it should have access to all required information).

I did a detailed exploration of this before starting to work on this issue and it can't be done without extra web3 call to get the transaction and decode the auction_id. The best solution would be to add the auction_id to the settlement event data when the new version of GPv2Settlement contract is deployed.

fleupold · 2024-01-23T08:07:33Z

I did a detailed exploration of this before starting to work on this issue and it can't be done without extra web3 call to get the transaction and decode the auction_id

We are doing this in OnSettlementEventUpdater updater anyways and event processing is already async so I don't think it changes much honestly. In fact we could probably call OnSettlementEventUpdater.update(tx_hash).await with minimal code changes from the existing event processing code today (although the dependency injection would be quite weird).

My suggested plan of action would be:

Change nonce/from for auction ID but keep the current delay (this get rid of a lot of code and is a strict improvement)
Create a new domain component that has the required dependencies (persistence, Ethereum) and implements EventStoring for all events that we care about. Event processing should be domain logic imo.
Move the current "OnSettlementEventUpdater" logic in this component also
Remove 64 block delay and instead invoke update logic when the settlement event is being processed.

Wdyt?

sunce86 · 2024-01-23T09:18:51Z

You can do (1) but you still need to keep the auction_transaction table if you don't want to introduce auction_kind. I mean, you would still need a mechanism to know which settlement you want to process in OnSettlementEventUpdater.

Your plan sounds good but it's more involving and I don't we will make it for zero fee launch.

I personally see benefit in separating settlement event insertion from the rest of the code, since the rest of the code can take additional time, fail, require reindexing in case of bugs etc. But your plan would probably simplify handling events and competition data.

fleupold · 2024-01-24T16:21:32Z

I removed the auction_kind enum and involved migration logic (meaning we still fetch 64 block old auctions without an auction id and amend their information). I plan to tackle those in another PR (first moving event handling into a domain component and then combining the two handlers).

Could I get a review please.

sunce86

Once merged, I think the OnSettlementEventUpdater will try to process the first settlement without auction_id which will be some pre-colocation settlement for which the recovery of auction_id from calldata will fail and it will be stuck.

sunce86 · 2024-01-24T17:03:34Z

crates/database/src/settlements.rs

+        .await
+}
+
+pub async fn data_exists(ex: &mut PgConnection, auction_id: i64) -> Result<bool, sqlx::Error> {


this now means already_processed instead of data_exists, I guess

fleupold · 2024-01-24T17:26:27Z

Once merged, I think the OnSettlementEventUpdater will try to process the first settlement without auction_id which will be some pre-colocation settlement for which the recovery of auction_id from calldata will fail and it will

You are right! Pre-colocation production transactions would fail on staging and vice versa. Do you think hardcoding a min block (past colocation launch) would be an ok workaround for this?

Edit: this would require a different block per chain which is a bit annoying :-(

sunce86 · 2024-01-24T17:32:16Z

this

I think that's ok and we can remove it at a later stage of refactoring.

fleupold · 2024-01-24T17:36:15Z

Would there be an issue with populating a fake auction ID (e.g. -1) for items that are far in the past and cannot be matched?

sunce86 · 2024-01-24T17:44:57Z

Would there be an issue with populating a fake auction ID (e.g. -1) for items that are far in the past and cannot be matched?

I think theoretically this could also happen when solvers start running their own driver (until we have a new smart contract version that enforces the existence of auction_id) so we definitely want to overcome this error and continue processing other settlements.

From the top of my head, I don't think -1 would cause something bad (would have to go through the codebase and double check all JOINs though)

sunce86

Approving with comment (since I cant approve my own PR 😄 )

The only thing I noticed is that we changed the way we handle InvalidSelector error on decoding the transaction, but I think this can't happen after colocation (it could happen before colocation because we saved auction_transaction entry during competition time).

sunce86 added 14 commits January 15, 2024 12:02

Remove auction_transaction

e0f006a

fix issues

4522a1b

Merge branch 'main' into auction-transaction

61a4f9d

fix solver competition test

eabc187

rename file

5171b2c

fix migr

f6eabc7

various fixes

a039a1a

fix cip_20_data_updated

53ce32e

fix cip_20_data_updated

757e76b

revent nonsense

b94835e

fix sql

94beabc

fix query

2f8a070

migration fix

c2e9478

fix syntax errors

9ee0a4f

sunce86 changed the title ~~DRAFT: Remove auction_transaction~~ Remove auction_transaction Jan 19, 2024

sunce86 added 3 commits January 19, 2024 13:21

tests

0346f80

e2e tests

b221e8c

small fixes

fca0198

sunce86 marked this pull request as ready for review January 19, 2024 12:40

sunce86 requested a review from a team as a code owner January 19, 2024 12:40

sunce86 added 3 commits January 19, 2024 14:18

remove mint blocks

062549c

small fixes

f10361d

return oldest unprocessed settlement

f000781

fleupold reviewed Jan 22, 2024

View reviewed changes

fleupold added 2 commits January 24, 2024 17:04

remove auction kind

f6a0152

Merge remote-tracking branch 'origin/main' into auction-transaction

1e684c0

fleupold requested a review from a team January 24, 2024 16:21

sunce86 commented Jan 24, 2024

View reviewed changes

fleupold added 2 commits January 24, 2024 18:15

fix e2e test

bae70f5

rename data_exists

5e4401e

set default auciton id for old settlements from other environment

40138bf

sunce86 commented Jan 25, 2024

View reviewed changes

fleupold approved these changes Jan 26, 2024

View reviewed changes

Merge branch 'main' into auction-transaction

d9f2996

fleupold merged commit 0213ec8 into main Jan 26, 2024
7 of 8 checks passed

fleupold deleted the auction-transaction branch January 26, 2024 09:33

github-actions bot locked and limited conversation to collaborators Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove auction_transaction #2283

Remove auction_transaction #2283

sunce86 commented Jan 15, 2024 •

edited by fleupold

Loading

fleupold left a comment •

edited

Loading

fleupold Jan 22, 2024

fleupold Jan 22, 2024

fleupold Jan 22, 2024

fleupold Jan 22, 2024

sunce86 commented Jan 22, 2024

fleupold commented Jan 23, 2024

sunce86 commented Jan 23, 2024

fleupold commented Jan 24, 2024

sunce86 left a comment

sunce86 Jan 24, 2024

fleupold commented Jan 24, 2024 •

edited

Loading

sunce86 commented Jan 24, 2024

fleupold commented Jan 24, 2024

sunce86 commented Jan 24, 2024

sunce86 left a comment

Remove auction_transaction #2283

Remove auction_transaction #2283

Conversation

sunce86 commented Jan 15, 2024 • edited by fleupold Loading

Description

Changes

How to test

fleupold left a comment • edited Loading

Choose a reason for hiding this comment

fleupold Jan 22, 2024

Choose a reason for hiding this comment

fleupold Jan 22, 2024

Choose a reason for hiding this comment

fleupold Jan 22, 2024

Choose a reason for hiding this comment

fleupold Jan 22, 2024

Choose a reason for hiding this comment

sunce86 commented Jan 22, 2024

fleupold commented Jan 23, 2024

sunce86 commented Jan 23, 2024

fleupold commented Jan 24, 2024

sunce86 left a comment

Choose a reason for hiding this comment

sunce86 Jan 24, 2024

Choose a reason for hiding this comment

fleupold commented Jan 24, 2024 • edited Loading

sunce86 commented Jan 24, 2024

fleupold commented Jan 24, 2024

sunce86 commented Jan 24, 2024

sunce86 left a comment

Choose a reason for hiding this comment

sunce86 commented Jan 15, 2024 •

edited by fleupold

Loading

fleupold left a comment •

edited

Loading

fleupold commented Jan 24, 2024 •

edited

Loading