From 7b42ad0b0eec39a0365765140e4bf17efe257d2e Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 20 Apr 2021 18:18:44 -0500 Subject: [PATCH 1/5] channeldb: add new method for adhoc zombie chan creation In this commit, we add a new method that allows us to mark a channel as being a zombie on the fly without needing to go through the normal channel deletion process. --- channeldb/graph.go | 32 ++++++++++++++++++++++++++++++++ channeldb/graph_test.go | 14 ++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/channeldb/graph.go b/channeldb/graph.go index fac890178..b3d1924c6 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -3397,6 +3397,38 @@ func (c *ChannelGraph) NewChannelEdgePolicy() *ChannelEdgePolicy { return &ChannelEdgePolicy{db: c.db} } +// MarkEdgeZombie attempts to mark a channel identified by its channel ID as a +// zombie. This method is used on an ad-hoc basis, when channels need to be +// marked as zombies outside the normal pruning cycle. +func (c *ChannelGraph) MarkEdgeZombie(chanID uint64, + pubKey1, pubKey2 [33]byte) error { + + c.cacheMu.Lock() + defer c.cacheMu.Unlock() + + err := kvdb.Batch(c.db, func(tx kvdb.RwTx) error { + edges := tx.ReadWriteBucket(edgeBucket) + if edges == nil { + return ErrGraphNoEdgesFound + } + zombieIndex, err := edges.CreateBucketIfNotExists(zombieBucket) + if err != nil { + return fmt.Errorf("unable to create zombie "+ + "bucket: %w", err) + } + + return markEdgeZombie(zombieIndex, chanID, pubKey1, pubKey2) + }) + if err != nil { + return err + } + + c.rejectCache.remove(chanID) + c.chanCache.remove(chanID) + + return nil +} + // markEdgeZombie marks an edge as a zombie within our zombie index. The public // keys should represent the node public keys of the two parties involved in the // edge. diff --git a/channeldb/graph_test.go b/channeldb/graph_test.go index 0708c3ad1..b19ea82f4 100644 --- a/channeldb/graph_test.go +++ b/channeldb/graph_test.go @@ -3045,6 +3045,20 @@ func TestGraphZombieIndex(t *testing.T) { t.Fatal("expected edge to not be marked as zombie") } assertNumZombies(t, graph, 0) + + // If we mark the edge as a zombie manually, then it should show up as + // being a zombie once again. + err = graph.MarkEdgeZombie( + edge.ChannelID, node1.PubKeyBytes, node2.PubKeyBytes, + ) + if err != nil { + t.Fatalf("unable to mark edge as zombie: %v", err) + } + isZombie, _, _ = graph.IsZombieEdge(edge.ChannelID) + if !isZombie { + t.Fatal("expected edge to be marked as zombie") + } + assertNumZombies(t, graph, 1) } // compareNodes is used to compare two LightningNodes while excluding the From 0dc6f8058d0f947908b6bdce664be405c4e5aa7c Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 20 Apr 2021 18:22:36 -0500 Subject: [PATCH 2/5] routing: add new error for spent channel UTXOs --- routing/errors.go | 5 +++++ routing/router.go | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/routing/errors.go b/routing/errors.go index a8bd6fbaa..ff055d2eb 100644 --- a/routing/errors.go +++ b/routing/errors.go @@ -15,6 +15,11 @@ const ( // this update can't bring us something new, or because a node // announcement was given for node not found in any channel. ErrIgnored + + // ErrChannelSpent is returned when we go to validate a channel, but + // the purported funding output has actually already been spent on + // chain. + ErrChannelSpent ) // routerError is a structure that represent the error inside the routing package, diff --git a/routing/router.go b/routing/router.go index a1f502154..673b2cefc 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1354,7 +1354,8 @@ func (r *ChannelRouter) processUpdate(msg interface{}, r.quit, ) if err != nil { - return fmt.Errorf("unable to fetch utxo "+ + + return newErrf(ErrChannelSpent, "unable to fetch utxo "+ "for chan_id=%v, chan_point=%v: %v", msg.ChannelID, fundingPoint, err) } From 897a19d9dfd35b8e4c6d8632c57f0aa28c1b3bac Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 20 Apr 2021 18:23:10 -0500 Subject: [PATCH 3/5] routing: add new error for invalid funding tx rejection --- routing/errors.go | 4 ++++ routing/router.go | 5 +++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/routing/errors.go b/routing/errors.go index ff055d2eb..b7421e6d4 100644 --- a/routing/errors.go +++ b/routing/errors.go @@ -20,6 +20,10 @@ const ( // the purported funding output has actually already been spent on // chain. ErrChannelSpent + + // ErrNoFundingTransaction is returned when we are unable to find the + // funding transaction described by the short channel ID on chain. + ErrNoFundingTransaction ) // routerError is a structure that represent the error inside the routing package, diff --git a/routing/router.go b/routing/router.go index 673b2cefc..7cb3a5269 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1309,8 +1309,9 @@ func (r *ChannelRouter) processUpdate(msg interface{}, channelID := lnwire.NewShortChanIDFromInt(msg.ChannelID) fundingTx, err := r.fetchFundingTx(&channelID) if err != nil { - return errors.Errorf("unable to fetch funding tx for "+ - "chan_id=%v: %v", msg.ChannelID, err) + return newErrf(ErrNoFundingTransaction, "unable to "+ + "fetch funding tx for chan_id=%v: %v", + msg.ChannelID, err) } // Recreate witness output to be sure that declared in channel From 92c47983cb95b6e3f93fdb5619f4ba58486beb1c Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 20 Apr 2021 18:24:34 -0500 Subject: [PATCH 4/5] routing: add chans rejected due to failed chain validation to zombie index In this commit, we start to add any channels that fail the normal chain validation to the zombie index. With this change, we'll ensure that we won't continue to re-process the same set of spent channels over and over again. Fixes #5191. --- routing/notifications_test.go | 14 ++++- routing/router.go | 49 +++++++++++++++ routing/router_test.go | 112 ++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+), 3 deletions(-) diff --git a/routing/notifications_test.go b/routing/notifications_test.go index 07e8e3a34..c5f9a6c43 100644 --- a/routing/notifications_test.go +++ b/routing/notifications_test.go @@ -19,6 +19,7 @@ import ( "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/btcwallet" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/routing/chainview" "github.com/lightningnetwork/lnd/routing/route" @@ -170,8 +171,8 @@ func (m *mockChain) GetBlockHash(blockHeight int64) (*chainhash.Hash, error) { hash, ok := m.blockIndex[uint32(blockHeight)] if !ok { - return nil, fmt.Errorf("can't find block hash, for "+ - "height %v", blockHeight) + return nil, fmt.Errorf("block number out of range: %v", + blockHeight) } return &hash, nil @@ -182,6 +183,13 @@ func (m *mockChain) addUtxo(op wire.OutPoint, out *wire.TxOut) { m.utxos[op] = *out m.Unlock() } + +func (m *mockChain) delUtxo(op wire.OutPoint) { + m.Lock() + delete(m.utxos, op) + m.Unlock() +} + func (m *mockChain) GetUtxo(op *wire.OutPoint, _ []byte, _ uint32, _ <-chan struct{}) (*wire.TxOut, error) { m.RLock() @@ -189,7 +197,7 @@ func (m *mockChain) GetUtxo(op *wire.OutPoint, _ []byte, _ uint32, utxo, ok := m.utxos[*op] if !ok { - return nil, fmt.Errorf("utxo not found") + return nil, btcwallet.ErrOutputSpent } return &utxo, nil diff --git a/routing/router.go b/routing/router.go index 7cb3a5269..bbee6a65a 100644 --- a/routing/router.go +++ b/routing/router.go @@ -4,6 +4,7 @@ import ( "bytes" "fmt" "runtime" + "strings" "sync" "sync/atomic" "time" @@ -23,6 +24,7 @@ import ( "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/lntypes" "github.com/lightningnetwork/lnd/lnwallet" + "github.com/lightningnetwork/lnd/lnwallet/btcwallet" "github.com/lightningnetwork/lnd/lnwallet/chanvalidate" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/multimutex" @@ -1309,6 +1311,37 @@ func (r *ChannelRouter) processUpdate(msg interface{}, channelID := lnwire.NewShortChanIDFromInt(msg.ChannelID) fundingTx, err := r.fetchFundingTx(&channelID) if err != nil { + // In order to ensure we don't errnosuly mark a channel as + // a zmobie due to an RPC failure, we'll attempt to string + // match for the relevant errors. + // + // * btcd: + // * https://github.com/btcsuite/btcd/blob/master/rpcserver.go#L1316 + // * https://github.com/btcsuite/btcd/blob/master/rpcserver.go#L1086 + // * bitcoind: + // * https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/rpc/blockchain.cpp#L770 + // * https://github.com/bitcoin/bitcoin/blob/7fcf53f7b4524572d1d0c9a5fdc388e87eb02416/src/rpc/blockchain.cpp#L954 + switch { + case strings.Contains(err.Error(), "not found"): + fallthrough + + case strings.Contains(err.Error(), "out of range"): + // If the funding transaction isn't found at all, then + // we'll mark the edge itself as a zombie so we don't + // continue to request it. We use the "zero key" for + // both node pubkeys so this edge can't be resurrected. + var zeroKey [33]byte + zErr := r.cfg.Graph.MarkEdgeZombie( + msg.ChannelID, zeroKey, zeroKey, + ) + if zErr != nil { + return fmt.Errorf("unable to mark spent "+ + "chan(id=%v) as a zombie: %w", msg.ChannelID, + zErr) + } + default: + } + return newErrf(ErrNoFundingTransaction, "unable to "+ "fetch funding tx for chan_id=%v: %v", msg.ChannelID, err) @@ -1355,6 +1388,22 @@ func (r *ChannelRouter) processUpdate(msg interface{}, r.quit, ) if err != nil { + // If we fail validation of the UTXO here, then we'll + // mark the channel as a zombie as otherwise, we may + // continue to continue to request it. We use the "zero + // key" for both node pubkeys so this edge can't be + // resurrected. + if errors.Is(err, btcwallet.ErrOutputSpent) { + var zeroKey [33]byte + zErr := r.cfg.Graph.MarkEdgeZombie( + msg.ChannelID, zeroKey, zeroKey, + ) + if zErr != nil { + return fmt.Errorf("unable to mark spent "+ + "chan(id=%v) as a zombie: %w", msg.ChannelID, + zErr) + } + } return newErrf(ErrChannelSpent, "unable to fetch utxo "+ "for chan_id=%v, chan_point=%v: %v", diff --git a/routing/router_test.go b/routing/router_test.go index c530daec8..b5de51b9f 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -3136,3 +3136,115 @@ func TestBuildRoute(t *testing.T) { t.Fatalf("unexpected no channel error node") } } + +// edgeCreationModifier is an enum-like type used to modify steps that are +// skipped when creating a channel in the test context. +type edgeCreationModifier uint8 + +const ( + // edgeCreationNoFundingTx is used to skip adding the funding + // transaction of an edge to the chain. + edgeCreationNoFundingTx edgeCreationModifier = iota + + // edgeCreationNoUTXO is used to skip adding the UTXO of a channel to + // the UTXO set. + edgeCreationNoUTXO +) + +// newChannelEdgeInfo is a helper function used to create a new channel edge, +// possibly skipping adding it to parts of the chain/state as well. +func newChannelEdgeInfo(ctx *testCtx, fundingHeight uint32, + ecm edgeCreationModifier) (*channeldb.ChannelEdgeInfo, error) { + + node1, err := createTestNode() + if err != nil { + return nil, err + } + node2, err := createTestNode() + if err != nil { + return nil, err + } + + fundingTx, _, chanID, err := createChannelEdge( + ctx, bitcoinKey1.SerializeCompressed(), + bitcoinKey2.SerializeCompressed(), 100, fundingHeight, + ) + if err != nil { + return nil, fmt.Errorf("unable to create edge: %w", err) + } + + edge := &channeldb.ChannelEdgeInfo{ + ChannelID: chanID.ToUint64(), + NodeKey1Bytes: node1.PubKeyBytes, + NodeKey2Bytes: node2.PubKeyBytes, + } + copy(edge.BitcoinKey1Bytes[:], bitcoinKey1.SerializeCompressed()) + copy(edge.BitcoinKey2Bytes[:], bitcoinKey2.SerializeCompressed()) + + if ecm == edgeCreationNoFundingTx { + return edge, nil + } + + fundingBlock := &wire.MsgBlock{ + Transactions: []*wire.MsgTx{fundingTx}, + } + ctx.chain.addBlock(fundingBlock, chanID.BlockHeight, chanID.BlockHeight) + + if ecm == edgeCreationNoUTXO { + ctx.chain.delUtxo(wire.OutPoint{ + Hash: fundingTx.TxHash(), + }) + } + + return edge, nil +} + +func assertChanChainRejection(t *testing.T, ctx *testCtx, + edge *channeldb.ChannelEdgeInfo, failCode errorCode) { + + t.Helper() + + err := ctx.router.AddEdge(edge) + if !IsError(err, failCode) { + t.Fatalf("validation should have failed: %v", err) + } + + // This channel should now be present in the zombie channel index. + _, _, _, isZombie, err := ctx.graph.HasChannelEdge( + edge.ChannelID, + ) + require.Nil(t, err) + require.True(t, isZombie, "edge should be marked as zombie") +} + +// TestChannelOnChainRejectionZombie tests that if we fail validating a channel +// due to some sort of on-chain rejection (no funding transaction, or invalid +// UTXO), then we'll mark the channel as a zombie. +func TestChannelOnChainRejectionZombie(t *testing.T) { + t.Parallel() + + ctx, cleanup, err := createTestCtxSingleNode(0) + if err != nil { + t.Fatal(err) + } + defer cleanup() + + // To start, we'll make an edge for the channel, but we won't add the + // funding transaction to the mock blockchain, which should cause the + // validation to fail below. + edge, err := newChannelEdgeInfo(ctx, 1, edgeCreationNoFundingTx) + require.Nil(t, err) + + // We expect this to fail as the transaction isn't present in the + // chain (nor the block). + assertChanChainRejection(t, ctx, edge, ErrNoFundingTransaction) + + // Next, we'll make another channel edge, but actually add it to the + // graph this time. + edge, err = newChannelEdgeInfo(ctx, 2, edgeCreationNoUTXO) + require.Nil(t, err) + + // Instead now, we'll remove it from the set of UTXOs which should + // cause the spentness validation to fail. + assertChanChainRejection(t, ctx, edge, ErrChannelSpent) +} From e0ce384f02a24c82a4579bddf4e3ace741db6cb1 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 22 Apr 2021 18:41:15 -0500 Subject: [PATCH 5/5] routing: add new error for failed funding tx validation In this commit we add a new error for when we fail to validate the funding transaction (invalid script, etc) and mark it as a zombie like the other failed validation cases. --- routing/errors.go | 4 +++ routing/router.go | 73 ++++++++++++++++++++++++------------------ routing/router_test.go | 14 ++++++++ 3 files changed, 59 insertions(+), 32 deletions(-) diff --git a/routing/errors.go b/routing/errors.go index b7421e6d4..a5c7fd3ca 100644 --- a/routing/errors.go +++ b/routing/errors.go @@ -24,6 +24,10 @@ const ( // ErrNoFundingTransaction is returned when we are unable to find the // funding transaction described by the short channel ID on chain. ErrNoFundingTransaction + + // ErrInvalidFundingOutput is returned if the channle funding output + // fails validation. + ErrInvalidFundingOutput ) // routerError is a structure that represent the error inside the routing package, diff --git a/routing/router.go b/routing/router.go index bbee6a65a..8b91bbfd6 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1244,6 +1244,22 @@ func (r *ChannelRouter) assertNodeAnnFreshness(node route.Vertex, return nil } +// addZombieEdge adds a channel that failed complete validation into the zombie +// index so we can avoid having to re-validate it in the future. +func (r *ChannelRouter) addZombieEdge(chanID uint64) error { + // If the edge fails validation we'll mark the edge itself as a zombie + // so we don't continue to request it. We use the "zero key" for both + // node pubkeys so this edge can't be resurrected. + var zeroKey [33]byte + err := r.cfg.Graph.MarkEdgeZombie(chanID, zeroKey, zeroKey) + if err != nil { + return fmt.Errorf("unable to mark spent chan(id=%v) as a "+ + "zombie: %w", chanID, err) + } + + return nil +} + // processUpdate processes a new relate authenticated channel/edge, node or // channel/edge update network update. If the update didn't affect the internal // state of the draft due to either being out of date, invalid, or redundant, @@ -1311,9 +1327,9 @@ func (r *ChannelRouter) processUpdate(msg interface{}, channelID := lnwire.NewShortChanIDFromInt(msg.ChannelID) fundingTx, err := r.fetchFundingTx(&channelID) if err != nil { - // In order to ensure we don't errnosuly mark a channel as - // a zmobie due to an RPC failure, we'll attempt to string - // match for the relevant errors. + // In order to ensure we don't erroneously mark a + // channel as a zombie due to an RPC failure, we'll + // attempt to string match for the relevant errors. // // * btcd: // * https://github.com/btcsuite/btcd/blob/master/rpcserver.go#L1316 @@ -1326,25 +1342,21 @@ func (r *ChannelRouter) processUpdate(msg interface{}, fallthrough case strings.Contains(err.Error(), "out of range"): - // If the funding transaction isn't found at all, then - // we'll mark the edge itself as a zombie so we don't - // continue to request it. We use the "zero key" for - // both node pubkeys so this edge can't be resurrected. - var zeroKey [33]byte - zErr := r.cfg.Graph.MarkEdgeZombie( - msg.ChannelID, zeroKey, zeroKey, - ) + // If the funding transaction isn't found at + // all, then we'll mark the edge itself as a + // zombie so we don't continue to request it. + // We use the "zero key" for both node pubkeys + // so this edge can't be resurrected. + zErr := r.addZombieEdge(msg.ChannelID) if zErr != nil { - return fmt.Errorf("unable to mark spent "+ - "chan(id=%v) as a zombie: %w", msg.ChannelID, - zErr) + return zErr } + default: } return newErrf(ErrNoFundingTransaction, "unable to "+ - "fetch funding tx for chan_id=%v: %v", - msg.ChannelID, err) + "locate funding tx: %v", err) } // Recreate witness output to be sure that declared in channel @@ -1373,7 +1385,14 @@ func (r *ChannelRouter) processUpdate(msg interface{}, FundingTx: fundingTx, }) if err != nil { - return err + // Mark the edge as a zombie so we won't try to + // re-validate it on start up. + if err := r.addZombieEdge(msg.ChannelID); err != nil { + return err + } + + return newErrf(ErrInvalidFundingOutput, "output "+ + "failed validation: %w", err) } // Now that we have the funding outpoint of the channel, ensure @@ -1388,20 +1407,10 @@ func (r *ChannelRouter) processUpdate(msg interface{}, r.quit, ) if err != nil { - // If we fail validation of the UTXO here, then we'll - // mark the channel as a zombie as otherwise, we may - // continue to continue to request it. We use the "zero - // key" for both node pubkeys so this edge can't be - // resurrected. if errors.Is(err, btcwallet.ErrOutputSpent) { - var zeroKey [33]byte - zErr := r.cfg.Graph.MarkEdgeZombie( - msg.ChannelID, zeroKey, zeroKey, - ) + zErr := r.addZombieEdge(msg.ChannelID) if zErr != nil { - return fmt.Errorf("unable to mark spent "+ - "chan(id=%v) as a zombie: %w", msg.ChannelID, - zErr) + return zErr } } @@ -1555,9 +1564,9 @@ func (r *ChannelRouter) fetchFundingTx( // block. numTxns := uint32(len(fundingBlock.Transactions)) if chanID.TxIndex > numTxns-1 { - return nil, fmt.Errorf("tx_index=#%v is out of range "+ - "(max_index=%v), network_chan_id=%v", chanID.TxIndex, - numTxns-1, chanID) + return nil, fmt.Errorf("tx_index=#%v "+ + "is out of range (max_index=%v), network_chan_id=%v", + chanID.TxIndex, numTxns-1, chanID) } return fundingBlock.Transactions[chanID.TxIndex], nil diff --git a/routing/router_test.go b/routing/router_test.go index b5de51b9f..144ea13d0 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -3149,6 +3149,10 @@ const ( // edgeCreationNoUTXO is used to skip adding the UTXO of a channel to // the UTXO set. edgeCreationNoUTXO + + // edgeCreationBadScript is used to create the edge, but use the wrong + // scrip which should cause it to fail output validation. + edgeCreationBadScript ) // newChannelEdgeInfo is a helper function used to create a new channel edge, @@ -3196,6 +3200,10 @@ func newChannelEdgeInfo(ctx *testCtx, fundingHeight uint32, }) } + if ecm == edgeCreationBadScript { + fundingTx.TxOut[0].PkScript[0] ^= 1 + } + return edge, nil } @@ -3247,4 +3255,10 @@ func TestChannelOnChainRejectionZombie(t *testing.T) { // Instead now, we'll remove it from the set of UTXOs which should // cause the spentness validation to fail. assertChanChainRejection(t, ctx, edge, ErrChannelSpent) + + // If we cause the funding transaction the chain to fail validation, we + // should see similar behavior. + edge, err = newChannelEdgeInfo(ctx, 3, edgeCreationBadScript) + require.Nil(t, err) + assertChanChainRejection(t, ctx, edge, ErrInvalidFundingOutput) }