Merge pull request #4455 from cfromknecht/psbt-no-publish

PSBT: add no_publish flag for safe batch channel opens v2

cmd/lncli/cmd_open_channel.go

@@ -163,6 +163,15 @@ var openChannelCommand = cli.Command{
 				"as a base and add the new channel output to " +
 				"it instead of creating a new, empty one.",
 		},
+		cli.BoolFlag{
+			Name: "no_publish",
+			Usage: "when using the interactive PSBT mode to open " +
+				"multiple channels in a batch, this flag " +
+				"instructs lnd to not publish the full batch " +
+				"transaction just yet. For safety reasons " +
+				"this flag should be set for each of the " +
+				"batch's transactions except the very last",
+		},
 		cli.Uint64Flag{
 			Name: "remote_max_value_in_flight_msat",
 			Usage: "(optional) the maximum value in msat that " +
@@ -270,6 +279,10 @@ func openChannel(ctx *cli.Context) error {
 	if ctx.Bool("psbt") {
 		return openChannelPsbt(ctx, client, req)
 	}
+	if !ctx.Bool("psbt") && ctx.Bool("no_publish") {
+		return fmt.Errorf("the --no_publish flag can only be used in " +
+			"combination with the --psbt flag")
+	}
 
 	stream, err := client.OpenChannel(ctxb, req)
 	if err != nil {
@@ -388,6 +401,7 @@ func openChannelPsbt(ctx *cli.Context, client lnrpc.LightningClient,
 			PsbtShim: &lnrpc.PsbtShim{
 				PendingChanId: pendingChanID[:],
 				BasePsbt:      basePsbtBytes,
+				NoPublish:     ctx.Bool("no_publish"),
 			},
 		},
 	}

docs/psbt.md

@@ -231,3 +231,62 @@ Base64 encoded PSBT: cHNidP8BAH0CAAAAAbxLLf9+AYfqfF69QAQuETnL6cas7GDiWBZF+3xxc/Y
 Success! We now have the final transaction ID of the published funding
 transaction. Now we only have to wait for some confirmations, then we can start
 using the freshly created channel.
+
+## Batch opening channels
+
+The PSBT channel funding flow makes it possible to open multiple channels in one
+transaction. This can be achieved by taking the initial PSBT returned by the
+`openchannel` and feed it into the `--base_psbt` parameter of the next
+`openchannel` command. This won't work with `bitcoind` though, as it cannot take
+a PSBT as partial input for the `walletcreatefundedpsbt` command.
+
+However, the `bitcoin-cli` examples from the command line can be combined into
+a single command. For example:
+
+Channel 1:
+```bash
+bitcoin-cli walletcreatefundedpsbt [] '[{"tb1qywvazres587w9wyy8uw03q8j9ek6gc9crwx4jvhqcmew4xzsvqcq3jjdja":0.01000000}]'
+```
+
+Channel 2:
+```bash
+bitcoin-cli walletcreatefundedpsbt [] '[{"tb1q53626fcwwtcdc942zaf4laqnr3vg5gv4g0hakd2h7fw2pmz6428sk3ezcx":0.01000000}]'
+```
+
+Combined command to get batch PSBT:
+```bash
+bitcoin-cli walletcreatefundedpsbt [] '[{"tb1q53626fcwwtcdc942zaf4laqnr3vg5gv4g0hakd2h7fw2pmz6428sk3ezcx":0.01000000},{"tb1qywvazres587w9wyy8uw03q8j9ek6gc9crwx4jvhqcmew4xzsvqcq3jjdja":0.01000000}]'
+```
+
+### Safety warning about batch transactions
+
+As mentioned before, the PSBT channel funding flow works by pausing the funding
+negotiation with the remote peer directly after the multisig keys have been
+exchanged. That means, the channel isn't fully opened yet at the time the PSBT
+is signed. This is fine for a single channel because the signed transaction is
+only published after the counter-signed commitment transactions were exchanged
+and the funds can be spent again by both parties.
+
+When doing batch transactions, **publishing** the whole transaction with
+multiple channel funding outputs **too early could lead to loss of funds**!
+
+For example, let's say we want to open two channels. We call `openchannel --psbt`
+two times, combine the funding addresses as shown above, verify the PSBT, sign
+it and finally paste it into the terminal of the first command. `lnd` then goes
+ahead and finishes the negotiations with peer 1. If successful, `lnd` publishes
+the transaction. In the meantime we paste the same PSBT into the second terminal
+window. But by now, the peer 2 for channel 2 has timed out our funding flow and
+aborts the negotiation. Normally this would be fine, we would just not publish
+the funding transaction. But in the batch case, channel 1 has already published
+the transaction that contains both channel outputs. But because we never got a
+signature from peer 2 to spend the funds now locked in a 2-of-2 multisig, the
+fund are lost (unless peer 2 cooperates in a complicated, manual recovery
+process).
+
+### Use --no_publish for batch transactions
+
+To mitigate the problem described in the section above, when open multiple
+channels in one batch transaction, it is **imperative to use the
+`--no_publish`** flag for each channel but the very last. This prevents the
+full batch transaction to be published before each and every single channel has
+fully completed its funding negotiation.

lnrpc/rpc.proto

@@ -1768,6 +1768,16 @@ message PsbtShim {
     non-empty, it must be a binary serialized PSBT.
     */
     bytes base_psbt = 2;
+
+    /*
+    If a channel should be part of a batch (multiple channel openings in one
+    transaction), it can be dangerous if the whole batch transaction is
+    published too early before all channel opening negotiations are completed.
+    This flag prevents this particular channel from broadcasting the transaction
+    after the negotiation with the remote peer. In a batch of channel openings
+    this flag should be set to true for every channel but the very last.
+    */
+    bool no_publish = 3;
 }
 
 message FundingShim {

lnrpc/rpc.swagger.json

@@ -4816,6 +4816,11 @@
           "type": "string",
           "format": "byte",
           "description": "An optional base PSBT the new channel output will be added to. If this is\nnon-empty, it must be a binary serialized PSBT."
+        },
+        "no_publish": {
+          "type": "boolean",
+          "format": "boolean",
+          "description": "If a channel should be part of a batch (multiple channel openings in one\ntransaction), it can be dangerous if the whole batch transaction is\npublished too early before all channel opening negotiations are completed.\nThis flag prevents this particular channel from broadcasting the transaction\nafter the negotiation with the remote peer. In a batch of channel openings\nthis flag should be set to true for every channel but the very last."
         }
       }
     },

lntest/itest/psbt.go

@@ -45,6 +45,10 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 	if err != nil {
 		t.Fatalf("unable to connect peers: %v", err)
 	}
+	err = net.EnsureConnected(ctxt, carol, net.Alice)
+	if err != nil {
+		t.Fatalf("unable to connect peers: %v", err)
+	}
 
 	// At this point, we can begin our PSBT channel funding workflow. We'll
 	// start by generating a pending channel ID externally that will be used
@@ -54,8 +58,15 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 		t.Fatalf("unable to gen pending chan ID: %v", err)
 	}
 
+	// We'll also test batch funding of two channels so we need another ID.
+	var pendingChanID2 [32]byte
+	if _, err := rand.Read(pendingChanID2[:]); err != nil {
+		t.Fatalf("unable to gen pending chan ID: %v", err)
+	}
+
 	// Now that we have the pending channel ID, Carol will open the channel
-	// by specifying a PSBT shim.
+	// by specifying a PSBT shim. We use the NoPublish flag here to avoid
+	// publishing the whole batch TX too early.
 	ctxt, cancel = context.WithTimeout(ctxb, defaultTimeout)
 	defer cancel()
 	chanUpdates, psbtBytes, err := openChannelPsbt(
@@ -65,23 +76,50 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 				Shim: &lnrpc.FundingShim_PsbtShim{
 					PsbtShim: &lnrpc.PsbtShim{
 						PendingChanId: pendingChanID[:],
+						NoPublish:     true,
 					},
 				},
 			},
 		},
 	)
 	if err != nil {
-		t.Fatalf("unable to open channel: %v", err)
+		t.Fatalf("unable to open channel to dave: %v", err)
 	}
 	packet, err := psbt.NewFromRawBytes(bytes.NewReader(psbtBytes), false)
 	if err != nil {
 		t.Fatalf("unable to parse returned PSBT: %v", err)
 	}
 
+	// Let's add a second channel to the batch. This time between carol and
+	// alice. We will the batch TX once this channel funding is complete.
+	ctxt, cancel = context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+	chanUpdates2, psbtBytes2, err := openChannelPsbt(
+		ctxt, carol, net.Alice, lntest.OpenChannelParams{
+			Amt: chanSize,
+			FundingShim: &lnrpc.FundingShim{
+				Shim: &lnrpc.FundingShim_PsbtShim{
+					PsbtShim: &lnrpc.PsbtShim{
+						PendingChanId: pendingChanID2[:],
+						NoPublish:     false,
+					},
+				},
+			},
+		},
+	)
+	if err != nil {
+		t.Fatalf("unable to open channel to alice: %v", err)
+	}
+	packet2, err := psbt.NewFromRawBytes(bytes.NewReader(psbtBytes2), false)
+	if err != nil {
+		t.Fatalf("unable to parse returned PSBT: %v", err)
+	}
+
 	// We'll now create a fully signed transaction that sends to the outputs
 	// encoded in the PSBT. We'll let the miner do it and convert the final
 	// TX into a PSBT, that's way easier than assembling a PSBT manually.
-	tx, err := net.Miner.CreateTransaction(packet.UnsignedTx.TxOut, 5, true)
+	allOuts := append(packet.UnsignedTx.TxOut, packet2.UnsignedTx.TxOut...)
+	tx, err := net.Miner.CreateTransaction(allOuts, 5, true)
 	if err != nil {
 		t.Fatalf("unable to create funding transaction: %v", err)
 	}
