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

Add a status field to listpeers indicating where an HTLC is currently being held #4580

Merged
merged 4 commits into from
Jun 5, 2021

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Jun 3, 2021

Debugging where an HTLC is currently being held can help us identify
plugins that are not behaving correctly. On top of the pure status
field in the htlcs array we also print debug messages when entering
and leaving a plugin hook, so logs are a bit more helpful too.

@cdecker cdecker force-pushed the htlc-tracking branch 2 times, most recently from f944222 to a8d33c6 Compare June 3, 2021 16:35
@cdecker cdecker requested a review from rustyrussell June 3, 2021 16:36
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Seems some uninitialized fields, should be easy to fix?

Comment on lines +58 to +60

/* A simple text annotation shown in `listpeers` */
char *status;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be in the wrong commit?

hin->status =
tal_fmt(hin, "Waiting for the htlc_accepted hook of plugin %s",
plugin->shortname);

Copy link
Contributor

Choose a reason for hiding this comment

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

You presumably should initialize this to NULL everywhere hin is allocated?

@cdecker
Copy link
Member Author

cdecker commented Jun 4, 2021

Got my commits mixed up it seems, I'll reslice them and update here.

We will start annotating some of the in-memory objects with a message
indicating which plugin currently is processing the hook.
@rustyrussell
Copy link
Contributor

Missing regeneration of files, fixed (and trivial rebase on master).

Ack 5dc4b55

@rustyrussell
Copy link
Contributor

Oops, and you freed it without setting to NULL, causing a use-after-free.

diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c
index 65a704023..d0887d650 100644
--- a/lightningd/peer_htlcs.c
+++ b/lightningd/peer_htlcs.c
@@ -1074,7 +1074,7 @@ htlc_accepted_hook_final(struct htlc_accepted_hook_payload *request STEALS)
 	struct htlc_in *hin = request->hin;
 	struct channel *channel = request->channel;
 
-	tal_free(request->hin->status);
+	request->hin->status = tal_free(request->hin->status);
 
 	/* *Now* we barf if it failed to decode */
 	if (!request->payload) {

Annotating the htlc in `listpeers` with their current status, and
which plugin is currently holding on to them with their
`htlc_accepted` hook can help us debug where plugins may go wrong.

Changelog-Added: jsonrpc: HTLCs in `listpeers` are now annotated with a status if they are waiting on an `htlc_accepted` hook of a plugin.
@rustyrussell
Copy link
Contributor

Ack b89ce78

@rustyrussell rustyrussell merged commit 76b8eb3 into ElementsProject:master Jun 5, 2021
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