-
Notifications
You must be signed in to change notification settings - Fork 83
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
base: settle-queue-to-driver
Are you sure you want to change the base?
Conversation
287ccc1
to
5b27c13
Compare
a5ab68a
to
7bcd92f
Compare
There was a problem hiding this 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.
de75f95
to
2efbcfe
Compare
Oh, that is cool, didn't know about that. Will try to use this instead. |
464b638
to
3705940
Compare
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. |
ad7d020
to
f1bc653
Compare
pub struct SettleOk<'a> { | ||
test: &'a Test, |
There was a problem hiding this comment.
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.
// 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Submit the first settlement. Sleep for a bit to make sure it gets dequeued and it's execution is paused.
- 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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.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 to2
, since 1 ongoing settlement(dequeued) + 2 more in the queue make it3
pending in total, which is high enough IMO.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.