[WIP] funding: add publisher

[ellemouton]

Fixes #4760

Fixes #386

Fixes #7868

The publisher does a few tx checks (sanity, standard, mempool test (not implemented yet)) before actually attempting to publish the transaction. Errors from each call is wrapped so that it is easy to reason about for the caller.

Still a WIP. Looking for approach comments. The last commit is a bit messy but i will clean it up after approach ACK.

Context:

The funding manager calls Publish at 2 points:

1. during funding flow handleFundingSigned, after persisting the channel and deleting (not canceling) the reservation context.
2. on startup: it tries to broadcast the funding tx without having a lock on the inputs.

I think we need to manage make decisions differently for each of the above scenarios:

1. For this scenario: we can safety abort the process if:

   • sanity check fails
   • (standardness check or mempool test check fails) && this was not an externally constructed tx. If this was externally funded then we should continue to monitor the channel.

   When we abort, we should make sure to release the locks on the utxos.

2. In this scenario, we need to be a bit more careful because at this point we dont know if the tx has ever been published before. So in this case, we only abort if we get a sanity check error OR we get a ErrDoubleSpend error.

Scope?:

The following still needs to be done. Should any of these be included in this PR?

• need to thread bitcoind testmempool call through to its rpc
• should add a testmempool fund to btcd's mempool struct and add the call to its rpc so we can call it
• for all the cases that we are lenient and dont fail the tx, we should instead occasionally try to rebroadcast it and after a few blocks, maybe we then release the inputs and wait for double spend. (note ,this double-spend-wait already happens in the startup scenario since the inputs are not locked. But it still requires the re-broadcasting logic)

Depends on btcsuite/btcd#1840

[ellemouton]

cc @yyforyongyu & @Crypt-iQ : looking for some approach comments before continuing. Pls see description :)

[Crypt-iQ]

on startup: it tries to broadcast the funding tx without having a lock on the inputs.

This seems like a bug?

[yyforyongyu]

Overall looks good.

The funding manager calls Publish at 2 points:

There's another place we call PublishTransaction in batch funding.

sanity check fails

Wonder what these checks are. Are they ln-specific or bitcoin-specific? Do we have similar checks in PublishTransaction already?

[Crypt-iQ]

Wonder what these checks are. Are they ln-specific or bitcoin-specific? Do we have similar checks in PublishTransaction already?

I'm guessing this is the lite version of bitcoin transaction standardness - I think PublishTransaction calls it

(standardness check or mempool test check fails) && this was not an externally constructed tx. If this was externally funded then we should continue to monitor the channel.

why not abort if the tx is non-standard? if the mempool accept API fails due to ancestor limits / is full, we should probably keep the channel. AFAIK the only way to get tx standardness in bitcoind without broadcasting the transaction is to call the mempool accept API - maybe there's another RPC though.

[Crypt-iQ]

i dont think this should be copied

[Crypt-iQ]

on startup: it tries to broadcast the funding tx without having a lock on the inputs.

This seems like a bug?

roasbeef said the locks are persisted and don't need to be re-acquired on startup, so this actually seems like correct behavior?

[lightninglabs-deploy]

@ellemouton, remember to re-request review from reviewers when ready

[ellemouton]

closing for the time being as im not actively working on this

[Roasbeef]

Re-opening as related to #7868. I think push the branch to origin, that way someone can take over just by building off that branch.

[ellemouton]

I think push the branch to origin, that way someone can take over just by building off that branch.

done!