Skip to content

improvement(ordermatch): split trie from orderbook to reduce pre-propagation locking#2661

Merged
shamardy merged 15 commits intodevfrom
fix/split-orderbook-trie-store
Nov 5, 2025
Merged

improvement(ordermatch): split trie from orderbook to reduce pre-propagation locking#2661
shamardy merged 15 commits intodevfrom
fix/split-orderbook-trie-store

Conversation

@shamardy
Copy link
Collaborator

@shamardy shamardy commented Oct 21, 2025

This PR removes the maker pubkeys trie state and the Patricia-trie memory db out of Orderbook into a dedicated trie store to reduce Orderbook locking time and let messages propagate before trie work.

Why

  • Avoid holding the orderbook/index lock while performing trie mutations.
  • Improve timeliness of order create/update/cancel and keep-alive propagation.

What changed

  • Orderbook keeps only the in-memory index.
  • New TrieStore owns pubkeys_state and the Patricia-trie MemoryDB.
  • Trie mutations are built as TrieOp types under the index lock then applied by a separate worker.
  • Topic subscriptions were moved from Orderbook into OrdermatchContext.

Compatibility

  • No RPC or P2P wire-format changes.
  • No configuration or migration required.

Effect

  • Shorter critical sections and earlier orderbook message propagation, less lock contention under load.

Future Work (Only suggestions)

  • Make the trie store lock per pubkey pair similar to how we handle maker orders in the maker context.
  • Shard Orderbook index by pair.
  • Look into more improvements to reduce locking.

@shamardy shamardy changed the title improvement(ordermatch): split trie from orderbook to reduce locking before propagation improvement(ordermatch): split trie from orderbook to reduce pre-propagation locking Oct 21, 2025
@onur-ozkan onur-ozkan self-requested a review October 22, 2025 10:51
@shamardy shamardy requested a review from dimxy November 4, 2025 11:53
Copy link

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

The PR description makes sense, but, doing all of them once in this PR alone causes massive change in a single module (lp_ordermatch) and it makes it difficult to review and understand them in a short time (it would be way easier/possible for me to review this if we could do this refactoring gradually in multiple PRs). Should we pass this QA and let them test it in detail instead?

@shamardy
Copy link
Collaborator Author

shamardy commented Nov 5, 2025

Should we pass this QA and let them test it in detail instead?

Sure, it's already been tested extensively by @cipig for a few weeks. I will just fix your notes @onur-ozkan and merge this unless @dimxy have more comments.

dimxy
dimxy previously approved these changes Nov 5, 2025
Copy link
Collaborator

@dimxy dimxy left a comment

Choose a reason for hiding this comment

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

LGTM
I'd suggest yet to start breaking lp_ordermatch.rs apart,
e.g. move TrieStore into new file.

@shamardy
Copy link
Collaborator Author

shamardy commented Nov 5, 2025

I'd suggest yet to start breaking lp_ordermatch.rs apart,
e.g. move TrieStore into new file.

Totally agree but it will introduce lots of conflicts in other open PRs. We can do that once our backlog is cleared.

@shamardy shamardy merged commit 544c028 into dev Nov 5, 2025
19 of 26 checks passed
@shamardy shamardy deleted the fix/split-orderbook-trie-store branch November 5, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants