Add tryGetFundsFromRemoteCommitmentTx + bugfixes

[canndrew]

This PR adds the tryGetFundsFromRemoteCommitmentTx and provides several bugfixes.

[canndrew]

@canndrew I see a new warning popping up here: The value 'remoteCommitmentPubKeys' is unused

This has been fixed.

[knocte]

Hey @canndrew, can you:

1. Add tryGetFundsFromRemoteCommitmentTx function …: you don't explain the high-level reason of why this is needed, is it force-closing?
2. Fix warning in tryGetFundsFromRemoteCommitmentTx …: squash this commit with the above ^
3. Pass feerate and target address to tryGetFundsFromRemoteCommitmentTx …: elaborate more about NBitcoin API limitations please. Also would like to know why is CPFP flag argument needed? will geewallet use it? when would it be desirable to pass it as false? Or is this just to be used by End2End tests?
4. Fix commitment number discovery for fund recovery txs: split this commit in 2 please (and after that, squash the one that changes tryGetFundsFromRemoteCommitmentTx with the 1st commit, and leave the other one separated)
5. Don't set sequence number when recovering from to_remote output …: squash this commit with the 1st one or 3rd one.
6. Fix fee rate calculation …: FYI I proposed this upstream here: Fix fee rate calculation joemphilips/DotNetLightning#144

[knocte]

Fix fee rate calculation …: FYI I proposed this upstream here: joemphilips#144

BTW, what's the effect of this patch above not being in m1/m2?

[canndrew]

I've applied the requested changes and force-pushed.

BTW, what's the effect of this patch above not being in m1/m2?

Nothing, as far as I'm aware. I only became aware that the function is buggy once I went to use it.

[knocte]

the function is buggy once I went to use it

And what's the effect of the bug? If there's no effect, there's no bug.

[knocte]

setting the tx version number and input sequence number
requires converting to an NBitcoin.Transaction

why? cannot we add this API to NBitcoin?

Also pass a requiresCpfp argument to both fund-recovery functions so
that cpfp can be explicitly requested. If the commitment tx is lingering

Please use UPPERCASE for acronyms (in the commit message).

in the mempool then this can be set to true to increase the chance of
it being mined. Otherwise, if the commitment tx has already been mined
then this can be set to false to avoid paying a larger fee.

If requiresCpfp parameter causes fee to be increased, then it's not a good name for the parameter. How about increaseFeeToRaiseCpfpIncentive?

[knocte]

