From fc9c6c426e08181d0a6140d67da45343f0049968 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 4 Aug 2022 06:59:21 +0800 Subject: [PATCH] lntemp+itest: refactor `testUpdateNodeAnnouncement` --- lntemp/rpc/peers.go | 34 +++ lntest/itest/assertions.go | 25 --- lntest/itest/list_on_test.go | 4 + lntest/itest/lnd_channel_graph_test.go | 293 ++++++++++--------------- lntest/itest/lnd_test_list_on_test.go | 4 - 5 files changed, 158 insertions(+), 202 deletions(-) diff --git a/lntemp/rpc/peers.go b/lntemp/rpc/peers.go index 2c1578dc2..ed4c0761a 100644 --- a/lntemp/rpc/peers.go +++ b/lntemp/rpc/peers.go @@ -1,5 +1,39 @@ package rpc +import ( + "context" + + "github.com/lightningnetwork/lnd/lnrpc/peersrpc" + "github.com/stretchr/testify/require" +) + // ===================== // PeerClient related RPCs. // ===================== + +type ( + AnnReq *peersrpc.NodeAnnouncementUpdateRequest + AnnResp *peersrpc.NodeAnnouncementUpdateResponse +) + +// UpdateNodeAnnouncement makes an UpdateNodeAnnouncement RPC call the the +// peersrpc client and asserts. +func (h *HarnessRPC) UpdateNodeAnnouncement(req AnnReq) AnnResp { + ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout) + defer cancel() + + resp, err := h.Peer.UpdateNodeAnnouncement(ctxt, req) + require.NoErrorf(h, err, "failed to update announcement") + + return resp +} + +// UpdateNodeAnnouncementErr makes an UpdateNodeAnnouncement RPC call the the +// peersrpc client and asserts an error is returned. +func (h *HarnessRPC) UpdateNodeAnnouncementErr(req AnnReq) { + ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout) + defer cancel() + + _, err := h.Peer.UpdateNodeAnnouncement(ctxt, req) + require.Error(h, err, "expect an error from update announcement") +} diff --git a/lntest/itest/assertions.go b/lntest/itest/assertions.go index 9cbffd8b4..239123d55 100644 --- a/lntest/itest/assertions.go +++ b/lntest/itest/assertions.go @@ -16,7 +16,6 @@ import ( "github.com/go-errors/errors" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/lnrpc" - "github.com/lightningnetwork/lnd/lnrpc/peersrpc" "github.com/lightningnetwork/lnd/lnrpc/routerrpc" "github.com/lightningnetwork/lnd/lnrpc/walletrpc" "github.com/lightningnetwork/lnd/lntest" @@ -1564,27 +1563,3 @@ func assertNodeAnnouncement(t *harnessTest, n1, n2 *lnrpc.NodeUpdate) { } } } - -// assertUpdateNodeAnnouncementResponse is a helper function to assert -// the response expected values. -func assertUpdateNodeAnnouncementResponse(t *harnessTest, - response *peersrpc.NodeAnnouncementUpdateResponse, - expectedOps map[string]int) { - - require.Equal( - t.t, len(response.Ops), len(expectedOps), - "unexpected number of Ops updating dave's node announcement", - ) - - ops := make(map[string]int, len(response.Ops)) - for _, op := range response.Ops { - ops[op.Entity] = len(op.Actions) - } - - for k, v := range expectedOps { - if v != ops[k] { - t.Fatalf("unexpected number of actions for operation "+ - "%s: got %d wanted %d", k, ops[k], v) - } - } -} diff --git a/lntest/itest/list_on_test.go b/lntest/itest/list_on_test.go index 9f98018ab..865e959ca 100644 --- a/lntest/itest/list_on_test.go +++ b/lntest/itest/list_on_test.go @@ -175,4 +175,8 @@ var allTestCasesTemp = []*lntemp.TestCase{ Name: "node announcement", TestFunc: testNodeAnnouncement, }, + { + Name: "update node announcement rpc", + TestFunc: testUpdateNodeAnnouncement, + }, } diff --git a/lntest/itest/lnd_channel_graph_test.go b/lntest/itest/lnd_channel_graph_test.go index fa27a4f77..e9e7ae648 100644 --- a/lntest/itest/lnd_channel_graph_test.go +++ b/lntest/itest/lnd_channel_graph_test.go @@ -1,12 +1,9 @@ package itest import ( - "context" "fmt" - "io" "strings" "testing" - "time" "github.com/btcsuite/btcd/btcutil" "github.com/lightningnetwork/lnd/chainreg" @@ -16,7 +13,6 @@ import ( "github.com/lightningnetwork/lnd/lnrpc/routerrpc" "github.com/lightningnetwork/lnd/lntemp" "github.com/lightningnetwork/lnd/lntemp/node" - "github.com/lightningnetwork/lnd/lntest" "github.com/lightningnetwork/lnd/lntest/wait" "github.com/stretchr/testify/require" ) @@ -422,110 +418,11 @@ func testNodeAnnouncement(ht *lntemp.HarnessTest) { ht.CloseChannel(bob, chanPoint) } -// graphSubscription houses the proxied update and error chans for a node's -// graph subscriptions. -type graphSubscription struct { - updateChan chan *lnrpc.GraphTopologyUpdate - errChan chan error - quit chan struct{} -} - -// subscribeGraphNotifications subscribes to channel graph updates and launches -// a goroutine that forwards these to the returned channel. -func subscribeGraphNotifications(ctxb context.Context, t *harnessTest, - node *lntest.HarnessNode) graphSubscription { - - // We'll first start by establishing a notification client which will - // send us notifications upon detected changes in the channel graph. - req := &lnrpc.GraphTopologySubscription{} - ctx, cancelFunc := context.WithCancel(ctxb) - topologyClient, err := node.SubscribeChannelGraph(ctx, req) - require.NoError(t.t, err, "unable to create topology client") - - // We'll launch a goroutine that will be responsible for proxying all - // notifications recv'd from the client into the channel below. - errChan := make(chan error, 1) - quit := make(chan struct{}) - graphUpdates := make(chan *lnrpc.GraphTopologyUpdate, 20) - go func() { - for { - defer cancelFunc() - - select { - case <-quit: - return - default: - graphUpdate, err := topologyClient.Recv() - select { - case <-quit: - return - default: - } - - if err == io.EOF { - return - } else if err != nil { - select { - case errChan <- err: - case <-quit: - } - return - } - - select { - case graphUpdates <- graphUpdate: - case <-quit: - return - } - } - } - }() - - return graphSubscription{ - updateChan: graphUpdates, - errChan: errChan, - quit: quit, - } -} - -// waitForNodeAnnUpdates monitors the nodeAnnUpdates until we get one for -// the expected node and asserts that has the expected information. -func waitForNodeAnnUpdates(graphSub graphSubscription, nodePubKey string, - expectedUpdate *lnrpc.NodeUpdate, t *harnessTest) { - - for { - select { - case graphUpdate := <-graphSub.updateChan: - for _, update := range graphUpdate.NodeUpdates { - if update.IdentityKey == nodePubKey { - assertNodeAnnouncement( - t, update, expectedUpdate, - ) - return - } - } - case err := <-graphSub.errChan: - t.Fatalf("unable to recv graph update: %v", err) - case <-time.After(defaultTimeout): - t.Fatalf("did not receive node ann update") - } - } -} - // testUpdateNodeAnnouncement ensures that the RPC endpoint validates // the requests correctly and that the new node announcement is brodcasted // with the right information after updating our node. -func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { - // context timeout for the whole test. - ctxt, cancel := context.WithTimeout( - context.Background(), defaultTimeout, - ) - defer cancel() - - // Launch notification clients for alice, such that we can - // get notified when there are updates in the graph. - aliceSub := subscribeGraphNotifications(ctxt, t, net.Alice) - defer close(aliceSub.quit) +func testUpdateNodeAnnouncement(ht *lntemp.HarnessTest) { + alice, bob := ht.Alice, ht.Bob var lndArgs []string @@ -534,20 +431,42 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { "192.168.1.1:8333", "[2001:db8:85a3:8d3:1319:8a2e:370:7348]:8337", "bkb6azqggsaiskzi.onion:9735", - "fomvuglh6h6vcag73xo5t5gv56ombih3zr2xvplkpbfd7wrog4swjwid.onion:1234", + "fomvuglh6h6vcag73xo5t5gv56ombih3zr2xvplkpbfd7wrog4swj" + + "wid.onion:1234", } for _, addr := range extraAddrs { lndArgs = append(lndArgs, "--externalip="+addr) } - dave := net.NewNode(t.t, "Dave", lndArgs) - defer shutdownAndAssert(net, t, dave) + dave := ht.NewNode("Dave", lndArgs) - // Get dave default information so we can compare - // it lately with the brodcasted updates. - nodeInfoReq := &lnrpc.GetInfoRequest{} - resp, err := dave.GetInfo(ctxt, nodeInfoReq) - require.NoError(t.t, err, "unable to get dave's information") + // assertNodeAnn is a helper closure that checks a given node update + // from Dave is seen by Alice. + assertNodeAnn := func(expected *lnrpc.NodeUpdate) { + err := wait.NoError(func() error { + // Get a list of node updates seen by Alice. + updates := alice.Watcher.GetNodeUpdates(dave.PubKeyStr) + // Check at least one of the updates matches the given + // node update. + for _, update := range updates { + err := compareNodeAnns(update, expected) + // Found a match, return nil. + if err == nil { + return nil + } + } + + // We've check all the updates and no match found. + return fmt.Errorf("alice didn't see the update: %v", + expected) + }, defaultTimeout) + + require.NoError(ht, err, "assertNodeAnn failed") + } + + // Get dave default information so we can compare it lately with the + // brodcasted updates. + resp := dave.RPC.GetInfo() defaultAddrs := make([]*lnrpc.NodeAddress, 0, len(resp.Uris)) for _, uri := range resp.GetUris() { values := strings.Split(uri, "@") @@ -564,9 +483,8 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { // update this one for another one unset by default at random. featureBit := lnrpc.FeatureBit_WUMBO_CHANNELS_REQ featureIdx := uint32(featureBit) - if _, ok := resp.Features[featureIdx]; ok { - t.Fatalf("unexpected feature bit enabled by default") - } + _, ok := resp.Features[featureIdx] + require.False(ht, ok, "unexpected feature bit enabled by default") defaultDaveNodeAnn := &lnrpc.NodeUpdate{ Alias: resp.Alias, @@ -576,44 +494,33 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { // Dave must have an open channel before he can send a node // announcement, so we open a channel with Bob. - net.ConnectNodes(t.t, net.Bob, dave) + ht.ConnectNodes(bob, dave) // Go ahead and open a channel between Bob and Dave. This // ensures that Alice receives the node announcement from Bob as part of // the announcement broadcast. - chanPoint := openChannelAndAssert( - t, net, net.Bob, dave, - lntest.OpenChannelParams{ + chanPoint := ht.OpenChannel( + bob, dave, lntemp.OpenChannelParams{ Amt: 1000000, }, ) - require.NoError(t.t, err, "unexpected error opening a channel") // Wait for Alice to receive dave's node announcement with the default // values. - waitForNodeAnnUpdates( - aliceSub, dave.PubKeyStr, defaultDaveNodeAnn, t, - ) + assertNodeAnn(defaultDaveNodeAnn) - // We cannot differentiate between requests with Alias = "" and requests - // that do not provide that field. If a user sets Alias = "" in the request - // the field will simply be ignored. The request must fail because no - // modifiers are applied. - invalidNodeAnnReq := &peersrpc.NodeAnnouncementUpdateRequest{ - Alias: "", - } - - _, err = dave.UpdateNodeAnnouncement(ctxt, invalidNodeAnnReq) - require.Error(t.t, err, "requests without modifiers should field") + // We cannot differentiate between requests with Alias = "" and + // requests that do not provide that field. If a user sets Alias = "" + // in the request the field will simply be ignored. The request must + // fail because no modifiers are applied. + invalidNodeAnnReq := &peersrpc.NodeAnnouncementUpdateRequest{Alias: ""} + dave.RPC.UpdateNodeAnnouncementErr(invalidNodeAnnReq) // Alias too long. invalidNodeAnnReq = &peersrpc.NodeAnnouncementUpdateRequest{ Alias: strings.Repeat("a", 50), } - - _, err = dave.UpdateNodeAnnouncement(ctxt, invalidNodeAnnReq) - require.Error(t.t, err, "failed to validate an invalid alias for an "+ - "update node announcement request") + dave.RPC.UpdateNodeAnnouncementErr(invalidNodeAnnReq) // Update Node. newAlias := "new-alias" @@ -653,8 +560,7 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { FeatureUpdates: updateFeatureActions, } - response, err := dave.UpdateNodeAnnouncement(ctxt, nodeAnnReq) - require.NoError(t.t, err, "unable to update dave's node announcement") + response := dave.RPC.UpdateNodeAnnouncement(nodeAnnReq) expectedOps := map[string]int{ "features": 1, @@ -662,7 +568,7 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { "alias": 1, "addresses": 3, } - assertUpdateNodeAnnouncementResponse(t, response, expectedOps) + assertUpdateNodeAnnouncementResponse(ht, response, expectedOps) newNodeAddresses := []*lnrpc.NodeAddress{} // We removed the first address. @@ -683,28 +589,18 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { // We'll then wait for Alice to receive dave's node announcement // with the new values. - waitForNodeAnnUpdates( - aliceSub, dave.PubKeyStr, newDaveNodeAnn, t, - ) + assertNodeAnn(newDaveNodeAnn) // Check that the feature bit was set correctly. - resp, err = dave.GetInfo(ctxt, nodeInfoReq) - require.NoError(t.t, err, "unable to get dave's information") - - if _, ok := resp.Features[featureIdx]; !ok { - t.Fatalf("failed to set feature bit") - } + resp = dave.RPC.GetInfo() + _, ok = resp.Features[featureIdx] + require.True(ht, ok, "failed to set feature bit") // Check that we cannot set a feature bit that is already set. nodeAnnReq = &peersrpc.NodeAnnouncementUpdateRequest{ FeatureUpdates: updateFeatureActions, } - - _, err = dave.UpdateNodeAnnouncement(ctxt, nodeAnnReq) - require.Error( - t.t, err, "missing expected error: cannot set a feature bit "+ - "that is already set", - ) + dave.RPC.UpdateNodeAnnouncementErr(nodeAnnReq) // Check that we can unset feature bits. updateFeatureActions = []*peersrpc.UpdateFeatureAction{ @@ -717,35 +613,25 @@ func testUpdateNodeAnnouncement(net *lntest.NetworkHarness, t *harnessTest) { nodeAnnReq = &peersrpc.NodeAnnouncementUpdateRequest{ FeatureUpdates: updateFeatureActions, } - - response, err = dave.UpdateNodeAnnouncement(ctxt, nodeAnnReq) - require.NoError(t.t, err, "unable to update dave's node announcement") + response = dave.RPC.UpdateNodeAnnouncement(nodeAnnReq) expectedOps = map[string]int{ "features": 1, } - assertUpdateNodeAnnouncementResponse(t, response, expectedOps) + assertUpdateNodeAnnouncementResponse(ht, response, expectedOps) - resp, err = dave.GetInfo(ctxt, nodeInfoReq) - require.NoError(t.t, err, "unable to get dave's information") - - if _, ok := resp.Features[featureIdx]; ok { - t.Fatalf("failed to unset feature bit") - } + resp = dave.RPC.GetInfo() + _, ok = resp.Features[featureIdx] + require.False(ht, ok, "failed to unset feature bit") // Check that we cannot unset a feature bit that is already unset. nodeAnnReq = &peersrpc.NodeAnnouncementUpdateRequest{ FeatureUpdates: updateFeatureActions, } - - _, err = dave.UpdateNodeAnnouncement(ctxt, nodeAnnReq) - require.Error( - t.t, err, "missing expected error: cannot unset a feature bit "+ - "that is already unset", - ) + dave.RPC.UpdateNodeAnnouncementErr(nodeAnnReq) // Close the channel between Bob and Dave. - closeChannelAndAssert(t, net, net.Bob, chanPoint, false) + ht.CloseChannel(bob, chanPoint) } // assertSyncType asserts that the peer has an expected syncType. @@ -767,3 +653,64 @@ func assertSyncType(ht *lntemp.HarnessTest, hn *node.HarnessNode, ht.Fatalf("unable to find peer: %s", peer) } + +// compareNodeAnns compares that two node announcements match or returns an +// error. +// +// NOTE: only used for tests in this file. +func compareNodeAnns(n1, n2 *lnrpc.NodeUpdate) error { + // Alias should match. + if n1.Alias != n2.Alias { + return fmt.Errorf("alias not match") + } + + // Color should match. + if n1.Color != n2.Color { + return fmt.Errorf("color not match") + } + + // NodeAddresses should match. + if len(n1.NodeAddresses) != len(n2.NodeAddresses) { + return fmt.Errorf("node addresses don't match") + } + + addrs := make(map[string]struct{}, len(n1.NodeAddresses)) + for _, nodeAddr := range n1.NodeAddresses { + addrs[nodeAddr.Addr] = struct{}{} + } + + for _, nodeAddr := range n2.NodeAddresses { + if _, ok := addrs[nodeAddr.Addr]; !ok { + return fmt.Errorf("address %v not found in node "+ + "announcement", nodeAddr.Addr) + } + } + + return nil +} + +// assertUpdateNodeAnnouncementResponse is a helper function to assert +// the response expected values. +// +// NOTE: only used for tests in this file. +func assertUpdateNodeAnnouncementResponse(ht *lntemp.HarnessTest, + response *peersrpc.NodeAnnouncementUpdateResponse, + expectedOps map[string]int) { + + require.Equal( + ht, len(response.Ops), len(expectedOps), + "unexpected number of Ops updating dave's node announcement", + ) + + ops := make(map[string]int, len(response.Ops)) + for _, op := range response.Ops { + ops[op.Entity] = len(op.Actions) + } + + for k, v := range expectedOps { + if v != ops[k] { + ht.Fatalf("unexpected number of actions for operation "+ + "%s: got %d wanted %d", k, ops[k], v) + } + } +} diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go index ea51f9630..e90923273 100644 --- a/lntest/itest/lnd_test_list_on_test.go +++ b/lntest/itest/lnd_test_list_on_test.go @@ -24,10 +24,6 @@ var allTestCases = []*testCase{ name: "single hop invoice", test: testSingleHopInvoice, }, - { - name: "test update node announcement rpc", - test: testUpdateNodeAnnouncement, - }, { name: "list outgoing payments", test: testListPayments,