routing: Namespaced Mission Control#9001
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
1658430 to
dc718ac
Compare
dc718ac to
cef2379
Compare
|
Note: this is currently stacked on top of #8911 just to make my life easier for building on top of these 2 together but there is no actual dependency on that PR. So if this gets approved before that one, I can switch the order of dependencies (although, I think performance wise, it makes sense to get #8911 in first since it will make the size of the result blobs way smaller). |
cef2379 to
200547c
Compare
bbea090 to
8e4c0e5
Compare
1c0bba2 to
8af0803
Compare
5c444a6 to
a996efe
Compare
8af0803 to
3cf0d5f
Compare
a996efe to
c2195f1
Compare
guggero
left a comment
There was a problem hiding this comment.
Very nice! Only small comments, everything's very straightforward 🚀
c2195f1 to
6d320d1
Compare
3cf0d5f to
9d45dc9
Compare
9691ae5 to
0582ce4
Compare
ellemouton
left a comment
There was a problem hiding this comment.
Wohoooo thanks @guggero 🚀
guggero
left a comment
There was a problem hiding this comment.
LGTM and tACK 🎉
Ran this on a copy of my mainnet node with 474 entries, migration was super fast (for both migration 32 and 33):
2024-09-16 13:00:12.172 [INF] CHDB: Checking for schema update: latest_version=33, db_version=31
2024-09-16 13:00:12.172 [INF] CHDB: Performing database schema migration
2024-09-16 13:00:12.172 [INF] CHDB: Applying migration #32
2024-09-16 13:00:12.172 [INF] CHDB: Migrating Mission Control store to use a more minimal encoding for routes
2024-09-16 13:00:12.192 [INF] CHDB: Applying migration #33
2024-09-16 13:00:12.192 [INF] CHDB: Migrating Mission Control store to use namespaced results
2024-09-16 13:00:12.200 [INF] CHDB: Checking for optional update: prune_revocation_log=false, db_version=0: prune revocation log
2024-09-16 13:00:12.201 [INF] LTND: Database(s) now open (time_to_open=1.772583823s)!
...
2024-09-16 13:00:16.431 [DBG] CRTR: Instantiating mission control with config: maximum history: 1000, minimum failure relax interval: 1m0s, estimator type: apriori, penalty halflife time: 1h0m0s, apriori hop probability: 0.6, apriori weight: 0.5, previous success probability: 0.95, capacity fraction: 0.9999
2024-09-16 13:00:16.432 [INF] CRTR: Loaded 474 mission control entries
2024-09-16 13:00:16.432 [DBG] CRTR: [default]: Mission control state reconstruction started
2024-09-16 13:00:16.437 [DBG] CRTR: [default]: Reporting pair success to Mission Control: pair=0256812a1cb2539a5500f3c1c20db5ed7626e3878a552e3356c032c7bec2b3060e -> 03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f, amt=75403539 mSAT
2024-09-16 13:00:16.437 [DBG] CRTR: Setting 0256812a1cb2539a5500f3c1c20db5ed7626e3878a552e3356c032c7bec2b3060e->03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f range to [75403539 mSAT-0 mSAT]
2024-09-16 13:00:16.437 [DBG] CRTR: [default]: Reporting pair success to Mission Control: pair=03864ef025fde8fb587d989186ce6a4a186895ee44a926bfc370e2c366597a3f8f -> 032795584d041034d0daec6c048be786bb566282a9e275ca6b22085598dc6db71e, amt=75395000 mSAT
...
|
@bitromortac: review reminder |
bitromortac
left a comment
There was a problem hiding this comment.
LGTM ⚡ This will be very useful, do you plan to add RPC functionality to be able to switch to different mission control namespaces? SetMissionControlConfig could be great for that.
Yes that is defs a next step but I think let's not do it in this PR just to keep this minimal for now thanks for the review! |
So that `missionControlStore` can be unaware of the backing DB structure it is writing to. In an upcoming commit when we change mission control to write to namespaced buckets instead, we then only need to update the `namespacedDB` implementation.
In this commit, the mission control store is migrated such that all existing pairs which are currently stored directly in the top level results bucket are now instead moved to a "default" namespace bucket. Note that this migration is not yet invoked in this commit. The migration will be invoked in the same commit that starts writing and reading the new format.
and invoke the associated mission control migration.
Only the MissionControl instance should use this variable and it should not be accessible to users of MissionControl.
0582ce4 to
f687b10
Compare
f687b10 to
7c20895
Compare
This commit renames the previous MissionControl to MissionController and the previous MissionController interface to MissionControlQuerier. This is done because soon the (new) MissionController will back multiple namespaced MissionControl instances. For now, it just houses a single MissionControl in the default namespace. This commit also replaces the MissionControl's `now` function with a `clock.Clock`.
7c20895 to
5bfc236
Compare
5bfc236 to
83c1d73
Compare
This PR:
With this PR we still only use the default namespace, but this is a starting point for starting to make use of other namespaces. A follow up could add functionality to combine the results of 2 or more on-disk namespaces into one in-memory.
Required for #8991 where we will want to report MC results in a different (non payment related) namespace.
Addresses part of #7812
Addresses part of #8849
Depends on #8911
Visual
MC DB structure currently:
MC DB structure after this PR: