[graph-work-side-branch]: side branch for graph work #9692
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
Pull reviewers statsStats of the last 30 days for lnd:
|
|
^ purely a rebase on master |
|
^ rebase on master for inclusion of #9789 |
|
^ rebase to include #9798 |
|
^ rebase to include #9804 |
We want `context.TODO()` to be high signal in the code-base. It should signal clearly that work is required to thread parent context through to the call-site. So to keep the signal-to-noise ratio high, we remove any context.TODO() calls from tests since these will never need to be replace by a parent context.
We want `context.TODO()` to be high signal in the code-base. It should signal clearly that work is required to thread parent context through to the call-site. So to keep the signal-to-noise ratio high, we remove any context.TODO() calls from tests since these will never need to be replace by a parent context. After this commit, there is only a single context.TODO() left in the code-base.
In preparation for starting to thread a single parent context through LND, we update the main `server.Start` method to take a context so that it can later pass it to any subsytem's Start method it calls. We also pass the context to `newServer` since it makes some calls that will eventually reach the DB (for example the graph db).
Pass the parent LND context to the gossiper, let it derive a child context that gets cancelled on Stop. Pass the context through to any methods that will eventually thread it through to any graph DB calls. One `context.TODO()` is added here - this will be removed in the next commit. NOTE: for any internal methods that the context gets passed to, if those methods already listen on the gossiper's `quit` channel, then then don't need to also listen on the passed context's Done() channel because the quit channel is closed at the same time that the context is cancelled.
And remove a context.TODO() that was added in the previous commit.
The `GossiperSyncer` makes various calls to the `ChannelGraphTimeSeries` interface which threads through to the graph DB. So in preparation for threading context through to all the methods on that interface, we update the GossipSyncer accordingly by passing contexts through. Two `context.TODO()`s are added in this commit. They will be removed in the upcoming commits.
Here, we remove one context.TODO() by threading a context through to the SyncManager.
With this, we move a context.TODO() out of the gossiper and into the brontide package - this will be removed in a future PR which focuses on threading contexts through that code.
Since the ChannelGraphBootstrapper implementation makes a call to the graph DB.
For any method that takes a context that has a select that listens on the systems quit channel, we should also listen on the ctx since we should not need to worry about if this context is derived internally or externally.
This commit cleans up the graph test code by removing unused kvdb type parameters from the `createTextVertex` and `createLightningNode` helper methods. We also pass in the testing parameter now so that we dont need to check the error each time we call `createTestVertex`.
Remove the kvdb.Backend parameter from the `createChannelEdge` helper. This is all in preparation for having the unit tests run against any DB backend.
Remove unused kvdb.Backend param from `randEdgePolicy` and `newEdgePolicy` test helpers.
Replace all tests calls to the private `forEachNode` method on the `KVStore` with the exported ForEachNode method. This is in preparation for having the tests run against an abstract DB backend.
The `GraphSource` interface in the `autopilot` package is directly implemented by the `graphdb.KVStore` and so we will eventually thread contexts through to this interface. So in this commit, we start updating the autopilot system to thread contexts through in preparation for passing the context through to any calls made to the GraphSource. Two context.TODOs are added here which will be addressed in follow up commits.
Remove one context.TODO and add one more.
Continue threading context through the autopilot system and remove the remaining context.TODOs.
So we can use`require.Equal` for the ChannelEdgeInfo type.
In preparation for our SQL Graph store which wont explicitly store the chain hash but will instead obtain it from the runtime config, we replace the test chainhash value with that of the mainnet genesis hash.
Instead of returning an error and needing to call `require.NoError` for each call to `MakeTestGraph`, rather just used the available testing variable to require no error within the function itself.
To clearly separate the mission control DB store (which for now will remain kvdb only) from the graph store, we rename the `graphBackend` variable to `mcBackend` in the `testGraphInstance` struct.
Currently none of the calls to MakeTestGraph make use of the KVStoreOptionModifier options but later on we will want to make use of the `WithUseGraphCache` ChannelGraphOption and so we take this opportunity to switch out the functional parameters that the helper function takes.
In this commit, we unify how all unit tests that make use of the graph create their test ChannelGraph instance. This will make it easier to ensure that once we plug in different graph DB implementations, that all unit tests are run against all variants of the graph DB. With this commit, `NewChannelGraph` is mainly only called via `MakeTestGraph` for all tests _except_ for `TestGraphLoading` which needs to be able to reload a ChannelGraph with the same backend. This will be addressed in a follow-up commit once more helpers are defined. Note that in all previous packages where we created a test graph using `kvdb.GetBoltBackend`, we now need to add a `TestMain` function with a call to `kvdb.RunTest` since the `MakeTestGraph` helper uses `GetTestBackend` instead of `kvdb.GetBoltBackend` which requires an embedded postgres instance to be running.
In this commit, we add a `test_kvdb.go` file with a single definition of the `NewTestDB` function. A new version of `MakeTestGraph` (called `MakeTestGraphNew` is added which makes use of this `NewTestDB` function to create the backing `V1Store` passed to the `ChannelGraph` for tests. Later on, we will add new implementations of this method backed by sqlite and postgres. When those are added, then build flags will control which version of `NewTestDB` is called. With this change, the only test call-site of `NewKVStore` is the new `test_kvdb.go` file.
Update the test methods to take a `testing.TB` param instead of a `*testing.T` so that the test functions can be used for all the graph's tests including benchmark tests. We also add a temporary replace so that we can make use of these changes and also since we will soon be adding graph related schemas and queries in this submodule that we'll want to have access to.
In this commit, we implement the postgres and sqlite versions of the NewTestDB function. We add the various build flags so that only one of the three versions of this function can be active at a time. We also introduce the SQLStore struct which is the SQL implementation of the V1Store interface. NOTE: it currently temporarily embeds the KVStore struct so that we can implement the V1Store interface incrementally. For any method not implemented, things will fall back to the KVStore. This is ONLY the case for the time being while this struct is purely used in unit tests only. Once all the methods have been implemented, the KVStore field will be removed from the SQLStore struct.
Add `migrations_dev.go` and `migrations_prod.go` files which each define a `migrationAdditions` slice to be appended to the `migrationConfig` slice. The `migrations_dev.go` file is only built if either the `test_db_postgres` or `test_db_sqlite` build flags are used. This slice will be used to add any migrations that are still under development that we want access to via unit tests and itests but do not want to expose to our release build.
Expand an existing test for ForEachNodeDirectedChannel so that it also tests the DB method and not just the ChannelGraph method which will use the in-memory graph cache for the query.
Here we expand TestEdgeInsertionDeletion to assert that the expected error is returned when AddChannelEdge is called for a channel that has already been persisted. We also take the opportunity to convert some of the error checks in the test to use strict error type matching. Finally, we test that the expected error is returned if DeleteLightningNode is called for a node that is no longer in the DB.
In this commit we add more test coverage for the persistence of the addresses of a LightningNode. This is so that we have unit test coverage that ensures that all the various address types can be persisted and that the order of the addresses (within a type) are preserved.
In preparation for using the same logic for non-bbolt backends, we adapt the batch.Schedular to be more generic. The only user of the scheduler at the moment is the KVStore in the `graph.db` package. This store instantiates the bbolt implementation of the scheduler.
Here we add a new BenchmarkBoltBatching test that helps us benchmark the performance when writing to a bbolt backend using various different configurations. We test using N txs for N writes, 1 tx for N writes and then various configurations when using the TimeScheduler.
In this commit, we update the batch schedular so that it has the ability to do read-only calls. It will do a best effort attempt at keeping a transaction in read-only mode and then if any requests get added to a batch that require a read-write tx, then the entire batch's tx will be upgraded to use a read-write tx.
ellemouton
changed the title
guggero
approved these changes
May 23, 2025
There was a problem hiding this comment.
Verified the full rebased diff by applying all the PRs on top of master.
#9791 was missing in the list, added it.
With that, everything is identical.
Also re-ran the "check commits" step locally, all commits compile successfully.
Awesome work @ellemouton 💯
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR shows all the work merged into the
elle-graphside branch. This will be a mixture of the remaining work from the Graph Abstraction project along with some of the incoming native SQL graph migration work (mainly clean-up/refactor prep work).We can merge this into master once v0.19 has dropped. I'll leave it in draft until then.
This PR has the following PRs merged in:
context.TODO#9681kvdbparameters from exported methods #9695Startmethods #9704V1Storeinterface #9791