Await for a settlement tx in autopilot #2630
Conversation
crates/autopilot/src/run_loop.rs
Outdated
| let start_offset = MAX_REORG_BLOCK_COUNT; | ||
| let max_blocks_wait = 20; | ||
| let current = eth.current_block().borrow().number; | ||
| let start = current.saturating_sub(start_offset); | ||
| let deadline = current.saturating_add(max_blocks_wait); | ||
| tracing::debug!(%current, %start, %deadline, ?tag, "waiting for tag"); | ||
| let mut seen_transactions: HashSet<H256> = Default::default(); |
There was a problem hiding this comment.
This logic is inspired/copied from this PR: #1135
Which was already explained in the comment: https://github.com/cowprotocol/services/pull/1135/files#r1088803161
squadgazzz
force-pushed
the
2609/autopilot-tracks-for-settlements
branch
from
97fc21a to
b1bc38d
Compare
|
The following config now controls both response awaiting and blockchain monitoring tasks. services/crates/autopilot/src/arguments.rs Lines 170 to 179 in b1bc38d |
| /// | ||
| /// Returns None if no transaction was found within the deadline or the task | ||
| /// is cancelled. | ||
| async fn wait_for_settlement_transaction( |
There was a problem hiding this comment.
Is there a way to test this in e2e or unit test?
There was a problem hiding this comment.
Actually, I couldn't find a quick way to achieve that. That would require either an artificial delay in the driver's response or creating unit tests with mocks. Someone may have a better idea of how to test it.
crates/autopilot/src/run_loop.rs
Outdated
| let max_blocks_wait = self.max_blocks_wait; | ||
| let (cancellation_tx, cancellation_rx) = watch::channel(false); | ||
| let settlement_tx_wait_task = tokio::spawn(async move { | ||
| let tag = auction_id.to_be_bytes(); |
There was a problem hiding this comment.
According to the
squadgazzz
force-pushed
the
2609/autopilot-tracks-for-settlements
branch
from
f953bcb to
11cda11
Compare
crates/autopilot/src/run_loop.rs
Outdated
| } | ||
| let Ok(mut hashes) = self | ||
| .persistence | ||
| .recent_settlement_tx_hashes(start..deadline + 1) |
There was a problem hiding this comment.
I like that you are using the existing event indexing instead of trying to inspect new blocks consecutively (which makes the code simpler)
However, the settlements table also contains the auction_id as a column. It might require exposing a new method on persistence, but I think it would be even simpler and more coherent to just see if we can fetch a settlement by auction and return its tx_hash.
There was a problem hiding this comment.
How come I missed that column 🙈 Thanks, updated the PR.
There was a problem hiding this comment.
LG.
We can improve by not trusting the driver in cases when driver returns success, but can be done in a separate PR.
Also we might reconsider dropping the tx_hash as a response from settle since it's not used for anything important in the competition (afais, maybe we can redesign the inflight orders).
| tokio::time::sleep(Duration::from_secs(5)).await; | ||
| } | ||
| Err(SettleError::Failure(anyhow::anyhow!( | ||
| "settlement transaction await reached deadline" |
There was a problem hiding this comment.
nit: we could define a separate variant Deadline instead of anyhow error.
Description
This PR introduces improvements to Autopilot's process of waiting for tx hashes from the driver after a settlement. Currently, the Autopilot passively awaits the
tx_hashfrom the driver, which can introduce unnecessary delays, particularly in a colocated environment where the driver might not immediately report the hash.Changes
How to test
Current e2e tests. A unit test can be created, but that would require refactoring to introduce mocks. Worth a separate PR.
Related issues
Fixes #2608, #2609