Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

core, orderwatch, meshdb: Implement a dynamically decreasing max expiration time for orders #450

Merged
merged 40 commits into from
Oct 24, 2019

Conversation

albrow
Copy link
Contributor

@albrow albrow commented Oct 16, 2019

Fixes #431.

This PR is still a WIP but I'm opening it now so we can discuss the progress so far. This is an implementation of the idea we've been discussing on Slack and a solution to the problem of finite database storage. To quickly re-summarize the idea:

When storage space is plentiful, we start without any kind of limit on expiration time. As the database fills up, we enforce a limit on the maximum expiration time. Any orders that expire after that time will be removed. If more orders keep coming in we can lower the expiration time and remove more orders.

So far this PR adds an index on ExpirationTime and implements a new method called TrimOrdersByExpirationTime which contains the bulk of the core logic for this feature. There are still some challenges around how to use this function and when to call it, but the approach is looking very feasible as of now. We also still need to determine when and how quickly to increase the max expiration time after storage frees up.

@albrow albrow force-pushed the feature/dynamic-max-expiration branch from 48fcd11 to f5896bb Compare October 16, 2019 23:47
constants/constants.go Outdated Show resolved Hide resolved
constants/constants.go Outdated Show resolved Hide resolved
core/core.go Outdated Show resolved Hide resolved
zeroex/order.go Outdated Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
@fabioberger fabioberger force-pushed the feature/dynamic-max-expiration branch from a11d5aa to 6eb6010 Compare October 18, 2019 04:34
@albrow albrow force-pushed the feature/dynamic-max-expiration branch 2 times, most recently from 511dc61 to 3c32584 Compare October 21, 2019 22:52
browser/ts/index.ts Outdated Show resolved Hide resolved
@albrow albrow force-pushed the feature/dynamic-max-expiration branch 2 times, most recently from 6d02665 to b7d3fa6 Compare October 22, 2019 17:52
@albrow albrow changed the title WIP: Implement a dynamically decreasing max expiration time for orders Implement a dynamically decreasing max expiration time for orders Oct 22, 2019
@albrow albrow changed the title Implement a dynamically decreasing max expiration time for orders core, orderwatcher: Implement a dynamically decreasing max expiration time for orders Oct 22, 2019
@albrow albrow changed the title core, orderwatcher: Implement a dynamically decreasing max expiration time for orders core, orderwatch: Implement a dynamically decreasing max expiration time for orders Oct 22, 2019
@albrow albrow changed the title core, orderwatch: Implement a dynamically decreasing max expiration time for orders core, orderwatch, meshdb: Implement a dynamically decreasing max expiration time for orders Oct 22, 2019
@albrow
Copy link
Contributor Author

albrow commented Oct 22, 2019

@fabioberger Removed WIP tag. This PR is ready for final review.

I do eventually want to optimize our SlowCounter implementation to reduce memory allocations and math/big operations. Our current strategy of using a polling loop inside of OrderWatcher to check if the max expiration time can be increased is also inefficient, but should be good enough for now.

@albrow
Copy link
Contributor Author

albrow commented Oct 22, 2019

Just reminding myself to update the CHANGELOG after we include everything there for the upcoming 5.1.0-beta launch.

Update: I added this.

@albrow albrow force-pushed the feature/dynamic-max-expiration branch from 963aa94 to 4eded3b Compare October 22, 2019 22:56
@albrow albrow force-pushed the feature/dynamic-max-expiration branch from ddae068 to a5bfe82 Compare October 23, 2019 23:58
browser/ts/index.ts Outdated Show resolved Hide resolved
meshdb/meshdb_test.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
// Final expiration time check before inserting the order. We might have just
// changed max expiration time above.
if orderInfo.SignedOrder.ExpirationTimeSeconds.Cmp(w.maxExpirationTime) == 1 {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this mean that the node operator won't get the STOPPED_WATCHING event emitted?

Copy link
Contributor Author

@albrow albrow Oct 24, 2019

Choose a reason for hiding this comment

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

@fabioberger Yes but it also means they never received the ADDED event, which seems consistent to me. What do you think is the right behavior here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since we ACKd the incoming JSON-RPC request, we should return STOPPED_WATCHING.

zeroex/orderwatch/order_watcher.go Show resolved Hide resolved
zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
zeroex/orderwatch/slowcounter/slow_counter.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fabioberger fabioberger left a comment

Choose a reason for hiding this comment

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

Small typo but otherwise LGTM!

zeroex/orderwatch/order_watcher.go Outdated Show resolved Hide resolved
@albrow albrow merged commit 97c1127 into development Oct 24, 2019
@albrow albrow deleted the feature/dynamic-max-expiration branch October 24, 2019 22:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants