Merge pull request #1030 from lightninglabs/channel-bug-fixes

custom channels: more bug fixes

Author: Roasbeef

cmd/tapcli/assets.go

@@ -47,6 +47,7 @@ var (
 	assetGroupedAssetName        = "grouped_asset"
 	assetShowWitnessName         = "show_witness"
 	assetShowSpentName           = "show_spent"
+	assetShowUnconfMintsName     = "show_unconfirmed_mints"
 	assetGroupKeyName            = "group_key"
 	assetGroupAnchorName         = "group_anchor"
 	batchKeyName                 = "batch_key"
@@ -554,6 +555,11 @@ var listAssetsCommand = cli.Command{
 			Name:  assetShowSpentName,
 			Usage: "include fully spent assets in the list",
 		},
+		cli.BoolFlag{
+			Name: assetShowUnconfMintsName,
+			Usage: "include freshly minted and not yet confirmed " +
+				"assets in the list",
+		},
 	},
 	Action: listAssets,
 }
@@ -566,8 +572,9 @@ func listAssets(ctx *cli.Context) error {
 	// TODO(roasbeef): need to reverse txid
 
 	resp, err := client.ListAssets(ctxc, &taprpc.ListAssetRequest{
-		WithWitness:  ctx.Bool(assetShowWitnessName),
-		IncludeSpent: ctx.Bool(assetShowSpentName),
+		WithWitness:             ctx.Bool(assetShowWitnessName),
+		IncludeSpent:            ctx.Bool(assetShowSpentName),
+		IncludeUnconfirmedMints: ctx.Bool(assetShowUnconfMintsName),
 	})
 	if err != nil {
 		return fmt.Errorf("unable to list assets: %w", err)

itest/assertions.go

@@ -1826,6 +1826,11 @@ func AssertAssetsMinted(t *testing.T, tapClient TapdClient,
 						firstOutpoint)
 				}
 
+				if anchor.BlockHeight == 0 {
+					return fmt.Errorf("missing block " +
+						"height")
+				}
+
 				return nil
 			},
 			AssetScriptKeyCheck(assetRequest.Asset.ScriptKey),

itest/utils.go

@@ -358,7 +358,9 @@ func FinalizeBatchUnconfirmed(t *testing.T, minerClient *rpcclient.Client,
 	// Make sure the assets were all minted within the same anchor but don't
 	// yet have a block hash associated with them.
 	listRespUnconfirmed, err := tapClient.ListAssets(
-		ctxt, &taprpc.ListAssetRequest{},
+		ctxt, &taprpc.ListAssetRequest{
+			IncludeUnconfirmedMints: true,
+		},
 	)
 	require.NoError(t, err)
 

rpcserver.go

@@ -975,6 +975,37 @@ func (r *rpcServer) ListAssets(ctx context.Context,
 		return nil, err
 	}
 
+	var (
+		filteredAssets   []*taprpc.Asset
+		unconfirmedMints uint64
+	)
+
+	// We now count and filter the assets according to the
+	// IncludeUnconfirmedMints flag.
+	//
+	// TODO(guggero): Do this on the SQL level once we add pagination to the
+	// asset list query, as this will no longer work with pagination.
+	for idx := range rpcAssets {
+		switch {
+		// If the asset isn't confirmed yet, we count it but only
+		// include it in the output list if the client requested it.
+		case rpcAssets[idx].ChainAnchor.BlockHeight == 0:
+			unconfirmedMints++
+
+			if req.IncludeUnconfirmedMints {
+				filteredAssets = append(
+					filteredAssets, rpcAssets[idx],
+				)
+			}
+
+		// Don't filter out confirmed assets.
+		default:
+			filteredAssets = append(
+				filteredAssets, rpcAssets[idx],
+			)
+		}
+	}
+
 	// We will also report the number of unconfirmed transfers. This is
 	// useful for clients as unconfirmed asset coins are not included in the
 	// asset list.
@@ -985,8 +1016,9 @@ func (r *rpcServer) ListAssets(ctx context.Context,
 	}
 
 	return &taprpc.ListAssetResponse{
-		Assets:               rpcAssets,
+		Assets:               filteredAssets,
 		UnconfirmedTransfers: uint64(len(outboundParcels)),
+		UnconfirmedMints:     unconfirmedMints,
 	}, nil
 }
 

tapchannel/aux_funding_controller.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"context"
 	crand "crypto/rand"
+	"errors"
 	"fmt"
 	"io"
 	"net/url"
@@ -1264,7 +1265,7 @@ func (f *FundingController) processFundingMsg(ctx context.Context,
 		)
 		if err != nil {
 			return tempPID, fmt.Errorf("unable to create "+
-				"commitment: %v", err)
+				"commitment: %w", err)
 		}
 
 		// Do we expect more proofs to be incoming?
@@ -1431,19 +1432,30 @@ func (f *FundingController) processFundingReq(fundingFlows fundingFlowIndex,
 				fundReq.ctx, f.cfg.ChainWallet,
 			)
 			if uErr != nil {
-				log.Errorf("unable to unlock "+
-					"inputs: %v", uErr)
+				log.Errorf("unable to unlock inputs: %v", uErr)
 			}
 
 			uErr = fundingState.unlockAssetInputs(
 				fundReq.ctx, f.cfg.CoinSelector,
 			)
 			if uErr != nil {
-				log.Errorf("Unable to unlock "+
-					"asset inputs: %v",
+				log.Errorf("Unable to unlock asset inputs: %v",
 					uErr)
 			}
 
+			// If anything went wrong during the funding process,
+			// the remote side might have an in-memory state and
+			// wouldn't allow us to try again within the next 10
+			// minutes (due to only one pending channel per peer
+			// default value). To avoid running into this issue, we
+			// make sure to inform the remote about us aborting the
+			// channel. We don't send them the actual error though,
+			// that would give away too much information.
+			f.cfg.ErrReporter.ReportError(
+				fundReq.ctx, fundReq.PeerPub, tempPID,
+				errors.New("internal error"),
+			)
+
 			fundReq.errChan <- err
 			return
 		}

tapchannel/commitment.go

@@ -667,9 +667,8 @@ func CreateAllocations(chanState *channeldb.OpenChannel, ourBalance,
 
 	// Next, we add the HTLC outputs, using this helper function to
 	// distinguish between incoming and outgoing HTLCs. The haveHtlcSplit
-	// boolean is used to determine if one of the HTLCs needs to become the
-	// split root. See below where it's used for a more in-depth
-	// explanation.
+	// boolean is used to store if one of the HTLCs has already been chosen
+	// to be the split root (only the very first HTLC might be chosen).
 	var haveHtlcSplitRoot bool
 	addHtlc := func(htlc *DecodedDescriptor, isIncoming bool) error {
 		htlcScript, err := lnwallet.GenTaprootHtlcScript(
@@ -688,34 +687,14 @@ func CreateAllocations(chanState *channeldb.OpenChannel, ourBalance,
 				"sibling: %w", err)
 		}
 
-		// The "incoming" vs. "outgoing" naming is always viewed from
-		// the perspective of the local node and independent of
-		// isOurCommit. So to find out if an HTLC is for the initiator
-		// it only depends on the direction and whether we are the
-		// initiator. The human-readable version of the below switch
-		// statement is: Either we're the initiator and the HTLC is
-		// incoming, or we're not the initiator and the HTLC is
-		// outgoing.
-		var htlcIsForInitiator bool
-		switch {
-		// If the HTLC is incoming and the local node is the channel
-		// initiator, then the HTLC is for the channel initiator.
-		case chanState.IsInitiator && isIncoming:
-			htlcIsForInitiator = true
-
-		// If the HTLC is outgoing and the local node is not the
-		// channel initiator, then the HTLC is for the channel
-		// initiator.
-		case !chanState.IsInitiator && !isIncoming:
-			htlcIsForInitiator = true
-		}
-
-		// We should always just have a single split root on the side
-		// of the initiator. So if the initiator doesn't have any normal
-		// asset output to house the split root, then we'll take the
-		// _first_ HTLC that pays to the initiator.
+		// We should always just have a single split root, which
+		// normally is the initiator's balance. However, if the
+		// initiator has no balance, then we choose the very first HTLC
+		// in the list to be the split root. If there are no HTLCs, then
+		// all the balance is on the receiver side and we don't need a
+		// split root.
 		shouldHouseSplitRoot := initiatorAssetBalance == 0 &&
-			htlcIsForInitiator && !haveHtlcSplitRoot
+			!haveHtlcSplitRoot
 
 		// Make sure we only select the very first HTLC that pays to the
 		// initiator.

tapdb/assets_store.go

@@ -755,11 +755,14 @@ func (a *AssetStore) dbAssetsToChainAssets(dbAssets []ConfirmedAsset,
 		}
 
 		chainAssets[i] = &asset.ChainAsset{
-			Asset:                  assetSprout,
-			IsSpent:                sprout.Spent,
-			AnchorTx:               anchorTx,
-			AnchorBlockHash:        anchorBlockHash,
-			AnchorOutpoint:         anchorOutpoint,
+			Asset:           assetSprout,
+			IsSpent:         sprout.Spent,
+			AnchorTx:        anchorTx,
+			AnchorBlockHash: anchorBlockHash,
+			AnchorOutpoint:  anchorOutpoint,
+			AnchorBlockHeight: uint32(
+				sprout.AnchorBlockHeight.Int32,
+			),
 			AnchorInternalKey:      anchorInternalKey,
 			AnchorMerkleRoot:       sprout.AnchorMerkleRoot,
 			AnchorTapscriptSibling: sprout.AnchorTapscriptSibling,
@@ -1820,6 +1823,11 @@ func (a *AssetStore) ListEligibleCoins(ctx context.Context,
 	assetFilter.Spent = sqlBool(false)
 	assetFilter.Leased = sqlBool(false)
 
+	// We also only want to select confirmed commitments (freshly minted
+	// unconfirmed assets would otherwise be included). Unconfirmed assets
+	// have a block height of 0, so we set the minimum block height to 1.
+	assetFilter.MinAnchorHeight = sqlInt32(1)
+
 	return a.queryCommitments(ctx, assetFilter)
 }
 

tapdb/assets_store_test.go

@@ -301,6 +301,7 @@ func TestImportAssetProof(t *testing.T) {
 		t, testProof.AssetSnapshot.OutPoint, dbAsset.AnchorOutpoint,
 	)
 	require.Equal(t, testProof.AnchorTx.TxHash(), dbAsset.AnchorTx.TxHash())
+	require.NotZero(t, dbAsset.AnchorBlockHeight)
 
 	// We should also be able to fetch the proof we just inserted using the
 	// script key of the new asset.

tapdb/sqlutils_test.go

@@ -105,7 +105,7 @@ func (d *DbHandler) AddRandomAssetProof(t *testing.T) (*asset.Asset,
 			Asset:             testAsset,
 			OutPoint:          anchorPoint,
 			AnchorBlockHash:   blockHash,
-			AnchorBlockHeight: test.RandInt[uint32](),
+			AnchorBlockHeight: uint32(test.RandIntn(1000) + 1),
 			AnchorTxIndex:     test.RandInt[uint32](),
 			AnchorTx:          anchorTx,
 			OutputIndex:       0,