Skip to content
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

Fix sql nodes table (again) #6068

Merged

Conversation

rustyrussell
Copy link
Contributor

It wasn't updating, and thus failing locally: perhaps the test is not working properly under valgrind?

As soon as we apply the next commit, we get a new problem: the
delete code didn't work.

Signed-off-by: Rusty Russell <[email protected]>
Without this patch, we only ever loaded the "nodes" table once, then
didn't see updates.

How this ever got past CI is a mystery; perhaps valgrind was so slow that
the updated node_announcement hit the gossmap before we even asked sql
on l3 about the nodes table?

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Fixed: Plugins: `sql` nodes table now gets refreshed when gossip changes.
@endothermicdev
Copy link
Collaborator

How this ever got past CI is a mystery; perhaps valgrind was so slow that the updated node_announcement hit the gossmap before we even asked sql on l3 about the nodes table?

I'm guessing it has to do with the default wait_for timeout of 60s (or 180s in the case of SLOW_MACHINE.) That would have allowed time for the gossmap to update most of the time. Maybe we explicitly add a shorter timeout here to test the plugin instead of testing gossmap. We still can't eliminate gossmap's influence entirely.

Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

ACK 50dfa02

@endothermicdev endothermicdev added this to the v23.02.1 milestone Mar 6, 2023
@endothermicdev endothermicdev merged commit 1e2bc66 into ElementsProject:master Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants