Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions channeldb/graph_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -729,6 +729,14 @@ func TestEdgeInfoUpdates(t *testing.T) {

// Create an edge and add it to the db.
edgeInfo, edge1, edge2 := createChannelEdge(db, node1, node2)

// Make sure inserting the policy at this point, before the edge info
// is added, will fail.
if err := graph.UpdateEdgePolicy(edge1); err != ErrEdgeNotFound {
t.Fatalf("expected ErrEdgeNotFound, got: %v", err)
}

// Add the edge info.
if err := graph.AddChannelEdge(edgeInfo); err != nil {
t.Fatalf("unable to create channel edge: %v", err)
}
Expand Down
41 changes: 9 additions & 32 deletions routing/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -1125,8 +1125,6 @@ func (r *ChannelRouter) processUpdate(msg interface{}) error {
}
r.rejectMtx.RUnlock()

channelID := lnwire.NewShortChanIDFromInt(msg.ChannelID)

// We make sure to hold the mutex for this channel ID,
// such that no other goroutine is concurrently doing
// database accesses for the same channel ID.
Expand All @@ -1142,6 +1140,15 @@ func (r *ChannelRouter) processUpdate(msg interface{}) error {

}

// If the channel doesn't exist in our database, we cannot
// apply the updated policy.
if !exists {
return newErrf(ErrIgnored, "Ignoring update "+
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return a slightly different error object than before. Is it okay with the callers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! I looked into this, and the error will eventually be delivered back to the gossiper.

Previously we would log the error and start rejecting this channel:

d.recentRejects[msg.ShortChannelID.ToUint64()] = struct{}{}

While we now will just ignore it, and process it as normal if we later receive it.

Note however, that this is situation most likely will never occur, since the gossiper does its own existence check first:

chanInfo, _, _, err := d.cfg.Router.GetChannelByID(msg.ShortChannelID)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the situation occur if something happens in between the two db queries for the channel?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it does, the error is handled correctly by the caller here:

if err := d.cfg.Router.UpdateEdge(update); err != nil {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear.

The other use of UpdateEdge is in ChannelRouter.applyChannelUpdate. But I think the behaviour improves there too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes looks okay:

if err != nil && !IsError(err, ErrIgnored, ErrOutdated) {

"(flags=%v|%v) for unknown chan_id=%v",
msg.MessageFlags, msg.ChannelFlags,
msg.ChannelID)
}

// As edges are directional edge node has a unique policy for
// the direction of the edge they control. Therefore we first
// check if we already have the most up to date information for
Expand Down Expand Up @@ -1174,36 +1181,6 @@ func (r *ChannelRouter) processUpdate(msg interface{}) error {
}
}

if !exists && !r.cfg.AssumeChannelValid {
// Before we can update the channel information, we'll
// ensure that the target channel is still open by
// querying the utxo-set for its existence.
chanPoint, fundingTxOut, err := r.fetchChanPoint(
&channelID,
)
if err != nil {
r.rejectMtx.Lock()
r.rejectCache[msg.ChannelID] = struct{}{}
r.rejectMtx.Unlock()

return errors.Errorf("unable to fetch chan "+
"point for chan_id=%v: %v",
msg.ChannelID, err)
}
_, err = r.cfg.Chain.GetUtxo(
chanPoint, fundingTxOut.PkScript,
channelID.BlockHeight,
)
if err != nil {
r.rejectMtx.Lock()
r.rejectCache[msg.ChannelID] = struct{}{}
r.rejectMtx.Unlock()

return errors.Errorf("unable to fetch utxo for "+
"chan_id=%v: %v", msg.ChannelID, err)
}
}

// Now that we know this isn't a stale update, we'll apply the
// new edge policy to the proper directional edge within the
// channel graph.
Expand Down
82 changes: 81 additions & 1 deletion routing/router_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
"github.com/btcsuite/btcd/wire"
"github.com/davecgh/go-spew/spew"

"github.com/lightningnetwork/lightning-onion"
sphinx "github.com/lightningnetwork/lightning-onion"
"github.com/lightningnetwork/lnd/channeldb"
"github.com/lightningnetwork/lnd/htlcswitch"
"github.com/lightningnetwork/lnd/lnwire"
Expand Down Expand Up @@ -1025,6 +1025,86 @@ func TestIgnoreNodeAnnouncement(t *testing.T) {
}
}

// TestIgnoreChannelEdgePolicyForUnknownChannel checks that a router will
// ignore a channel policy for a channel not in the graph.
func TestIgnoreChannelEdgePolicyForUnknownChannel(t *testing.T) {
t.Parallel()

const startingBlockHeight = 101

// Setup an initially empty network.
testChannels := []*testChannel{}
testGraph, err := createTestGraphFromChannels(testChannels)
if err != nil {
t.Fatalf("unable to create graph: %v", err)
}
defer testGraph.cleanUp()

ctx, cleanUp, err := createTestCtxFromGraphInstance(
startingBlockHeight, testGraph,
)
if err != nil {
t.Fatalf("unable to create router: %v", err)
}
defer cleanUp()

var pub1 [33]byte
copy(pub1[:], priv1.PubKey().SerializeCompressed())

var pub2 [33]byte
copy(pub2[:], priv2.PubKey().SerializeCompressed())

// Add the edge between the two unknown nodes to the graph, and check
// that the nodes are found after the fact.
fundingTx, _, chanID, err := createChannelEdge(
ctx, bitcoinKey1.SerializeCompressed(),
bitcoinKey2.SerializeCompressed(), 10000, 500,
)
if err != nil {
t.Fatalf("unable to create channel edge: %v", err)
}
fundingBlock := &wire.MsgBlock{
Transactions: []*wire.MsgTx{fundingTx},
}
ctx.chain.addBlock(fundingBlock, chanID.BlockHeight, chanID.BlockHeight)

edge := &channeldb.ChannelEdgeInfo{
ChannelID: chanID.ToUint64(),
NodeKey1Bytes: pub1,
NodeKey2Bytes: pub2,
BitcoinKey1Bytes: pub1,
BitcoinKey2Bytes: pub2,
AuthProof: nil,
}
edgePolicy := &channeldb.ChannelEdgePolicy{
SigBytes: testSig.Serialize(),
ChannelID: edge.ChannelID,
LastUpdate: testTime,
TimeLockDelta: 10,
MinHTLC: 1,
FeeBaseMSat: 10,
FeeProportionalMillionths: 10000,
}

// Attempt to update the edge. This should be ignored, since the edge
// is not yet added to the router.
err = ctx.router.UpdateEdge(edgePolicy)
if !IsError(err, ErrIgnored) {
t.Fatalf("expected to get ErrIgnore, instead got: %v", err)
}

// Add the edge.
if err := ctx.router.AddEdge(edge); err != nil {
t.Fatalf("expected to be able to add edge to the channel graph,"+
" even though the vertexes were unknown: %v.", err)
}

// Now updating the edge policy should succeed.
if err := ctx.router.UpdateEdge(edgePolicy); err != nil {
t.Fatalf("unable to update edge policy: %v", err)
}
}

// TestAddEdgeUnknownVertexes tests that if an edge is added that contains two
// vertexes which we don't know of, the edge should be available for use
// regardless. This is due to the fact that we don't actually need node
Expand Down