Fix CLN service startup failure by trimming trailing spaces in config parameters

[maxrantil]

fixes #7132

I would really appreciate some friendly feedback on this, as it is my first PR to the project and I might have misunderstood or missed important details.

I did find a similar function strip_trailing_whitespace in plugins/bcli.c, but from what I understand, having void trim_trailing_spaces(char *str); in ccan would still be beneficial, right?

[vincenzopalazzo]

Thanks for this, looks good to me

I was wondering if it is possible move this trim function inside tal string https://github.com/rustyrussell/ccan/blob/master/ccan/tal/str/str.h

[maxrantil]

I was wondering if it is possible move this trim function inside tal string https://github.com/rustyrussell/ccan/blob/master/ccan/tal/str/str.h

Do you mean to also change the function to use tal for allocating memory for the lines then? I thought it was not necessary since they were already allocated, but I can of course fix that if it is desirable. @vincenzopalazzo

[vincenzopalazzo]

Do you mean to also change the function to use tal for allocating memory for the lines then?

No I mean to have like tal_strtrim or strtrim (not in tal), at the end of the day your code is implementing a subset of functionality to trim an origin string.

[maxrantil]

Pardon me, but I don't fully understand what you mean. Could you elaborate on your suggestion? To my understanding, the tal functions you are linking to, @vincenzopalazzo , relate to string functions involving memory allocation. If I were to implement tal_strtrim(), from my experience, those implementations remove spaces from the beginning and end and allocate a new string. Is that what you are aiming for here? The current placement of the function trim_trailing_spaces() in ccan/ccan/str/str.c made sense to me because it manipulates an already allocated string.
Like I said at the beginning, I am new here and would appreciate a more thorough explanation if you have time for that.

[endothermicdev]

Nice PR! It's a neat solution to the problem. I agree with Vincenzo - so long as a ccan library is being modified, it would be nice to go ahead and trim the leading whitespace too and resize the buffer (which would make this a tal/str operation). It's not a significant difference in this case, but it could be more useful for the library. I'm okay with either approach.

[maxrantil]

Thanks for the comment @endothermicdev, I'll start doing a strtrim function in tal tomorrow if you both think that is more useful then 👍

[maxrantil]

@vincenzopalazzo & @endothermicdev , I've now changed to using tal_strtrim, is this what you had in mind?

[maxrantil]

@endothermicdev I am having some issues with tests on my local machine using tal_resize like this.
Is there anything in the way I am using it now, that you can see, is not working like it should? I get some out of memory errors from within the tal_resize() on my machine but the tests seems to run fine here in github-actions...
I am working on writing a test for it that I will push with the PR.

[maxrantil]

The local test I have tried is to add a space after a config varible in the startup_regtest.sh file and then sourced it and start_ln 2.

[maxrantil]

If someone could have a look at the pytest and tell me if something is wrong in it, that would be appreciated.

[rustyrussell]

Hi @maxrantil sorry this has been so long before I review!

The concept is great, the implementation needs some work though.

There are a few config options where we don't want to trim whitespace. In particular, log-prefix, and alias but you could also argue for things that take paths, such log-file, lightning-dir, pid-file ,conf etc. But I'm happy to just stick with log-prefix and alias.

1. This implies that we should add a new flag, OPT_KEEP_WHITESPACE in common/configvar.h, and trim cv->optarg in common/configvar.c before calling return ot->cb_arg(cv->optarg, ot->u.arg); unless (ot->type & OPT_KEEP_WHITESPACE).
2. Add this flag to those options which need to preserve whitespace.
3. Trimming whitespace is trivial, you should simply do it by replacing the final space with a NUL terminator:

while (strlen(s) != 0 && cisspace(s[strlen(s)-1])
    s[strlen(s)-1] = '\0';

4. We should note in doc/lightningd-config.5.md: "Because it's a common error, we automatically trim whitespace from the end of most configuration options. Exceptions are noted below". And then note it in alias and log-prefix, and any others you do it to.

Thanks!

[maxrantil]

Hi @rustyrussell ,
Thanks for the feedback! I didn't notice the review until now but I will get into it asap.

[maxrantil]

@rustyrussell or @endothermicdev , do you find it acceptable to manipulate a const char * with memmove() like I do in the trimming func??

[rustyrussell]

Given how close we are to rc1, I simply added a commit with various cleanups. Thanks for this work!

[maxrantil]

That's fine @rustyrussell ! I learned some new things from studying your changes.

I'm not sure how to fix the error in the test that is failing now, tho.

[rustyrussell]

THose issues were from master, fortunately!

[maxrantil]

Is there any chance of getting the sphinx bounty of 100k sats for this? 😊 I can't get Sphinx Chat to work with Core lightning on my umbrel node. Is there another way to get the payout if i am eligible for it?