BTW, isn't e6cf382's author actually Afshin? (#19)

[canndrew]

And what's the effect of the bug? If there's no effect, there's no bug.

Here's the definition of the type.

/// feerate per kilo weight
type FeeRatePerKw = | FeeRatePerKw of uint32 with

Note that this type is supposed to represent a fee per weight.
Here's the definition of the (unfixed) function:

static member FromFee(fee: Money, weight: uint64) =
    (((uint64 fee.Satoshi) * weight) / 1000UL)
    |> uint32
    |> FeeRatePerKw

Which is being changed to:

static member FromFee(fee: Money, weight: uint64) =
    (((uint64 fee.Satoshi) * 1000UL) / weight)
    |> uint32
    |> FeeRatePerKw

I'm pretty sure the only reason this didn't come up earlier is that we were never using this function.

why? cannot we add this API to NBitcoin?

Possibly. But it's easier to change the function so that we pass it the destination and fee rate, rather than mess around with the (very abstruse) internals of TransactionBuilder.

Please use UPPERCASE for acronyms (in the commit message).

Fixed.

If requiresCpfp parameter causes fee to be increased, then it's not a good name for the parameter. How about increaseFeeToRaiseCpfpIncentive?

Fixed.

BTW, isn't e6cf382's author actually Afshin? (#19)

I've amended the commit to make him the author.

[knocte]

I'm pretty sure the only reason this didn't come up earlier is that we were never using this function.

So this function is only used for CPFP? That's kinda surprising because looks generic enough.

If requiresCpfp parameter causes fee to be increased, then it's not a good name for the parameter. How about increaseFeeToRaiseCpfpIncentive?

If you agreed to the above, this still makes me scratch my head sorry. How much is the increase? it looks very subjective.

Possibly. But it's easier to change the function so that we pass it the destination and fee rate, rather than mess around with the (very abstruse) internals of TransactionBuilder.

Well, if you mess arounds with the internals of TransactionBuilder and the maintainer of NBitcoin merges your PR, it gives me more confidence than suddenly make this little code flow here use Transaction, when we have used TransactionBuilder in all other places.

[knocte]

BTW regarding the 1st commit:

• Its CI is marked as red, what's going on there?
• The git commit message mentions force-closing, however AFAIU force-closing was already finished in geewallet's m3 branch thanks to this other patch in DNL.Kiss' m3 branch: d23e1c1

[canndrew]

So this function is only used for CPFP? That's kinda surprising because looks generic enough.

That function isn't the only way to construct a FeeRatePerKw. So it's not terribly surprising that we'd never happened to use it previously.

If you agreed to the above, this still makes me scratch my head sorry. How much is the increase? it looks very subjective.

It's not subjective. It's whatever the deficit is in the fee of the commitment transaction. If the commitment transaction has a fee of A, but it needs to offer a fee of B to get mined (based on the current fee rate) then we'll add (B - A) to the recovery tx fee.

Well, if you mess arounds with the internals of TransactionBuilder and the maintainer of NBitcoin merges your PR, it gives me more confidence than suddenly make this little code flow here use Transaction, when we have used TransactionBuilder in all other places.

What do you mean by "all other places"? We use the Transaction type fairly often as well. TransactionBuilder is a level of abstraction above individual inputs/outputs of a transaction, so I'm not sure if it makes sense to be able to set sequence numbers of inputs via TransactionBuilder.

[knocte]

If the commitment transaction has a fee of A, but it needs to offer a fee of B to get mined (based on the current fee rate) then we'll add (B - A) to the recovery tx fee.

Let's put this as a comment on top of the cpfp parameter.

We use the Transaction type fairly often as well.

Whenever I've seen the Transaction type being used I've always told you to use TransactionBuilder instead. Please let me know all the places where I forgot.

[canndrew]

Whenever I've seen the Transaction type being used I've always told you to use TransactionBuilder instead. Please let me know all the places where I forgot.

Sorry, but what are you actually asking for here? The only reason this function originally returned a TransactionBuilder is because it's not its business where the funds go, so it made marginally more sense to have the caller set that outside of the function. But having the caller just pass in the destination works too.

Transaction allows you to set the version number and input sequence id. If TransactionBuilder allowed us to do that then we could re-order these three lines:

let recoveryTransaction = transactionBuilder.BuildTransaction false
recoveryTransaction.Version <- TxVersionNumberOfCommitmentTxs
recoveryTransaction.Inputs.[0].Sequence <- Sequence(uint32 commitments.LocalParams.ToSelfDelay.Value)

to this:

transactionBuilder.Version <- TxVersionNumberOfCommitmentTxs
transactionBuilder.Inputs.[0].Sequence <- Sequence(uint32 commitments.LocalParams.ToSelfDelay.Value)
let recoveryTransaction = transactionBuilder.BuildTransaction false

But I don't see how that offers a marked improvement.

Transaction is used throughout geewallet and DNL whereever we need to represent a transaction. You can't eliminate it by replacing it with TransactionBuilder since the only thing you can do with a TransactionBuilder is to build it into a Transaction.

[canndrew]

BTW regarding the 1st commit:

• Its CI is marked as red, what's going on there?

It looks like that macaroons random failure again, so not related to the commit. I'll re-run to see what it does. Edit: It passed.

• The git commit message mentions force-closing, however AFAIU force-closing was already finished in geewallet's m3 branch thanks to this other patch in DNL.Kiss' m3 branch: d23e1c1

That patch allows us force-close channels and recover the funds. The first commit here allows us to recover funds from remotely force-closed channels.

[knocte]

I said in the past that I've had many problems with the API of Transaction and I'd prefer to use TransactionBuilder everywhere. In many cases where you were even skeptic that you could do this, I've done the changes myself and it has worked.

Transaction is used throughout geewallet and DNL whereever we need to represent a transaction. You can't eliminate it by replacing it with TransactionBuilder since the only thing you can do with a TransactionBuilder is to build it into a Transaction.

What I'm saying is that if you need a Transaction, build it with TransactionBuilder, don't use Transaction API. Don't deal with transactions, it's too low level, just deal with them when you're about to broadcast them or save them, don't modify them, don't use them as building blocks, use TransactionBuilder! If you're missing any API from TxBuilder please contribute it to NBitcoin. Create a PR on NBitcoin now please.

[knocte]

@canndrew one last thing, can you move the commit titled Fix commitment number discovery in tryGetFundsFromLocalCommitmentTx to be the 1st of the PR? (and merge it with the NBitcoin upgrade), sorry to be so picky, I'll explain why later

[canndrew]

@knocte: Done.

[canndrew]

This PR has been replaced by #25.