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.

Sign up for GitHub

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

Jump to bottom

move the channels in listpeers to listpeerchannels #5568

Closed
wants to merge 4 commits into from
Closed

move the channels in listpeers to listpeerchannels #5568

wants to merge 4 commits into from

Conversation

vincenzopalazzo
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo commented Sep 2, 2022 •
edited
Loading

Now that the release v0.12.x is out I think it is the moment to resurrect #5022 and finish my work there!

Fixes #4729
Override #5022

Todos

  • Add deprecation in the json schema instead of removing the channel from listpeers
  • Fixes some nasty test hide inside the CI
  • Make sure that all the pay logic is sane after the refactoring

@vincenzopalazzo vincenzopalazzo mentioned this pull request Sep 2, 2022
Closed
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 7 times, most recently from 2dd706c to 2d6d978 Compare September 7, 2022 20:02
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 13 times, most recently from 2f0603b to 42cb3db Compare September 26, 2022 16:35
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 8 times, most recently from 7c852a2 to 4f0e902 Compare October 3, 2022 21:39
@rustyrussell rustyrussell marked this pull request as ready for review October 20, 2022 02:39
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 2 times, most recently from 257c7eb to 9aac1d5 Compare October 21, 2022 16:39
@vincenzopalazzo
Copy link
Collaborator Author

I would probably (not now!) have split this into commits like so:

Yes I'm not happy at all with my committed organization, I refactored them several time next time I will try to be more organized on it

@vincenzopalazzo vincenzopalazzo mentioned this pull request Oct 27, 2022
Closed
@cdecker cdecker removed this from the v22.11 milestone Nov 3, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 6 times, most recently from 9288fa8 to 391bc0f Compare November 15, 2022 18:39
@vincenzopalazzo
lightningd: move channel from listpeers to listpeerchannels
eda0070 
Changelog-Added: JSON-RPC: new command `listpeerchannels` now contains information on direct channels with our peers.

Changelog-Deprecated: JSON-RPC: `listpeers` `channels` array: use
`listpeerchannels`

Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
docs: update docs to reflect the new API
0d93f84 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo vincenzopalazzo added this to the v23.02 milestone Dec 12, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the macros/resurrection_listpeers branch 2 times, most recently from ccae4cc to c718593 Compare December 13, 2022 19:33
@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Dec 13, 2022 •
edited
Loading

Ok, I spend some time on this PR in the last few days and I do not find any direct cause of the refactoring that cause the error CI error.

However, from I think that the problem is from this piece of code

https://github.com/ElementsProject/lightning/blob/master/plugins/bkpr/bookkeeper.c#L1661-L1678

But I'm not sure about that because I do not have to much experiences in the bookkeeper code, but I make a nice log filtering that shows that the log is missing a coin_move 2 (invoice) 222msat -0msat channel_mvt 1668546274 event https://hackmd.io/@cln/SkOPjtWXo

The magic command is pytest tests -k test_bookkeeping_closing_subsat_htlcs -s

@vincenzopalazzo
rust: autogenerate code from the new docs
0c9df5a 
Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@rustyrussell rustyrussell mentioned this pull request Dec 15, 2022
Merged
@rustyrussell @vincenzopalazzo
pytest: fix race in test_bookkeeping_closing_subsat_htlcs
33e92e8 
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 <rusty@rustcorp.com.au>
@vincenzopalazzo
Copy link
Collaborator Author

Ok I stole 33e92e8 from @rustyrussell to just give a fresh run inside the CI see if all it is ok with this fix.

@vincenzopalazzo
Copy link
Collaborator Author

closing in favor of #5825

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdecker cdecker Awaiting requested review from cdecker cdecker is a code owner

@rustyrussell rustyrussell Awaiting requested review from rustyrussell

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v23.02
Development

Successfully merging this pull request may close these issues.

listpeers channel list should be split into listpeerchannels, channels[] deprecated.
3 participants
@vincenzopalazzo @rustyrussell @cdecker