-
Notifications
You must be signed in to change notification settings - Fork 2.2k
graphdb: add caching for isPublicNode query #10363
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| //go:build test_db_postgres || test_db_sqlite | ||
|
|
||
| package graphdb | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestNodeIsPublicCache verifies that once a node is observed as public, it | ||
| // remains public on cache hit even if it later has zero public channels in the | ||
| // DB. | ||
Abdulkbk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // | ||
| // NOTE: Once a pubkey is leaked, we don't invalidate the key from the cache | ||
| // even if it has no public channels anymore. This is because once leaked as | ||
| // public, the node cannot undo the state. | ||
| func TestNodeIsPublicCache(t *testing.T) { | ||
| t.Parallel() | ||
| ctx := t.Context() | ||
|
|
||
| graph := MakeTestGraph(t) | ||
|
|
||
| alice := createTestVertex(t) | ||
| bob := createTestVertex(t) | ||
| carol := createTestVertex(t) | ||
|
|
||
| require.NoError(t, graph.SetSourceNode(ctx, alice)) | ||
|
|
||
| alice.LastUpdate = nextUpdateTime() | ||
| bob.LastUpdate = nextUpdateTime() | ||
| carol.LastUpdate = nextUpdateTime() | ||
|
|
||
| require.NoError(t, graph.AddNode(ctx, alice)) | ||
| require.NoError(t, graph.AddNode(ctx, bob)) | ||
| require.NoError(t, graph.AddNode(ctx, carol)) | ||
|
|
||
| // Carol has no public channels, so she should be private. | ||
| isPublic, err := graph.IsPublicNode(carol.PubKeyBytes) | ||
| require.NoError(t, err) | ||
| require.False(t, isPublic) | ||
|
|
||
| // Add a public edge so Alice becomes public and is cached as such. | ||
| edge, _ := createEdge(10, 0, 0, 0, alice, bob) | ||
| require.NoError(t, graph.AddChannelEdge(ctx, &edge)) | ||
|
|
||
| isPublic, err = graph.IsPublicNode(alice.PubKeyBytes) | ||
| require.NoError(t, err) | ||
| require.True(t, isPublic) | ||
|
|
||
| // Delete Alice's only public edge. Since we're using a public node | ||
| // cache, Alice is still treated as public until she is evicted from | ||
| // the cache. | ||
| require.NoError(t, graph.DeleteChannelEdges(false, true, edge.ChannelID)) | ||
|
|
||
| isPublic, err = graph.IsPublicNode(alice.PubKeyBytes) | ||
| require.NoError(t, err) | ||
| require.True(t, isPublic) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3925,6 +3925,15 @@ func TestNodeIsPublic(t *testing.T) { | |
| // participant to replicate real-world scenarios (private edges being in | ||
| // some graphs but not others, etc.). | ||
| aliceGraph := MakeTestGraph(t) | ||
|
|
||
| // SQL store caches public nodes and once a node is cached as public, it | ||
| // stays public until eviction/restart. This test asserts | ||
| // public<->private transitions, so it doesn't apply to SQL. | ||
| if _, ok := aliceGraph.V1Store.(*SQLStore); ok { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes let's update the testcase to reflect this new behavior, so we do not special case it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since I'd be adding cache for kvdb, I will leave this as is for now and remove/update this test once that is done so as not to block this PR from getting in. |
||
| t.Skip("Skipping test because SQL backend uses public node " + | ||
| "cache, public status is sticky until eviction") | ||
| } | ||
|
|
||
| aliceNode := createTestVertex(t) | ||
| if err := aliceGraph.SetSourceNode(ctx, aliceNode); err != nil { | ||
| t.Fatalf("unable to set source node: %v", err) | ||
|
|
||
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.
I wonder if we can also add this cache for the KV store, it shouldn't be a big lift but at least it makes the config value more general, because right now kv db node runners would also think they have this feature available. So either we add clear comments that this is a sql feature or we also add it to the kv store which probably isn't a big change and would improve the performance there as well
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.
Sure, will aim for it in a followup but if it's done before this get marged that will be fine too. For now I'll update the note.