From 87df6b71c5e4e0c6b677fbfe9d423d98fa430328 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:37 +0100 Subject: [PATCH 01/10] router test: only cleanup if context creation succeeds --- routing/router_test.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/routing/router_test.go b/routing/router_test.go index c184c3ce..c1b1e8bc 100644 --- a/routing/router_test.go +++ b/routing/router_test.go @@ -170,10 +170,10 @@ func TestFindRoutesFeeSorting(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // In this test we'd like to ensure proper integration of the various // functions that are involved in path finding, and also route @@ -225,10 +225,10 @@ func TestFindRoutesWithFeeLimit(t *testing.T) { ctx, cleanUp, err := createTestCtxFromFile( startingBlockHeight, basicGraphFilePath, ) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // This test will attempt to find routes from roasbeef to sophon for 100 // satoshis with a fee limit of 10 satoshis. There are two routes from @@ -280,10 +280,10 @@ func TestSendPaymentRouteFailureFallback(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // Craft a LightningPayment struct that'll send a payment from roasbeef // to luo ji for 1000 satoshis, with a maximum of 1000 satoshis in fees. @@ -516,10 +516,10 @@ func TestSendPaymentErrorRepeatedFeeInsufficient(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // Craft a LightningPayment struct that'll send a payment from roasbeef // to luo ji for 100 satoshis. @@ -620,10 +620,10 @@ func TestSendPaymentErrorNonFinalTimeLockErrors(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // Craft a LightningPayment struct that'll send a payment from roasbeef // to sophon for 1k satoshis. @@ -755,10 +755,10 @@ func TestSendPaymentErrorPathPruning(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // Craft a LightningPayment struct that'll send a payment from roasbeef // to luo ji for 1000 satoshis, with a maximum of 1000 satoshis in fees. @@ -997,10 +997,10 @@ func TestIgnoreNodeAnnouncement(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() pub := priv1.PubKey() node := &channeldb.LightningNode{ @@ -1030,10 +1030,10 @@ func TestAddEdgeUnknownVertexes(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() var pub1 [33]byte copy(pub1[:], priv1.PubKey().SerializeCompressed()) @@ -1299,10 +1299,10 @@ func TestWakeUpOnStaleBranch(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxSingleNode(startingBlockHeight) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() const chanValue = 10000 @@ -1502,10 +1502,10 @@ func TestDisconnectedBlocks(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxSingleNode(startingBlockHeight) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() const chanValue = 10000 @@ -1692,10 +1692,10 @@ func TestRouterChansClosedOfflinePruneGraph(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxSingleNode(startingBlockHeight) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() const chanValue = 10000 @@ -1845,10 +1845,10 @@ func TestFindPathFeeWeighting(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() var preImage [32]byte copy(preImage[:], bytes.Repeat([]byte{9}, 32)) @@ -1903,10 +1903,10 @@ func TestIsStaleNode(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxSingleNode(startingBlockHeight) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // Before we can insert a node in to the database, we need to create a // channel that it's linked to. @@ -1985,10 +1985,10 @@ func TestIsKnownEdge(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxSingleNode(startingBlockHeight) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // First, we'll create a new channel edge (just the info) and insert it // into the database. @@ -2038,10 +2038,10 @@ func TestIsStaleEdgePolicy(t *testing.T) { const startingBlockHeight = 101 ctx, cleanUp, err := createTestCtxFromFile(startingBlockHeight, basicGraphFilePath) - defer cleanUp() if err != nil { t.Fatalf("unable to create router: %v", err) } + defer cleanUp() // First, we'll create a new channel edge (just the info) and insert it // into the database. From 81fe6e73ed0f6b9148ce483caa223ebbd25bcb06 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 02/10] router+graph: return ErrGraphNodesNotFound if no nodes to prune Avoids creating a bucket unneccessarily. --- channeldb/graph.go | 6 +++--- routing/router.go | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 21680eb4..7cf7f898 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -795,9 +795,9 @@ func (c *ChannelGraph) PruneGraph(spentOutputs []*wire.OutPoint, // node gains more channels, it will be re-added back to the graph. func (c *ChannelGraph) PruneGraphNodes() error { return c.db.Update(func(tx *bbolt.Tx) error { - nodes, err := tx.CreateBucketIfNotExists(nodeBucket) - if err != nil { - return err + nodes := tx.Bucket(nodeBucket) + if nodes == nil { + return ErrGraphNodesNotFound } edges := tx.Bucket(edgeBucket) if edges == nil { diff --git a/routing/router.go b/routing/router.go index 0e1a9b26..d815592f 100644 --- a/routing/router.go +++ b/routing/router.go @@ -445,7 +445,8 @@ func (r *ChannelRouter) Start() error { // Finally, before we proceed, we'll prune any unconnected nodes from // the graph in order to ensure we maintain a tight graph of "useful" // nodes. - if err := r.cfg.Graph.PruneGraphNodes(); err != nil { + err = r.cfg.Graph.PruneGraphNodes() + if err != nil && err != channeldb.ErrGraphNodesNotFound { return err } From 5d8d99b7bcd553d19791f5bcb009a4a271edef50 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 03/10] channeldb/graph: don't create bucket in UpdateChannelEdge It would return ErrEdgeNotFound when edge not found in bucket anyway. --- channeldb/graph.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 7cf7f898..6a7d496a 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -652,13 +652,14 @@ func (c *ChannelGraph) UpdateChannelEdge(edge *ChannelEdgeInfo) error { binary.BigEndian.PutUint64(chanKey[:], edge.ChannelID) return c.db.Update(func(tx *bbolt.Tx) error { - edges, err := tx.CreateBucketIfNotExists(edgeBucket) - if err != nil { - return err + edges := tx.Bucket(edgeBucket) + if edge == nil { + return ErrEdgeNotFound } - edgeIndex, err := edges.CreateBucketIfNotExists(edgeIndexBucket) - if err != nil { - return err + + edgeIndex := edges.Bucket(edgeIndexBucket) + if edgeIndex == nil { + return ErrEdgeNotFound } if edgeInfo := edgeIndex.Get(chanKey[:]); edgeInfo == nil { From 351f4aab798aa0398a05740e747d02071bcbe810 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 04/10] channeldb/graph: don't create buckets in DeleteChannelEdge Instead return ErrEdgeNotFound if buckets don't exist. This would be returned anyway, when the chanIndex is checked for the channel point in question. --- channeldb/graph.go | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 6a7d496a..ce98cd76 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -1067,25 +1067,30 @@ func (c *ChannelGraph) DeleteChannelEdge(chanPoint *wire.OutPoint) error { return c.db.Update(func(tx *bbolt.Tx) error { // First grab the edges bucket which houses the information // we'd like to delete - edges, err := tx.CreateBucketIfNotExists(edgeBucket) - if err != nil { - return err + edges := tx.Bucket(edgeBucket) + if edges == nil { + return ErrEdgeNotFound } - // Next grab the two edge indexes which will also need to be updated. - edgeIndex, err := edges.CreateBucketIfNotExists(edgeIndexBucket) - if err != nil { - return err + + // Next grab the two edge indexes which will also need to be + // updated. + edgeIndex := edges.Bucket(edgeIndexBucket) + if edgeIndex == nil { + return ErrEdgeNotFound } - chanIndex, err := edges.CreateBucketIfNotExists(channelPointBucket) - if err != nil { - return err + + chanIndex := edges.Bucket(channelPointBucket) + if chanIndex == nil { + return ErrEdgeNotFound } nodes, err := tx.CreateBucketIfNotExists(nodeBucket) if err != nil { return err } - return delChannelByEdge(edges, edgeIndex, chanIndex, nodes, chanPoint) + return delChannelByEdge( + edges, edgeIndex, chanIndex, nodes, chanPoint, + ) }) } From 9722323161afd25b82eeb30793037f5f6ad4a9cd Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 05/10] channeldb/graph: don't create node bucket in DeleteChannelEdge Instead return ErrGraphNodeNotFound directly. If the node bucket was created it would be empty, and the call delChannelByEdge -> fetchChanEdgePolicies -> fetchChanEdgePolicy -> deserializeChanEdgePolicy -> fetchLightningNode would return this error anyway. --- channeldb/graph.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index ce98cd76..adf15371 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -1083,9 +1083,10 @@ func (c *ChannelGraph) DeleteChannelEdge(chanPoint *wire.OutPoint) error { if chanIndex == nil { return ErrEdgeNotFound } - nodes, err := tx.CreateBucketIfNotExists(nodeBucket) - if err != nil { - return err + + nodes := tx.Bucket(nodeBucket) + if nodes == nil { + return ErrGraphNodeNotFound } return delChannelByEdge( From 84de318553fe884439dce314e1f2a5be964f235b Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 06/10] channeldb/graph: don't create bucket in UpdateEdgePolicy instead return ErEdgeNotFound, which would be returned anyway when querying the edgeIndex for the channel. --- channeldb/graph.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index adf15371..9abf0631 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -1638,13 +1638,14 @@ func delChannelByEdge(edges *bbolt.Bucket, edgeIndex *bbolt.Bucket, // the nodes on either side of the channel. func (c *ChannelGraph) UpdateEdgePolicy(edge *ChannelEdgePolicy) error { return c.db.Update(func(tx *bbolt.Tx) error { - edges, err := tx.CreateBucketIfNotExists(edgeBucket) - if err != nil { - return err + edges := tx.Bucket(edgeBucket) + if edge == nil { + return ErrEdgeNotFound } - edgeIndex, err := edges.CreateBucketIfNotExists(edgeIndexBucket) - if err != nil { - return err + + edgeIndex := edges.Bucket(edgeIndexBucket) + if edgeIndex == nil { + return ErrEdgeNotFound } nodes, err := tx.CreateBucketIfNotExists(nodeBucket) if err != nil { From 86d40636cdd3038a44c4edf9bc6d6e6dc1c22c18 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 07/10] channeldb/graph: don't create bucket in delEdgeUpdateIndexEntry We are deleting what's in the bucket, so if it doesn't exist, just return early with a non-error. --- channeldb/graph.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 9abf0631..99e0dc24 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -1528,11 +1528,10 @@ func delEdgeUpdateIndexEntry(edgesBucket *bbolt.Bucket, chanID uint64, // First, we'll fetch the edge update index bucket which currently // stores an entry for the channel we're about to delete. - updateIndex, err := edgesBucket.CreateBucketIfNotExists( - edgeUpdateIndexBucket, - ) - if err != nil { - return err + updateIndex := edgesBucket.Bucket(edgeUpdateIndexBucket) + if updateIndex == nil { + // No edges in bucket, return early. + return nil } // Now that we have the bucket, we'll attempt to construct a template From 1b9ca0656396ae48038a69ff4693741d77dcbe54 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:38 +0100 Subject: [PATCH 08/10] channeldb/graph: reuse nodes bucket when fetchin source node --- channeldb/graph.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 99e0dc24..ed4d0b0a 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -277,7 +277,14 @@ func (c *ChannelGraph) ForEachNode(tx *bbolt.Tx, cb func(*bbolt.Tx, *LightningNo func (c *ChannelGraph) SourceNode() (*LightningNode, error) { var source *LightningNode err := c.db.View(func(tx *bbolt.Tx) error { - node, err := c.sourceNode(tx) + // First grab the nodes bucket which stores the mapping from + // pubKey to node information. + nodes := tx.Bucket(nodeBucket) + if nodes == nil { + return ErrGraphNotFound + } + + node, err := c.sourceNode(nodes) if err != nil { return err } @@ -296,14 +303,7 @@ func (c *ChannelGraph) SourceNode() (*LightningNode, error) { // of the graph. The source node is treated as the center node within a // star-graph. This method may be used to kick off a path finding algorithm in // order to explore the reachability of another node based off the source node. -func (c *ChannelGraph) sourceNode(tx *bbolt.Tx) (*LightningNode, error) { - // First grab the nodes bucket which stores the mapping from - // pubKey to node information. - nodes := tx.Bucket(nodeBucket) - if nodes == nil { - return nil, ErrGraphNotFound - } - +func (c *ChannelGraph) sourceNode(nodes *bbolt.Bucket) (*LightningNode, error) { selfPub := nodes.Get(sourceKey) if selfPub == nil { return nil, ErrSourceNodeNotSet @@ -823,7 +823,7 @@ func (c *ChannelGraph) pruneGraphNodes(tx *bbolt.Tx, nodes *bbolt.Bucket, // We'll retrieve the graph's source node to ensure we don't remove it // even if it no longer has any open channels. - sourceNode, err := c.sourceNode(tx) + sourceNode, err := c.sourceNode(nodes) if err != nil { return err } From 9d467d3534cc535d9b8c16d04e3c5e87133087c0 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:25:05 +0100 Subject: [PATCH 09/10] channeldb/graph: reuse nodes bucket in deleteLightningNode Lets us avoid passing the tx from pruneGraphNodes --- channeldb/graph.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index ed4d0b0a..22efa93a 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -420,20 +420,22 @@ func (c *ChannelGraph) LookupAlias(pub *btcec.PublicKey) (string, error) { func (c *ChannelGraph) DeleteLightningNode(nodePub *btcec.PublicKey) error { // TODO(roasbeef): ensure dangling edges are removed... return c.db.Update(func(tx *bbolt.Tx) error { - return c.deleteLightningNode(tx, nodePub.SerializeCompressed()) + nodes := tx.Bucket(nodeBucket) + if nodes == nil { + return ErrGraphNodeNotFound + } + + return c.deleteLightningNode( + nodes, nodePub.SerializeCompressed(), + ) }) } // deleteLightningNode uses an existing database transaction to remove a // vertex/node from the database according to the node's public key. -func (c *ChannelGraph) deleteLightningNode(tx *bbolt.Tx, +func (c *ChannelGraph) deleteLightningNode(nodes *bbolt.Bucket, compressedPubKey []byte) error { - nodes := tx.Bucket(nodeBucket) - if nodes == nil { - return ErrGraphNodesNotFound - } - aliases := nodes.Bucket(aliasIndexBucket) if aliases == nil { return ErrGraphNodesNotFound @@ -781,7 +783,7 @@ func (c *ChannelGraph) PruneGraph(spentOutputs []*wire.OutPoint, // Now that the graph has been pruned, we'll also attempt to // prune any nodes that have had a channel closed within the // latest block. - return c.pruneGraphNodes(tx, nodes, edgeIndex) + return c.pruneGraphNodes(nodes, edgeIndex) }) if err != nil { return nil, err @@ -809,14 +811,14 @@ func (c *ChannelGraph) PruneGraphNodes() error { return ErrGraphNoEdgesFound } - return c.pruneGraphNodes(tx, nodes, edgeIndex) + return c.pruneGraphNodes(nodes, edgeIndex) }) } // pruneGraphNodes attempts to remove any nodes from the graph who have had a // channel closed within the current block. If the node still has existing // channels in the graph, this will act as a no-op. -func (c *ChannelGraph) pruneGraphNodes(tx *bbolt.Tx, nodes *bbolt.Bucket, +func (c *ChannelGraph) pruneGraphNodes(nodes *bbolt.Bucket, edgeIndex *bbolt.Bucket) error { log.Trace("Pruning nodes from graph with no open channels") @@ -889,7 +891,7 @@ func (c *ChannelGraph) pruneGraphNodes(tx *bbolt.Tx, nodes *bbolt.Bucket, // If we reach this point, then there are no longer any edges // that connect this node, so we can delete it. - if err := c.deleteLightningNode(tx, nodePubKey[:]); err != nil { + if err := c.deleteLightningNode(nodes, nodePubKey[:]); err != nil { log.Warnf("Unable to prune node %x from the "+ "graph: %v", nodePubKey, err) continue From 70aeda15895811b9c4c1df1741efff51c8127471 Mon Sep 17 00:00:00 2001 From: "Johan T. Halseth" Date: Tue, 4 Dec 2018 11:20:39 +0100 Subject: [PATCH 10/10] channeldb/graph: don't create node bucket in PruneGraph Instead return ErrSourceNodeNotSet directly. This would be returned from the call pruneGraphNodes -> sourceNode anyway. --- channeldb/graph.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/channeldb/graph.go b/channeldb/graph.go index 22efa93a..849afd9f 100644 --- a/channeldb/graph.go +++ b/channeldb/graph.go @@ -710,9 +710,9 @@ func (c *ChannelGraph) PruneGraph(spentOutputs []*wire.OutPoint, if err != nil { return err } - nodes, err := tx.CreateBucketIfNotExists(nodeBucket) - if err != nil { - return err + nodes := tx.Bucket(nodeBucket) + if nodes == nil { + return ErrSourceNodeNotSet } // For each of the outpoints that have been spent within the