listpeerchannels: split conversion into stages #5825
Merged
Conversation
This file contains 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
rustyrussell
force-pushed
the
guilt/pr-5568
branch
from
7b37e0e
to
6bb311e
Compare
rustyrussell
force-pushed
the
guilt/pr-5568
branch
3 times, most recently
from
f4956d0
to
dc560d7
Compare
Merged
vincenzopalazzo
changed the title
There was a problem hiding this comment.
Sorry for the messy history, I should do a better job with it and make life easier for everybody.
I think the last change that this PR needs is
rm contrib/pyln-testing/pyln/testing/node_pb2.py and make -j$(nproc) check DEVELOPER=1
this will regenerate the python code that we had, maybe this works also without removing the file?
|
OK, I've further diagnosed the bookkeeper issue. What happens (once listpeers is faster due to removal of channels[]):
Before (all settled before unilateral close): After (one settled by unilateral close): |
rustyrussell
force-pushed
the
guilt/pr-5568
branch
from
dc560d7
to
91928a3
Compare
|
I've rebased this onto master; it deprecates the schema and doesn't remove it now. We don't have deprecations for parameter removal, so that still gets removed entirely. |
3 tasks
vincenzopalazzo
previously requested changes
Jan 11, 2023
There was a problem hiding this comment.
Looks like the bookkeeper error is hide under the following one
=========================== short test summary info ============================
FAILED tests/test_connection.py::test_wumbo_channels - KeyError: 'channels'
============ 1 failed, 499 passed, 54 skipped in 1863.02s (0:31:03) ============
I think this is one reason to prioritize this PR and not merge it close to the end of the merge windows, this PR will break many (if not all) others PRs
We're soon going to call json_add_unsaved_channel and json_add_uncommitted_channel from a new place, where we want the peer state directly included. Based on patch by @vincenzopalazzo. Signed-off-by: Rusty Russell <[email protected]>
Changelog-Added: JSON-RPC: new command `listpeerchannels` now contains information on direct channels with our peers. Signed-off-by: Vincenzo Palazzo <[email protected]>
Instead of returning a peers -> channels heirarchy, return (as callers want!) a flat array of channels. This is actually most of the transition work to make them work with listpeerchannels. Signed-off-by: Rusty Russell <[email protected]>
…nels(). This way we always have an SCID and a direction. Signed-off-by: Rusty Russell <[email protected]>
Don't parse the listpeers.channels output ourselves: with two extra fields we can simply reuse json_to_listpeers_channels(). Signed-off-by: Rusty Russell <[email protected]>
Signed-off-by: Rusty Russell <[email protected]>
With the next change (which, as a side-effect, speeds up listpeers), we seem to hit a race in this test. The bookkeeper doesn't get to process the final payment before the node is shutdown. Signed-off-by: Rusty Russell <[email protected]>
Changelog-Deprecated: JSON-RPC: `listpeers` `channels` array: use `listpeerchannels` Signed-off-by: Vincenzo Palazzo <[email protected]>
Some are best copied into the schema, but some are already out-of-date, so cleanest to remove them and rely on the generated (and thus, checked!) fields. Signed-off-by: Rusty Russell <[email protected]>
|
These changes always find things that have changed since I did the initial sweep! No big deal, I'm re-testing now, rebased on master. |
rustyrussell
force-pushed
the
guilt/pr-5568
branch
from
91928a3
to
6b41666
Compare
7 tasks
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 is #5568 split into simple commits. Hopefully this will not just help with review, but with debugging!