@@ -119,7 +157,7 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	// We have a PSBT that has no witness data yet, which is exactly what we
-	// need for the next step: Verify the PSBT with the funding intent.
+	// need for the next step: Verify the PSBT with the funding intents.
 	_, err = carol.FundingStateStep(ctxb, &lnrpc.FundingTransitionMsg{
 		Trigger: &lnrpc.FundingTransitionMsg_PsbtVerify{
 			PsbtVerify: &lnrpc.FundingPsbtVerify{
@@ -131,6 +169,17 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 	if err != nil {
 		t.Fatalf("error verifying PSBT with funding intent: %v", err)
 	}
+	_, err = carol.FundingStateStep(ctxb, &lnrpc.FundingTransitionMsg{
+		Trigger: &lnrpc.FundingTransitionMsg_PsbtVerify{
+			PsbtVerify: &lnrpc.FundingPsbtVerify{
+				PendingChanId: pendingChanID2[:],
+				FundedPsbt:    buf.Bytes(),
+			},
+		},
+	})
+	if err != nil {
+		t.Fatalf("error verifying PSBT with funding intent 2: %v", err)
+	}
 
 	// Now we'll add the witness data back into the PSBT to make it a
 	// complete and signed transaction that can be finalized. We'll trick
@@ -162,7 +211,7 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	// Consume the "channel pending" update. This waits until the funding
-	// transaction has been published.
+	// transaction was fully compiled.
 	ctxt, cancel = context.WithTimeout(ctxb, defaultTimeout)
 	defer cancel()
 	updateResp, err := receiveChanUpdate(ctxt, chanUpdates)
@@ -181,6 +230,49 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 		OutputIndex: upd.ChanPending.OutputIndex,
 	}
 
+	// No transaction should have been published yet.
+	mempool, err := net.Miner.Node.GetRawMempool()
+	if err != nil {
+		t.Fatalf("error querying mempool: %v", err)
+	}
+	if len(mempool) != 0 {
+		t.Fatalf("unexpected txes in mempool: %v", mempool)
+	}
+
+	// Let's progress the second channel now.
+	_, err = carol.FundingStateStep(ctxb, &lnrpc.FundingTransitionMsg{
+		Trigger: &lnrpc.FundingTransitionMsg_PsbtFinalize{
+			PsbtFinalize: &lnrpc.FundingPsbtFinalize{
+				PendingChanId: pendingChanID2[:],
+				SignedPsbt:    buf.Bytes(),
+			},
+		},
+	})
+	if err != nil {
+		t.Fatalf("error finalizing PSBT with funding intent 2: %v", err)
+	}
+
+	// Consume the "channel pending" update for the second channel. This
+	// waits until the funding transaction was fully compiled and in this
+	// case published.
+	ctxt, cancel = context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+	updateResp2, err := receiveChanUpdate(ctxt, chanUpdates2)
+	if err != nil {
+		t.Fatalf("unable to consume channel update message: %v", err)
+	}
+	upd2, ok := updateResp2.Update.(*lnrpc.OpenStatusUpdate_ChanPending)
+	if !ok {
+		t.Fatalf("expected PSBT funding update, instead got %v",
+			updateResp2)
+	}
+	chanPoint2 := &lnrpc.ChannelPoint{
+		FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{
+			FundingTxidBytes: upd2.ChanPending.Txid,
+		},
+		OutputIndex: upd2.ChanPending.OutputIndex,
+	}
+
 	// Great, now we can mine a block to get the transaction confirmed, then
 	// wait for the new channel to be propagated through the network.
 	txHash := tx.TxHash()
@@ -192,6 +284,10 @@ func testPsbtChanFunding(net *lntest.NetworkHarness, t *harnessTest) {
 	if err != nil {
 		t.Fatalf("carol didn't report channel: %v", err)
 	}
+	err = carol.WaitForNetworkChannelOpen(ctxt, chanPoint2)
+	if err != nil {
+		t.Fatalf("carol didn't report channel 2: %v", err)
+	}
 
 	// With the channel open, ensure that it is counted towards Carol's
 	// total channel balance.

lnwallet/chanfunding/assembler.go

@@ -135,3 +135,18 @@ type FundingTxAssembler interface {
 	// transaction for the channel via the intent it returns.
 	FundingTxAvailable()
 }
+
+// ConditionalPublishAssembler is an assembler that can dynamically define if
+// the funding transaction should be published after channel negotiations or
+// not. Not publishing the transaction is only useful if the particular channel
+// the assembler is in charge of is part of a batch of channels. In that case
+// it is only safe to wait for all channel negotiations of the batch to complete
+// before publishing the batch transaction.
+type ConditionalPublishAssembler interface {
+	Assembler
+
+	// ShouldPublishFundingTx is a method of the assembler that signals if
+	// the funding transaction should be published after the channel
+	// negotiations are completed with the remote peer.
+	ShouldPublishFundingTx() bool
+}

lnwallet/chanfunding/psbt_assembler.go

@@ -387,6 +387,10 @@ type PsbtAssembler struct {
 	// netParams are the network parameters used to encode the P2WSH funding
 	// address.
 	netParams *chaincfg.Params
+
+	// shouldPublish specifies if the assembler should publish the
+	// transaction once the channel funding has completed.
+	shouldPublish bool
 }
 
 // NewPsbtAssembler creates a new CannedAssembler from the material required
@@ -394,12 +398,13 @@ type PsbtAssembler struct {
 // be supplied which will be used to add the channel output to instead of
 // creating a new one.
 func NewPsbtAssembler(fundingAmt btcutil.Amount, basePsbt *psbt.Packet,
-	netParams *chaincfg.Params) *PsbtAssembler {
+	netParams *chaincfg.Params, shouldPublish bool) *PsbtAssembler {
 
 	return &PsbtAssembler{
-		fundingAmt: fundingAmt,
-		basePsbt:   basePsbt,
-		netParams:  netParams,
+		fundingAmt:    fundingAmt,
+		basePsbt:      basePsbt,
+		netParams:     netParams,
+		shouldPublish: shouldPublish,
 	}
 }
 
@@ -436,16 +441,18 @@ func (p *PsbtAssembler) ProvisionChannel(req *Request) (Intent, error) {
 	return intent, nil
 }
 
-// FundingTxAvailable is an empty method that an assembler can implement to
-// signal to callers that its able to provide the funding transaction for the
-// channel via the intent it returns.
+// ShouldPublishFundingTx is a method of the assembler that signals if the
+// funding transaction should be published after the channel negotiations are
+// completed with the remote peer.
 //
-// NOTE: This method is a part of the FundingTxAssembler interface.
-func (p *PsbtAssembler) FundingTxAvailable() {}
+// NOTE: This method is a part of the ConditionalPublishAssembler interface.
+func (p *PsbtAssembler) ShouldPublishFundingTx() bool {
+	return p.shouldPublish
+}
 
-// A compile-time assertion to ensure PsbtAssembler meets the Assembler
-// interface.
-var _ Assembler = (*PsbtAssembler)(nil)
+// A compile-time assertion to ensure PsbtAssembler meets the
+// ConditionalPublishAssembler interface.
+var _ ConditionalPublishAssembler = (*PsbtAssembler)(nil)
 
 // sumUtxoInputValues tries to extract the sum of all inputs specified in the
 // UTXO fields of the PSBT. An error is returned if an input is specified that

lnwallet/chanfunding/psbt_assembler_test.go

@@ -34,7 +34,7 @@ func TestPsbtIntent(t *testing.T) {
 
 	// Create a simple assembler and ask it to provision a channel to get
 	// the funding intent.
-	a := NewPsbtAssembler(chanCapacity, nil, &params)
+	a := NewPsbtAssembler(chanCapacity, nil, &params, true)
 	intent, err := a.ProvisionChannel(&Request{LocalAmt: chanCapacity})
 	if err != nil {
 		t.Fatalf("error provisioning channel: %v", err)
@@ -215,7 +215,7 @@ func TestPsbtIntentBasePsbt(t *testing.T) {
 
 	// Now as the next step, create a new assembler/intent pair with a base
 	// PSBT to see that we can add an additional output to it.
-	a := NewPsbtAssembler(chanCapacity, pendingPsbt, &params)
+	a := NewPsbtAssembler(chanCapacity, pendingPsbt, &params, true)
 	intent, err := a.ProvisionChannel(&Request{LocalAmt: chanCapacity})
 	if err != nil {
 		t.Fatalf("error provisioning channel: %v", err)
@@ -373,7 +373,7 @@ func TestPsbtVerify(t *testing.T) {
 
 	// Create a simple assembler and ask it to provision a channel to get
 	// the funding intent.
-	a := NewPsbtAssembler(chanCapacity, nil, &params)
+	a := NewPsbtAssembler(chanCapacity, nil, &params, true)
 	intent, err := a.ProvisionChannel(&Request{LocalAmt: chanCapacity})
 	if err != nil {
 		t.Fatalf("error provisioning channel: %v", err)
@@ -500,7 +500,7 @@ func TestPsbtFinalize(t *testing.T) {
 
 	// Create a simple assembler and ask it to provision a channel to get
 	// the funding intent.
-	a := NewPsbtAssembler(chanCapacity, nil, &params)
+	a := NewPsbtAssembler(chanCapacity, nil, &params, true)
 	intent, err := a.ProvisionChannel(&Request{LocalAmt: chanCapacity})
 	if err != nil {
 		t.Fatalf("error provisioning channel: %v", err)

lnwallet/reservation.go

@@ -287,10 +287,24 @@ func NewChannelReservation(capacity, localFundingAmt btcutil.Amount,
 			chanType |= channeldb.SingleFunderBit
 		}
 
+		switch a := fundingAssembler.(type) {
+		// The first channels of a batch shouldn't publish the batch TX
+		// to avoid problems if some of the funding flows can't be
+		// completed. Only the last channel of a batch should publish.
+		case chanfunding.ConditionalPublishAssembler:
+			if !a.ShouldPublishFundingTx() {
+				chanType |= channeldb.NoFundingTxBit
+			}
+
+		// Normal funding flow, the assembler creates a TX from the
+		// internal wallet.
+		case chanfunding.FundingTxAssembler:
+			// Do nothing, a FundingTxAssembler has the transaction.
+
 		// If this intent isn't one that's able to provide us with a
 		// funding transaction, then we'll set the chanType bit to
 		// signal that we don't have access to one.
-		if _, ok := fundingAssembler.(chanfunding.FundingTxAssembler); !ok {
+		default:
 			chanType |= channeldb.NoFundingTxBit
 		}
 

rpcserver.go

@@ -1672,6 +1672,7 @@ func newPsbtAssembler(req *lnrpc.OpenChannelRequest, normalizedMinConfs int32,
 	// to pass into the wallet.
 	return chanfunding.NewPsbtAssembler(
 		btcutil.Amount(req.LocalFundingAmount), packet, netParams,
+		!psbtShim.NoPublish,
 	), nil
 }