lightninglabs / taproot-assets

A layer 1 daemon, for the Taproot Assets Protocol specification, written in Go (golang)
MIT License
458 stars 110 forks source link

[bug]: Handle possibility of transfer change outputs being below dust threshold #738

Closed jharveyb closed 5 months ago

jharveyb commented 9 months ago

Context

Discovered while working on #605. In the freighter, when anchoring our vTXs, we have a block where we fund, adjust, and then sign a PSBT.

We create a template TX with enough P2TR dummy outputs to hold the Taproot Asset trees for all receivers, and sender change. This template TX has no inputs, so that when passed to FundPsbt the backing wallet will attach enough funds to pay for the vsize of the outputs and attach a change output if needed.

After that, we 'adjust' the now-funded PSBT by attaching our anchor inputs that hold our input assets for the transfer. Since we've increased the vsize of the TX, we compensate by decrementing the value of the change output, so that we maintain the target feerate.

One problem we hit earlier is that this could cause the new 'change' value to be negative, which we check for and fail in here:

https://github.com/lightninglabs/taproot-assets/blob/564795a0e4c1d1034493b498f58a552920f2e39c/tapfreighter/wallet.go#L1726

The newly discovered issue is that the change value could also be below the dust threshold (546?), which would cause broadcast of the transfer TX to fail. In #605 we check for this and fail at that point, but ideally we could better communicate the final vsize of our transfer TX to the backing wallet when calling FundPsbt so that the TX is funded correctly (and FundPsbt already performs these sanity checks).

Mitigation

A possible solution is to have the backing wallet fund the TX and handle change adjustment by calling FundPsbt twice. The first call is the same as the current call, with only dummy P2TR outputs. Before the second call, we also attach our anchor inputs, but remove the change output added by FundPsbt.

On the second call, FundPsbt would then not attach any new inputs, but instead use the vsize of this TX, and consider the additional vsize of any change output, to compute the correct change output for the given feerate.

This would remove the need for any fee or size estimation code in the freighter and outsource that to the backing wallet. We would still need to shuffle outputs after the second funding call.

A hackier alternative is to adjust the value of the dummy outputs to cover the fee for the vsize of the anchor inputs, but that sounds strictly worse.

Problems with mitigations

Between the two FundPsbt calls, the inputs added to the TX must be unlocked; this means that they could be spent by some other caller. This would be detected by the backing wallet and cause the second FundPsbt call to fail gracefully. In that case, we would need to be able to repeat the first call to FundPsbt.

I'm pretty sure the second FundPsbt call would handle removing a change output entirely if it would be dust, but I don't know if that holds if the template TX has only one output. This could happen from a full-value send from an anchor only holding one asset, or a full-value burn of the same type of input.

Regardless of which mitigation we choose, we'd want some way to reattempt PSBT funding and signal that we need more funds (or at least an informative error message).

Acceptance criteria

jharveyb commented 5 months ago

This was resolved with https://github.com/lightninglabs/taproot-assets/pull/858.