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

suggestion to fix the missing number of confirmations in static void billboard_update(const struct peer *peer) #1780

Closed
renepickhardt opened this issue Jul 30, 2018 · 4 comments
Assignees
Labels
good first issue good for onboarding json-rpc

Comments

@renepickhardt
Copy link
Collaborator

looking at #1778 I found this FIXME tag:

/* FIXME: Say how many blocks to go! */

it asks to make the message for the user more specific by stating how many more confirmations of the funding tx have to happen so that the channel can be used. Afaik the funding tx has to have 6 confirmations. Since I was interacting a lot with the lightningd.sqlite3 I wonder if something along the following line of pseudocode would actually fix this issue:

		// struct bitcoin_txid * funding_tx =&(peer->channel->funding_txid);
		// funding_block_height = select blockheight from transaction where id == funding_tx->shad;
		// current_block_height = select select height from blocks order by height desc Limit 1;
		// confirmations_to_wait = 6 - (current_block_height - funding_block_height);

It seems ugly to me to resolve this via the database and db queries. On the other hand - as a newbe - this is currently the only way I am aware of to gather the necessary information. Hence before providing the patch based on the sql solution I wanted to ask if that would be ok or if there is some preferable way of gathering this information?

@cdecker
Copy link
Member

cdecker commented Jul 31, 2018

The 6 confirmations is only relevant for the channel announcement and serves as a lower bound on the confirmations needed before others will accept a channel_announcement message. The actual number of confirmations needed before the channel becomes operational, i.e., moves into state CHANNELD_NORMAL depends on the minimum_depth in the accept_channel message (see spec). So this is a bit more complicated, but easily cached as well. As a matter of fact, since we have a reference to the struct peer we can easily just go an look at the channel->scid and extract the confirmation height from it (if set) and subtract it from peer->ld->topology->tip->height and compare it with peer->channel->minimum_depth 😉

@cdecker
Copy link
Member

cdecker commented Jul 31, 2018

I took the liberty of assigning you :-)

@renepickhardt
Copy link
Collaborator Author

renepickhardt commented Aug 3, 2018

I tried to fix this. However two problems:

1.) the code looks ugly and I am sure this can be done in a better way?
2.) peer->ld does not exist. I guess that is the reason why it was tagged as FIXME in the first place. The peer struct that we have is defined in the same file and does not contain a pointer to ld also I don't see any other fields from which I could extract the information. There is the message queue with the master deamon but I am pretty sure this should not be resolved by sending messages (and I am not familiar with the message queue system yet)

if we used struct peer from lightningd/peer_control.h it would be ok. c.f.:

struct peer {

here is what the code would look like IF there was an ld-pointer in the peer struct as suggested by @cdecker

else if (!peer->funding_locked[LOCAL] && !peer->funding_locked[REMOTE]){
	u32 minimum_depth = peer->channel->minimum_depth;
	//contains the blockheight of the funding tx. Is NULL if not on chain yet
	char * short_channel_id = peer->channel->scid;
	if (!short_channel_id)
		funding_status = tal_fmt(tmpctx,
				"Funding needs %d more confirmations.",
				minimum_depth);
	else {
		char *ptr;
		ptr = strtok(short_channel_id, ":");
		u32 funding_tx_height;
		sscanf(ptr, "%d", &funding_tx_height);
		u32 current_block_height;
		// THIS LINE WON'T WORK
		current_block_height = peer->ld->topology->tip->height;
		u32 confirmations_needed = minimum_depth - (current_block_height - funding_tx_height);
		if (confirmations_needed == 1)
			funding_status = "Funding needs 1 more confirmation";
		else if (confirmations_needed > 1)
			funding_status = tal_fmt(tmpctx,
								"Funding needs %d more confirmations.",
								confirmations_needed);
		// Something went wrong with calculation do a generic error message
		else
			funding_status = "Funding needs more confirmations.";
	}
}

@trueptolemy
Copy link
Collaborator

This can be closed via #2405 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue good for onboarding json-rpc
Projects
None yet
Development

No branches or pull requests

4 participants