wallet: respect policy-level transaction size limits when crafting regular and funding transactions

[Roasbeef]

Tracking this here instead of the btcwallet issue for more visibility. As is, we don't always ensure that a transaction either we construct manually (for funding) or within btcwallet itself is below the current widely used policy that constraints transaction sizes. Thankfully, we can simply start using the existing policy level function within btcd itself to enforce these limits: https://github.com/btcsuite/btcd/blob/master/mempool/policy.go#L271

When a user encounters this issue, they'll see this in the logs:

: unmatched backend error: -26: 64: tx-size

And then end up with a dangling transaction that'll keep being rebroadcast, when instead we should remove it from the tx store.

[Roasbeef]

Actually, reading the btcwallet code more closely here, I think we already properly handle this case. What we don't do however, is remove the "pending open" channel from the lnd's DB.

[ellemouton]

Just want to get some feedback re the scope of this before diving any deeper:

seems to me there are 2 issues here with ranging scopes:

1. missing sanity checks when constructing a funding tx.

This would involve some changes to the ChannelReservation logic to ensure that the funding tx it builds is below a certain size (would need to fetch that size somewhere) and to then retry with different coins if size is too big. ie, this becomes a coin selection issue.

2. dealing with a pending channel after having tried broadcasting the tx.

This is a lot trickier than problem 1 since with this, we have already sent our tx out into the wild and cant ever be 100% sure that it has not actually broadcast. Not entirely sure what the fix for this should be... yes we can examine the returned errors but i dont think we can assume the errors are trustworthy enough to release the inputs and forget the channel.

Are the above assumptions correct? And if so, should the scope of the fix for this issue address both?

(any thoughts would be much appreciated @yyforyongyu , @Roasbeef @ & @Crypt-iQ (saw that you posted some thoughts on the linked PR))

[Crypt-iQ]

Fixing both at once sounds good (could split up into 2 pr's)

1. Does not have to be in ChannelReservation logic. Ideally we'd have a testmempoolaccept api that handled this instead of trying to mimic standardness rules in the funding code.
2. If step 1 is implemented and an error is returned, then something besides standardness is going on. Maybe this is an unreliable error. I see two paths here:
   2a. If an error occurs, try to double-spend the transaction. This would need Funding detect double spends  #5567 to detect the double-spend, release inputs, and forget the channel.
   2b. Continually rebroadcast the funding transaction if an error occurs.

2a & 2b could be combined so that 2b falls back to 2a after some time.

[yyforyongyu]

we have already sent our tx out into the wild and cant ever be 100% sure that it has not actually broadcast.

I don't think this is something we want to deal here. We use the interface PublishTransaction here, which is implemented in btcwallet. If the above statement is an issue then we should fix it in btcwallet.

Basically we rely on this interface here, and it must do what it guarantees. Otherwise we'd have an issue not just in funding, but every call site of this interface in lnd.

As for the scope, I think this issue deals specifically with the error : unmatched backend error: -26: 64: tx-size. However, PublishTransaction doesn't explicitly return this error, as it maps the error codes here. So to continue the work in #5158, I think if we could explicitly check this error we'd be safe. So either,

• we change btcwallet to catch and return this error explicitly, or,
• we can do an error match in lnd to be sure we are getting this exact error when PublishTransaction fails.

[Crypt-iQ]

we have already sent our tx out into the wild and cant ever be 100% sure that it has not actually broadcast.

I don't think this is something we want to deal here. We use the interface PublishTransaction here, which is implemented in btcwallet. If the above statement is an issue then we should fix it in btcwallet.

I believe elle is referring to the case where we send a transaction and we receive a malicious MsgReject in neutrino. Or another scenario like maybe we're eclipsed and have sent it out, but it hasn't propagated. This neutrino case should definitely be fixed in addition to the PR that fixes this issue.

Basically we rely on this interface here, and it must do what it guarantees. Otherwise we'd have an issue not just in funding, but every call site of this interface in lnd.

I still think having some API that calls PublishTransaction and continues to do so until some block-timeout would be useful.

As for the scope, I think this issue deals specifically with the error : unmatched backend error: -26: 64: tx-size. However, PublishTransaction doesn't explicitly return this error, as it maps the error codes here. So to continue the work in #5158, I think if we could explicitly check this error we'd be safe. So either,

* we change `btcwallet` to catch and return this error explicitly, or,

* we can do an error match in `lnd` to be sure we are getting this exact error when `PublishTransaction` fails.

This is what a testmempoolaccept api could pre-emptively handle. This is already something that we need as having one would have caught the high-S signatures CVE. I do think though that a minimum fix would be something along the lines of your original PR, but we'd want to catch more than just tx-size since there are other policy checks that can fail besides size. As an example, an error that is returned that is not policy and wouldn't be caught if we'd only catch tx-size is mempool min fee not met. If all non-double-spend errors are caught, however, I think it would be a good idea on error to wait until the input is double-spent or the channel is confirmed on chain (either we missed something or the user took their raw tx and broadcast it elsewhere) before cleaning up as a safety measure in case something was missed.

[yyforyongyu]

@Crypt-iQ If we want to handle more than tx-size error, it'd be nice but another story. This testmempoolaccept is dope and can be implemented in btcwallet. I think our abstractions are pretty clear here, lnd defines all types of transactions, btcwallet treats them as raw tx and handles the validation and broadcast. Behind the wallet there are three chain backends handling layer one stuff. If we want to fix the issues across the layers of abs, then a good first step IMO is to refactor PublishTransaction to return a list of explicitly defined errors, and we handle the errors one by one in lnd. I don't think there's one solution to fit in all the scenarios, eg if tx-size is not right we are sure we can abandon the funding flow.

or the user took their raw tx and broadcast it elsewhere

If this happens it's nice to handle it but not necessary or urgent IMO. Meanwhile, curious why would a user do that?

[Crypt-iQ]

testmempoolaccept would probably be written in btcd and called from btcwallet. My point is there are more policy errors besides tx-size that a user can hit and these would probably occur with the psbt api. This issue is concerned with tx-size, but it could easily include every policy violation and PublishTransaction would just return some PolicyError. Also the mempool-min-fee error should be caught and special-cased probably

Meanwhile, curious why would a user do that?

Users pretty much do anything and everything given some large timeframe. I was thinking this would happen using the psbt api.

[Roasbeef]

Seeing this in #6566 but odd in that if you go to my last linked comment there, in the code we should always remove the transaction on startup when/if we go to rebroadcast it, but also the very first time we go to broadcast it (unless that fails for some reason).

[ellemouton]

More details from users who have run into this for both sendcoins and during channel open can be found in these issues:

#6556
#7622