grpc: addpsbtoutput command

[s373nZ]

@cdecker This PR is a first pass at #6986. The major fix here was including AddPsbtOutput to the big list of RPC commands in utils.py for msggen. After re-running the bundle and generate commands, custom changes to the .msggen.json file were necessary to map fields and set versions. The rest of the files were generated automatically.

Also added parameter descriptions and some references to addpsbtoutput in the docs where it seemed to make sense in a separate commit.

I was a little paranoid about committing the generated code in case there were small differences between build environments, and ended up double-checking by building with Docker. Hope it worked out...

@BitcoinJiuJitsu I'm hoping this qualifies to claim the Sphinx bounty, too ;)

[s373nZ]

@cdecker Pretty sure this CI got disrupted by the Poetry 1.8.0 release a few hours ago. The Dockerfile.alpine works locally for me when I change the command to python3 -m pip install "poetry==1.7.1", but I imagine this might break elsewhere though...

[s373nZ]

Trying to avoid errors in the CI Prebuild check here by running a build using: docker build -t lightningd-ubuntu -f contrib/docker/Dockerfile.ubuntu .

And committing the generated code from that.

[cdecker]

@cdecker Pretty sure this CI got disrupted by the Poetry 1.8.0 release a few hours ago. The Dockerfile.alpine works locally for me when I change the command to python3 -m pip install "poetry==1.7.1", but I imagine this might break elsewhere though...

I'm removing the bloody alpine test, it keeps rotting away. See #7114. This PR will not make it into the v24.02 anyway, the feature freeze was 2 weeks ago :-)

[s373nZ]

I'm removing the bloody alpine test, it keeps rotting away. See #7114. This PR will not make it into the v24.02 anyway, the feature freeze was 2 weeks ago :-)

I removed the Python pin commit for Alpine then. I don't mean to waste build cycles or attention on review notifications -- if there is a standard way to run the build, codgen and doc tests locally, please do share. So far I've been using:

./configure
make
make check

But the check reports some different command names (even on master) so I'm not sure how far it gets. Also been experimenting trying to run the various builds through the Dockerfiles... Currently running the above under that experimental Nix flake, so there is ample room for confusion :/

[cdecker]

This being a msggen-related change, you probably want to make check-gen-updated which is what is run on CI to check that all dependent files have beenm updated, and then make pytest to run through the python integration tests.

[s373nZ]

@cdecker I rebased the latest from master. After experiencing some inconsistent behavior from pytest locally, I managed to produce two successful consecutive runs after:

1. Freeing up some more disk space
2. Raising the ulimit for open files as I saw some related crashes when pyln was reading the bitcoin.conf

In any event, I'm happy to have a development environment working as expected. Thanks for fielding questions in Discord. AFAICT, it seems like the next step is another round of CI.

[cdecker]

Hm, that ulimit comment is concerning, we should never have more than 1024 FDs open at a time, and this might indicate an FD leak.

[s373nZ]

@cdecker I re-ran pytest today with ulimit set to 1024 and yes, there is an FD leak. It shows up under my Nix setup, because Nix also opens a lot of files 1 2. I think I've narrowed it down to this:

lightning/contrib/pyln-testing/pyln/testing/utils.py

Line 203 in d1101f4

self.stdout_write = open(self.stdout_filename, "wt")

Just added commit 42e1e6b which seems to resolve the FD leak for me locally. A few notes:

1. Initially I tried to do this in a __del__() destructor, but it didn't work -- maybe because NodeFactory is a fixture.
2. Although stop() calls kill() in some cases, if there are tests that call kill() directly, maybe there is a more robust place to centralize the cleanup.
3. Let me know if you would like to open a new issue or cherry-pick to a new PR for the FD leak topic, as it's not specifically related to the addpsbtoutput command.

edit: fix link markdown

[s373nZ]

Removed the FD leak changes due to a few failing tests and opened #7130 to investigate them there.

[daywalker90]

This is still based on the old json schema format. Will you update this PR @s373nZ ?

[s373nZ]

@cdecker @daywalker90 It looks like the msggen component is undergoing some serious refactor recently. Do you have any advice on how to move this PR forward toward a merge-able state, or feedback on my comments?

I'd like to finish this issue to completion, but prefer not to spend time keeping up with bitrot if the PR becomes unnecessary or has no priority.

[daywalker90]

IIRC you should not change the added version but instead use the patch.py to backfill the fields:

• uncomment these lines in contrib/msggen/msggen/patch.py:

# if f.added is None and 'added' not in m:
#    m['added'] = 'pre-v0.10.1'

• run msggen
• comment lines in patch.py again
• commit

[s373nZ]

@daywalker90 I did as you suggested. Still a little confused why the satoshi field isn't included in the .msggen.json model-field-versions, but it is of type sat and the other example of that field in doc/schemas/lightning-funderupdate.json named policy_mod isn't included either. Perhaps it is excluded as a complex field.

Thanks for the feedback. Let me know if you see anything else.

[daywalker90]

Ah yes you have to change the sat type to msat for now. Then rerun msggen

[s373nZ]

@daywalker90 Nice -- that looks better to me, although I don't know much of what to expect from the generated code. Also rebased against master.

edit: Also, I didn't update the documentation for satoshi to reflect msat denominations because my understanding of that field is that millisatoshi's don't make sense in this case.

[daywalker90]

looks good to me

[cdecker]

Rebased on top of master. Hopefully we can make it into the release, no reason not to try :-)

Sorry for forgetting about this one ☹️

[s373nZ]

Thanks! It had almost slipped on me as well. I've submitted a plea for inclusion over Discord.

[endothermicdev]

This doesn't look right. The psbt code handles satoshi amounts and the description matches this. Maybe there needs to be a new rust primitive type?

[daywalker90]

@cdecker and me discussed this before, for now this is correct. But could be improved in the future. I think this was changed when the schemas got merged and some were forgotten. I've had to do this on a couple commits aswell.

[daywalker90]

see here: #7230 (comment)

[endothermicdev]

Ah, thanks for the back story!

[ShahanaFarooqui]

ACK 6196871.