Merge pull request #10003 from ellemouton/fixPeerBootstrappingFlake

discovery: deterministic bootstrapping for local test networks

Author: ellemouton

chainreg/chainregistry.go

@@ -528,7 +528,7 @@ func NewPartialChainControl(cfg *Config) (*PartialChainControl, func(), error) {
 			// On local test networks we usually don't have multiple
 			// chain backend peers, so we can skip
 			// the checkOutboundPeers test.
-			if cfg.Bitcoin.SimNet || cfg.Bitcoin.RegTest {
+			if cfg.Bitcoin.IsLocalNetwork() {
 				return nil
 			}
 
@@ -651,7 +651,7 @@ func NewPartialChainControl(cfg *Config) (*PartialChainControl, func(), error) {
 			// On local test networks we usually don't have multiple
 			// chain backend peers, so we can skip
 			// the checkOutboundPeers test.
-			if cfg.Bitcoin.SimNet || cfg.Bitcoin.RegTest {
+			if cfg.Bitcoin.IsLocalNetwork() {
 				return nil
 			}
 

discovery/bootstrapper.go

@@ -18,6 +18,7 @@ import (
 	"github.com/lightningnetwork/lnd/autopilot"
 	"github.com/lightningnetwork/lnd/lnutils"
 	"github.com/lightningnetwork/lnd/lnwire"
+	"github.com/lightningnetwork/lnd/routing/route"
 	"github.com/lightningnetwork/lnd/tor"
 	"github.com/miekg/dns"
 )
@@ -121,11 +122,10 @@ func shuffleBootstrappers(candidates []NetworkPeerBootstrapper) []NetworkPeerBoo
 type ChannelGraphBootstrapper struct {
 	chanGraph autopilot.ChannelGraph
 
-	// hashAccumulator is a set of 32 random bytes that are read upon the
-	// creation of the channel graph bootstrapper. We use this value to
-	// randomly select nodes within the known graph to connect to. After
-	// each selection, we rotate the accumulator by hashing it with itself.
-	hashAccumulator [32]byte
+	// hashAccumulator is used to determine which nodes to use for
+	// bootstrapping. It allows us to potentially introduce some randomness
+	// into the selection process.
+	hashAccumulator hashAccumulator
 
 	tried map[autopilot.NodeID]struct{}
 }
@@ -138,18 +138,33 @@ var _ NetworkPeerBootstrapper = (*ChannelGraphBootstrapper)(nil)
 // backed by an active autopilot.ChannelGraph instance. This type of network
 // peer bootstrapper will use the authenticated nodes within the known channel
 // graph to bootstrap connections.
-func NewGraphBootstrapper(cg autopilot.ChannelGraph) (NetworkPeerBootstrapper, error) {
+func NewGraphBootstrapper(cg autopilot.ChannelGraph,
+	deterministicSampling bool) (NetworkPeerBootstrapper, error) {
 
-	c := &ChannelGraphBootstrapper{
-		chanGraph: cg,
-		tried:     make(map[autopilot.NodeID]struct{}),
-	}
-
-	if _, err := rand.Read(c.hashAccumulator[:]); err != nil {
-		return nil, err
+	var (
+		hashAccumulator hashAccumulator
+		err             error
+	)
+	if deterministicSampling {
+		// If we're using deterministic sampling, then we'll use a
+		// no-op hash accumulator that will always return false for
+		// skipNode.
+		hashAccumulator = newNoOpHashAccumulator()
+	} else {
+		// Otherwise, we'll use a random hash accumulator to sample
+		// nodes from the channel graph.
+		hashAccumulator, err = newRandomHashAccumulator()
+		if err != nil {
+			return nil, fmt.Errorf("unable to create hash "+
+				"accumulator: %w", err)
+		}
 	}
 
-	return c, nil
+	return &ChannelGraphBootstrapper{
+		chanGraph:       cg,
+		tried:           make(map[autopilot.NodeID]struct{}),
+		hashAccumulator: hashAccumulator,
+	}, nil
 }
 
 // SampleNodeAddrs uniformly samples a set of specified address from the
@@ -199,7 +214,7 @@ func (c *ChannelGraphBootstrapper) SampleNodeAddrs(_ context.Context,
 			// it's 50/50. If it isn't less, than then we'll
 			// continue forward.
 			nodePubKeyBytes := node.PubKey()
-			if bytes.Compare(c.hashAccumulator[:], nodePubKeyBytes[1:]) > 0 {
+			if c.hashAccumulator.skipNode(nodePubKeyBytes) {
 				return nil
 			}
 
@@ -259,7 +274,7 @@ func (c *ChannelGraphBootstrapper) SampleNodeAddrs(_ context.Context,
 		tries++
 
 		// We'll now rotate our hash accumulator one value forwards.
-		c.hashAccumulator = sha256.Sum256(c.hashAccumulator[:])
+		c.hashAccumulator.rotate()
 
 		// If this attempt didn't yield any addresses, then we'll exit
 		// early.
@@ -546,3 +561,83 @@ search:
 func (d *DNSSeedBootstrapper) Name() string {
 	return fmt.Sprintf("BOLT-0010 DNS Seed: %v", d.dnsSeeds)
 }
+
+// hashAccumulator is an interface that defines the methods required for
+// a hash accumulator used to sample nodes from the channel graph.
+type hashAccumulator interface {
+	// rotate rotates the hash accumulator value.
+	rotate()
+
+	// skipNode returns true if the node with the given public key
+	// should be skipped based on the current hash accumulator state.
+	skipNode(pubKey route.Vertex) bool
+}
+
+// randomHashAccumulator is an implementation of the hashAccumulator
+// interface that uses a random hash to sample nodes from the channel graph.
+type randomHashAccumulator struct {
+	hash [32]byte
+}
+
+// A compile time assertion to ensure that randomHashAccumulator meets the
+// hashAccumulator interface.
+var _ hashAccumulator = (*randomHashAccumulator)(nil)
+
+// newRandomHashAccumulator returns a new instance of a randomHashAccumulator.
+// This accumulator is used to randomly sample nodes from the channel graph.
+func newRandomHashAccumulator() (*randomHashAccumulator, error) {
+	var r randomHashAccumulator
+
+	if _, err := rand.Read(r.hash[:]); err != nil {
+		return nil, fmt.Errorf("unable to read random bytes: %w", err)
+	}
+
+	return &r, nil
+}
+
+// rotate rotates the hash accumulator by hashing the current value
+// with itself. This ensures that we have a new random value to compare
+// against when we sample nodes from the channel graph.
+//
+// NOTE: this is part of the hashAccumulator interface.
+func (r *randomHashAccumulator) rotate() {
+	r.hash = sha256.Sum256(r.hash[:])
+}
+
+// skipNode returns true if the node with the given public key should be skipped
+// based on the current hash accumulator state. It will return false for the
+// pub key if it is lexicographically less than our current accumulator value.
+// It does so by comparing the current hash accumulator value with the passed
+// byte slice. When comparing, we skip the first byte as it's 50/50 between 02
+// and 03 for compressed pub keys.
+//
+// NOTE: this is part of the hashAccumulator interface.
+func (r *randomHashAccumulator) skipNode(pub route.Vertex) bool {
+	return bytes.Compare(r.hash[:], pub[1:]) > 0
+}
+
+// noOpHashAccumulator is a no-op implementation of the hashAccumulator
+// interface. This is used when we want deterministic behavior and don't
+// want to sample nodes randomly from the channel graph.
+type noOpHashAccumulator struct{}
+
+// newNoOpHashAccumulator returns a new instance of a noOpHashAccumulator.
+func newNoOpHashAccumulator() *noOpHashAccumulator {
+	return &noOpHashAccumulator{}
+}
+
+// rotate is a no-op for the noOpHashAccumulator.
+//
+// NOTE: this is part of the hashAccumulator interface.
+func (*noOpHashAccumulator) rotate() {}
+
+// skipNode always returns false, meaning that no nodes will be skipped.
+//
+// NOTE: this is part of the hashAccumulator interface.
+func (*noOpHashAccumulator) skipNode(route.Vertex) bool {
+	return false
+}
+
+// A compile-time assertion to ensure that noOpHashAccumulator meets the
+// hashAccumulator interface.
+var _ hashAccumulator = (*noOpHashAccumulator)(nil)

docs/release-notes/release-notes-0.20.0.md

@@ -144,7 +144,9 @@ reader of a payment request.
   disabling has now been 
   [removed](https://github.com/lightningnetwork/lnd/pull/9967) meaning that any 
   test network scripts that rely on bootstrapping being disabled will need to 
-  explicitly define the `--nobootstrap` flag.
+  explicitly define the `--nobootstrap` flag. Bootstrapping will now also be
+  [deterministic](https://github.com/lightningnetwork/lnd/pull/10003) on local 
+  test networks so that bootstrapping behaviour can be tested for.
 
 ## Database
 

lncfg/chain.go

@@ -52,3 +52,9 @@ func (c *Chain) Validate(minTimeLockDelta uint32, minDelay uint16) error {
 
 	return nil
 }
+
+// IsLocalNetwork returns true if the chain is a local network, such as
+// simnet or regtest.
+func (c *Chain) IsLocalNetwork() bool {
+	return c.SimNet || c.RegTest
+}

server.go

@@ -3064,15 +3064,19 @@ func initNetworkBootstrappers(s *server) ([]discovery.NetworkPeerBootstrapper, e
 	// this can be used by default if we've already partially seeded the
 	// network.
 	chanGraph := autopilot.ChannelGraphFromDatabase(s.graphDB)
-	graphBootstrapper, err := discovery.NewGraphBootstrapper(chanGraph)
+	graphBootstrapper, err := discovery.NewGraphBootstrapper(
+		chanGraph, s.cfg.Bitcoin.IsLocalNetwork(),
+	)
 	if err != nil {
 		return nil, err
 	}
 	bootStrappers = append(bootStrappers, graphBootstrapper)
 
-	// If this isn't simnet mode, then one of our additional bootstrapping
-	// sources will be the set of running DNS seeds.
-	if !s.cfg.Bitcoin.SimNet {
+	// If this isn't using simnet or regtest mode, then one of our
+	// additional bootstrapping sources will be the set of running DNS
+	// seeds.
+	if !s.cfg.Bitcoin.IsLocalNetwork() {
+		//nolint:ll
 		dnsSeeds, ok := chainreg.ChainDNSSeeds[*s.cfg.ActiveNetParams.GenesisHash]
 
 		// If we have a set of DNS seeds for this chain, then we'll add