Do not enforce new reserved value in PSBT midstep

[Kixunil]

This change avoids enforcing new reserved value when PSBT funding is not
finished yet as new inputs and outputs may still be added that could
change the outcome of the check.

This originally failed in the scenario when funding a channel from
external wallet and depositing to on-chain wallet was done
simultaneously in a single transaction. If such transaction confirms
then reserved UTXO is guaranteed to be available but the check didn't
take it into account.

The enforcement still occurs in the final step of PSBT funding flow, so
it is safe. It also occurs in case of non-PSBT funding.

This partially fixes #5363 (comment)
For full fix it'd be great to be able to optionally avoid anchor commitments (to improve privacy).

I will look at tests later, I mainly wanted to get initial feedback on this. If anyone would like to help with writing tests I'd be happy to accept. :)

Pull Request Checklist

• If this is your first time contributing, we recommend you read the Code
  Contribution Guidelines
• All changes are Go version 1.12 compliant
• The code being submitted is commented according to Code Documentation and Commenting
• For new code: Code is accompanied by tests which exercise both
  the positive and negative (error paths) conditions (if applicable)
• For bug fixes: Code is accompanied by new tests which trigger
  the bug being fixed to prevent regressions
• Any new logging statements use an appropriate subsystem and
  logging level
• Code has been formatted with go fmt
• Protobuf files (lnrpc/**/*.proto) have been formatted with
  make rpc-format and compiled with make rpc
• New configuration flags have been added to sample-lnd.conf
• For code and documentation: lines are wrapped at 80 characters
  (the tab character should be counted as 8 characters, not 4, as some IDEs do
  per default)
• Running make check does not fail any tests
• Running go vet does not report any issues
• Running make lint does not report any new issues that did not
  already exist
• All commits build properly and pass tests. Only in exceptional
  cases it can be justifiable to violate this condition. In that case, the
  reason should be stated in the commit message.
• Commits have a logical structure according to Ideal Git Commit Structure

[Kixunil]

Ping, could someone check if this is at least going in the right direction?

[guggero]

The PR must've slipped below the radar, sorry for the delay.
I think this is going into the right direction. Especially when reading that comment I think that was the intended behavior from the beginning but a refactor/fix somehow changed it.

This should be covered by an integration test I think, just to make sure nothing else in the funding flow breaks.

[guggero]

There are three types of ChanFunder: PSBT, canned (used for Pool) and the wallet assembler. We only want to skip this check for the wallet assembler. So I think we should use the default value true and only disable it for the PSBT funder (with a type check).

[Kixunil]

I'm not sure I understood you correctly, so I added another commit. Is that what you had in mind?

[guggero]

Yes, this is what I meant. But it should be done in the same commit.

[Kixunil]

I agree it should have test. Unfortunately I have literally zero experience with writing tests in Go and LND. If you could give me a few bullet points on where to put it and how to structure it that'd be very helpful.

[guggero]

See lntest/itest/lnd_psbt_test.go, there we already have a test that asserts the default PSBT channel funding flow. We could add an additional test that makes sure the reserves are checked in the correct step. Or extend the existing test with some additional checks.

[Kixunil]

I looked into it and it seems rpctest.Harness doesn't have public methods for creating and broadcasting transactions separately. It seems it could be solved by adding CreateTransaction and SendRawTransaction that'd pass arguments to the internal wallet.

Should I file a PR against https://github.com/btcsuite/btcd/ first or is there some other process I should follow?

That wouldn't help and I think it's not needed. We can use a third node to fund PSBT.

[Kixunil]

Is there a way to run a specific test only? The tests are slow and failing for what seem to be unrelated reasons, so I'd like to focus on getting this working.

[guggero]

https://github.com/lightningnetwork/lnd/blob/master/docs/MAKEFILE.md#itest

[Kixunil]

Strangely when I wrote my test, I tried to check if it actually fails without the change and it didn't. I couldn't figure out why and then life dragged me into bunch of other things. I still intend to try resolve this but if someone can help that'd be appreciated.

[Kixunil]

Figured it out - forgot to enable anchors 🤦‍♂️

I rebased the changes and added the test which fails before the fix and passes after the fix. I only need to add release notes, but I don't know which version. Would it be possible to add this to point release, please? I was originally hoping to get this into 0.14...

[guggero]

Let's try with 0.14.1.

[Kixunil]

LOL, was looking at test and thinking why they don't run yet. :D Done.

[Kixunil]

Looks like same unrelated/flakes again.

[Kixunil]

Updated release notes version to 0.14.2

[guggero]

LGTM 🎉

[Kixunil]

Rebased

[Roasbeef]

LGTM 🎎

[Kixunil]

Unrelated test failures again.