Skip to content
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

Driver settle queue tests #3133

Open
wants to merge 29 commits into
base: settle-queue-to-driver
Choose a base branch
from

Conversation

squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Nov 22, 2024

Description

An attempt to properly test #3129. The idea is to send multiple /settle requests to the same driver to check that some will be discarded due to the settlement queue limit.

Changes

  • anvil with disabled auto-mining is used to accumulate /settle requests in the queue.
  • Introduced a new TooManyPendingSettlements API error to properly identify the cause. I couldn't find a better way to ensure the logic is correct. Also, it might be helpful for all the parties to understand why a specific solution/settlement was discarded.
  • default_settle_queue_size is reduced to 2, since 1 ongoing settlement(dequeued) + 2 more in the queue make it 3 pending in total, which is high enough IMO.
  • Removed lifetime params from the Settle struct, which stores the /settle endpoint response data in order to fix a compilation issue. More details: Driver settle queue tests #3133 (comment)

How to test

This is the test.

@squadgazzz squadgazzz changed the base branch from main to settle-queue-to-driver November 22, 2024 15:12
@squadgazzz squadgazzz force-pushed the driver-settle-queue-tests branch from 287ccc1 to 5b27c13 Compare November 22, 2024 15:13
@squadgazzz squadgazzz marked this pull request as ready for review November 22, 2024 18:11
@squadgazzz squadgazzz requested a review from a team as a code owner November 22, 2024 18:11
@squadgazzz squadgazzz force-pushed the driver-settle-queue-tests branch from a5ab68a to 7bcd92f Compare November 22, 2024 18:14
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the ethrpc configurations seems weird to make this test work.
Did you consider using anvil's helper methods instead?
You could first disable auto mining and manually clear the mempool to make sure that transactions actually do not get mined so that the submission futures don't return.

crates/driver/src/tests/cases/settle.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/cases/settle.rs Show resolved Hide resolved
@squadgazzz squadgazzz force-pushed the driver-settle-queue-tests branch 2 times, most recently from de75f95 to 2efbcfe Compare November 28, 2024 10:26
@squadgazzz
Copy link
Contributor Author

You could first disable auto mining and manually clear the mempool to make sure that transactions actually do not get mined so that the submission futures don't return.

Oh, that is cool, didn't know about that. Will try to use this instead.

@squadgazzz squadgazzz marked this pull request as draft November 28, 2024 11:32
@squadgazzz squadgazzz marked this pull request as ready for review November 30, 2024 20:18
@squadgazzz squadgazzz marked this pull request as draft November 30, 2024 20:18
@squadgazzz squadgazzz force-pushed the driver-settle-queue-tests branch from 464b638 to 3705940 Compare December 2, 2024 17:02
Copy link

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

@squadgazzz squadgazzz force-pushed the driver-settle-queue-tests branch from ad7d020 to f1bc653 Compare December 16, 2024 11:41
Comment on lines -1520 to -1521
pub struct SettleOk<'a> {
test: &'a Test,
Copy link
Contributor Author

@squadgazzz squadgazzz Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the lifetime param is a workaround to fix the compilation issue(link) that occurred when trying to execute /settle calls asynchronously here: https://github.com/cowprotocol/services/pull/3133/files#diff-8534e72b43bec3cc35e3a704050781525cf44370eb548da8d5ae8ba3fb2b06ebR161-R173

The problem arose when I added tokio::spawn to start executing the settlements in parallel. To make any progress with them, I have to mine a new block manually only after those settlements are received by the driver and before awaiting for results. I tried multiple options, but this one requires fewer changes so far.

@squadgazzz squadgazzz marked this pull request as ready for review December 16, 2024 12:30
crates/driver/src/tests/setup/mod.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/cases/settle.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/cases/settle.rs Outdated Show resolved Hide resolved
crates/driver/src/tests/cases/settle.rs Outdated Show resolved Hide resolved
Comment on lines 193 to 196
// There is a high possibility of a race condition where the first settlement
// request is dequeued at an unpredictable time. Therefore, we can't guarantee
// the order of the errors received, but their count must be persistent.
assert!(queue_is_full_err_count == 2 && failed_to_submit_err_count == 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request queue gets flushed whenever a request exceeds the related submission deadline, right?
Isn't that enough to make this more predictable? I'd like to better understand why we apparently can't fix this race condition altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how this is going to help here. But I found a way how to avoid race conditions:

  1. Submit the first settlement. Sleep for a bit to make sure it gets dequeued and it's execution is paused.
  2. Submit all the remaining settle requests.

);

// Now we send the last settlement request. It fails because with the current
// framework setup it is impossible to settle the same solution twice.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we call /settle() twice for the same ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment was incorrect. Only 1 order gets created. So there is no way to settle different solutions for the same order more than once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants