openingd/: Fail fundchannel_start if we already are, or will become, the fundee.

[ZmnSCPxj]

Fixes: #4108

Requesting review from nifty and rusty, since my grasp of the openingd is not good.

As well, there may be a similar issue in dual-funding, so please consider as well for that point-of-view, I am much more ignorant about the dual-funding flow than openingd flow.

[niftynei]

Nice debugging work on this! You don't need a new wire call to fix this. We already do a similar/exact call a few lines above, opening_got_offer, which is used for the openchannel hook. We should toggle this flag there.

[niftynei]

Having a separate declaration from implementation is only kosher for recursive functions. It unnecessarily confuses ctag jumps in vim and clutters the code. Either move opening_fundee_started below the function you're using or move the other up here.

[ZmnSCPxj]

I think opening_fundee_started is just fine where it is, and the other one is also just fine where it is. A small declaration to me is just small and easy to skip over with proper folding. shrug.

[ZmnSCPxj]

Also, modern vim understands etags just fine and we have a make TAGS that uses etags, declarations do not confuse etags the way they confuse ctags. ctags is really just a lousy beta-level software, use etags instead. Twisting your codebase because of lousy tools is worse than just not having that tool.

[rustyrussell]

@niftynei is right here; C is always written bottom to top. Sometimes judicious use of pre-declaration can reduce diff size, but best practice is usually a separate preparatory commit to reorder.

[ZmnSCPxj]

The reason why I did not reuse got_offer is because of this:

lightning/openingd/openingd.c

Lines 965 to 972 in 60be62a

	/* If they give us a reason to reject, do so. */
	if (err_reason) {
	u8 *errmsg = towire_errorfmt(NULL, &state->channel_id,
	"%s", err_reason);
	sync_crypto_write(state->pps, take(errmsg));
	tal_free(err_reason);
	return NULL;
	}

We would need to add more code in lightningd to clear the is_fundee/got_offer flag in case we end up rejecting it, I think (not sure, opening flow is unfamiliar to me).

Looking over at lightningd/opening_control.c, it seems to me I have to set the got_offer flag after this check, so if we got into this part of the code, we reject before we set the got_offer flag:

lightning/lightningd/opening_control.c

Lines 729 to 736 in 60be62a

	/* Tell them they can't open, if we already have open channel. */
	if (peer_active_channel(uc->peer)) {
	subd_send_msg(openingd,
	take(towire_openingd_got_offer_reply(NULL,
	"Already have active channel", NULL)));
	return;
	}

Then later, if a plugin tells us to fail it, we should add a check in this function:

lightning/lightningd/opening_control.c

Lines 625 to 644 in 60be62a

	static void
	openchannel_hook_final(struct openchannel_hook_payload *payload STEALS)
	{
	struct subd *openingd = payload->openingd;
	const u8 *our_upfront_shutdown_script = payload->our_upfront_shutdown_script;
	const char *errmsg = payload->errmsg;
	/* We want to free this, whatever happens. */
	tal_steal(tmpctx, payload);
	/* If openingd went away, don't send it anything! */
	if (!openingd)
	return;
	tal_del_destructor2(openingd, openchannel_payload_remove_openingd, payload);
	subd_send_msg(openingd,
	take(towire_openingd_got_offer_reply(NULL, errmsg,
	our_upfront_shutdown_script)));
	}

Unfortunately, it seems we do not have access to the struct uncommitted_channel here to clear the got_offer flag, so I should modify openchannel_hook_payload to include that, is that correct?

[ZmnSCPxj]

Removed the new wire. Cancel a conflicting fundchannel_start after plugins all agree everything is kosher. Renamed is_fundee to got_offer. Did not move function just to avoid pre-declaration, I think avoiding pre-declaration at all costs leads to awful code layout where later-executed functions are read first before earlier-executed functions, I think the minor cost of a few lines for pre-declaration is far cheaper than the mental gymnastics needed to understand code written in a non-straightforward order. If I had moved opening_funder_failed_cancel_commands above opening_funder_failed just to avoid a pre-declaration, then it would have obscured that the new function is just the latter part of the original single function with no modifications.

[niftynei]

So, not to get too "putting on my arm-chair psychologist hat" here, but as you did bring up the topic of "mental gymnastics" I think we should discuss the laundry list of rationales you've given so far as to why it's ok to disregard the request to do the reorg needed to remove the static function declaration.

Rationales offered:

• "opening_fundee_started is just fine where it is"
• "the other one is also just fine where it is"
• "A small declaration to me is just small and easy to skip over"
• "ctags is really just a lousy beta-level software"
• "Twisting your codebase because of lousy tools is worse than just not having that tool."
• "I think avoiding pre-declaration at all costs leads to awful code layout"
• "the minor cost of a few lines for pre-declaration is far cheaper than the mental gymnastics needed"

These are all stated in the pull request comments. However, I find it peculiar that none of these are mentioned in the actual code comment you added (and vice versa, that the reason given there isn't mentioned elsewhere), which is

/* These two functions used to be a single function, pre-declaring
 * here in order to reduce review load since the original function
 * is not really changed at all in its operation, just need access
 * to the cleanup part in a separate piece of code.
 */

Which can be summarized as

• "reduce review load"

I'm not going to address the rationales given here, I merely wish to point out that you've triggered my "methinksth thou dost protesth too much" heuristic of argumentation. Usually this is an indication that there's actually another, unspoken and perhaps unconscious, rationale for as to why you'd like to keep the code the way it is, rather than following the fairly small request, I think at this point, to change it.

What do you think?

[ZmnSCPxj]

Well, I could put a long treatise in the comments like so, if that is what you would prefer:

/* Often, in programming, we find we need to split functions into smaller pieces,
 * simply as a consequence of refactoring code for code reuse, or in a server
 * context, simply as a consequence of having to do callback-style coding
 * in order to block for a task but not block the entire server.
 * Now, ideally you want those functions to be in a reasonable order, that is,
 * in the same order you would have read the lines if they were a single
 * large function instead of broken up into tiny pieces.
 *
 * However, C requires functions to be predeclared before first use.
 * So if you have to split up a function into two pieces, for refactoring, or
 * to make a callback for a blocking operation, and the first piece has to
 * refer to the second piece (to call it or to provide as callback function)
 *
 * - Either you predeclare the second piece and add a few lines.
 * - You twist your code so that the second piece is written before the first.
 *
 * Now, there are other reasons to predeclare, or not predeclare.
 * 
 * - If you have to do mutual recursion, no choice but to predeclare.
 * - If you want to isolate a common operation off into its own function,
 *   you should not predeclare, or should consider moving it off into
 *   its own source file.
 *   (And if it is off into its own source file, it is effectively predeclared
 *   in its own header).
 *
 * On the other hand, maintaining the proper order for executing
 * code should generally be a reason to predeclare code.
 * Predeclaration is just a few lines.
 *
 * But really, the issue is that C requires predeclaration *at all*.
 */

Does that satisfy your psychologist? Or can we get on with fixing this bug?

[ZmnSCPxj]

Removed the predeclaration as a separate commit so that the code-move can be reviewed separately from the code-modification.

[niftynei]

bump @ZmnSCPxj

[niftynei]

ACK 1ace2ad

Rebased on master to resolve conflict with (merged) #4126