lnwire+features: transition to the user friendly list of features

features.go

@@ -2,12 +2,10 @@ package main
 
 import "github.com/lightningnetwork/lnd/lnwire"
 
-// globalFeaturesMap is a map which binds the name of the global feature with it
+// globalFeatures feature vector which affects HTLCs and thus are also
-// index. The index is just an order of the feature and the final binary
+// advertised to other nodes.
-// representation of feature vector is determined by decode function.
+var globalFeatures = lnwire.NewFeatureVector([]lnwire.Feature{})
-var globalFeaturesMap = lnwire.FeaturesMap{}
 
-// localFeaturesMap is a map which binds the name of the local feature with it
+// localFeatures is an feature vector which represent the features which
-// index. The index is just an order of the feature and the final binary
+// only affect the protocol between these two nodes.
-// representation of feature vector is determined by decode function.
+var localFeatures = lnwire.NewFeatureVector([]lnwire.Feature{})
-var localFeaturesMap = lnwire.FeaturesMap{}

lnwire/features.go

@@ -27,17 +27,6 @@ func (f featureFlag) String() string {
 // compile errors if we specify wrong feature name.
 type featureName string
 
-// FeaturesMap is the map which stores the correspondence between feature name
-// and its index within feature vector.
-//
-// NOTE: Index within feature vector and actual binary position of feature
-// are different things)
-type FeaturesMap map[featureName]int
-
-// indexToFlag is the map which stores the correspondence between feature
-// position and its flag.
-type indexToFlag map[int]featureFlag
-
 const (
 	// OptionalFlag represent the feature which we already have but it isn't
 	// required yet, and if remote peer doesn't have this feature we may
@@ -70,25 +59,47 @@ const (
 	maxAllowedSize = 32781
 )
 
+// Feature represent the feature which is used on stage of initialization of
+// feature vector. Initial feature flags might be changed dynamically later.
+type Feature struct {
+	Name featureName
+	Flag featureFlag
+}
+
 // FeatureVector represents the global/local feature vector. With this structure
 // you may set/get the feature by name and compare feature vector with remote
 // one.
 type FeatureVector struct {
-	features FeaturesMap // name -> index
+	// featuresMap is the map which stores the correspondence between
-	flags    indexToFlag // index -> flag
+	// feature name and its index within feature vector. Index within
+	// feature vector and actual binary position of feature
+	// are different things)
+	featuresMap map[featureName]int // name -> index
+
+	// flags is the map which stores the correspondence between feature
+	// index and its flag.
+	flags map[int]featureFlag // index -> flag
 }
 
 // NewFeatureVector creates new instance of feature vector.
-func NewFeatureVector(features FeaturesMap) *FeatureVector {
+func NewFeatureVector(features []Feature) *FeatureVector {
+	featuresMap := make(map[featureName]int)
+	flags := make(map[int]featureFlag)
+
+	for index, feature := range features {
+		featuresMap[feature.Name] = index
+		flags[index] = feature.Flag
+	}
+
 	return &FeatureVector{
-		features: features,
+		featuresMap: featuresMap,
-		flags:    make(indexToFlag),
+		flags:       flags,
 	}
 }
 
 // SetFeatureFlag assign flag to the feature.
 func (f *FeatureVector) SetFeatureFlag(name featureName, flag featureFlag) error {
-	position, ok := f.features[name]
+	position, ok := f.featuresMap[name]
 	if !ok {
 		return errors.Errorf("can't find feature with name: %v", name)
 	}
@@ -97,16 +108,16 @@ func (f *FeatureVector) SetFeatureFlag(name featureName, flag featureFlag) error
 	return nil
 }
 
-// SerializedSize returns the number of bytes which is needed to represent feature
+// serializedSize returns the number of bytes which is needed to represent
-// vector in byte format.
+// feature vector in byte format.
-func (f *FeatureVector) SerializedSize() uint16 {
+func (f *FeatureVector) serializedSize() uint16 {
 	return uint16(math.Ceil(float64(flagBitsSize*len(f.flags)) / 8))
 }
 
 // String returns the feature vector description.
 func (f *FeatureVector) String() string {
 	var description string
-	for name, index := range f.features {
+	for name, index := range f.featuresMap {
 		if flag, ok := f.flags[index]; ok {
 			description += fmt.Sprintf("%s: %s\n", name, flag)
 		}
@@ -128,7 +139,7 @@ func (f *FeatureVector) String() string {
 // later become compulsory.
 func NewFeatureVectorFromReader(r io.Reader) (*FeatureVector, error) {
 	f := &FeatureVector{
-		flags: make(indexToFlag),
+		flags: make(map[int]featureFlag),
 	}
 
 	getFlag := func(data []byte, position int) featureFlag {
@@ -186,7 +197,7 @@ func (f *FeatureVector) Encode(w io.Writer) error {
 
 	// Write length of feature vector.
 	var l [2]byte
-	length := f.SerializedSize()
+	length := f.serializedSize()
 	binary.BigEndian.PutUint16(l[:], length)
 	if _, err := w.Write(l[:]); err != nil {
 		return err
@@ -214,7 +225,7 @@ func (f *FeatureVector) Encode(w io.Writer) error {
 // are incompatible.
 func (local *FeatureVector) Compare(remote *FeatureVector) (*SharedFeatures,
 	error) {
-	shared := NewSharedFeatures(local.features)
+	shared := newSharedFeatures(local.Copy())
 
 	for index, flag := range local.flags {
 		if _, exist := remote.flags[index]; !exist {
@@ -225,6 +236,7 @@ func (local *FeatureVector) Compare(remote *FeatureVector) (*SharedFeatures,
 			case OptionalFlag:
 				// If feature is optional and remote side
 				// haven't it than it might be safely disabled.
+				delete(shared.flags, index)
 				continue
 			}
 		}
@@ -243,6 +255,7 @@ func (local *FeatureVector) Compare(remote *FeatureVector) (*SharedFeatures,
 			case OptionalFlag:
 				// If feature is optional and local side
 				// haven't it than it might be safely disabled.
+				delete(shared.flags, index)
 				continue
 			}
 		}
@@ -255,6 +268,20 @@ func (local *FeatureVector) Compare(remote *FeatureVector) (*SharedFeatures,
 	return shared, nil
 }
 
+// Copy generate new distinct instance of the feature vector.
+func (f *FeatureVector) Copy() *FeatureVector {
+	features := make([]Feature, len(f.featuresMap))
+
+	for name, index := range f.featuresMap {
+		features[index] = Feature{
+			Name: name,
+			Flag: f.flags[index],
+		}
+	}
+
+	return NewFeatureVector(features)
+}
+
 // SharedFeatures is a product of comparison of two features vector
 // which consist of features which are present in both local and remote
 // features vectors.
@@ -262,22 +289,22 @@ type SharedFeatures struct {
 	*FeatureVector
 }
 
-// NewSharedFeatures creates new shared features instance.
+// newSharedFeatures creates new shared features instance.
-func NewSharedFeatures(features FeaturesMap) *SharedFeatures {
+func newSharedFeatures(f *FeatureVector) *SharedFeatures {
-	return &SharedFeatures{NewFeatureVector(features)}
+	return &SharedFeatures{f}
 }
 
 // IsActive checks is feature active or not, it might be disabled during
 // comparision with remote feature vector if it was optional and
 // remote peer doesn't support it.
 func (f *SharedFeatures) IsActive(name featureName) bool {
-	position, ok := f.features[name]
+	index, ok := f.featuresMap[name]
 	if !ok {
 		// If we even have no such feature in feature map, than it
 		// can't be active in any circumstances.
 		return false
 	}
 
-	_, exist := f.flags[position]
+	_, exist := f.flags[index]
 	return exist
 }

lnwire/features_test.go

@@ -9,26 +9,19 @@ import (
 // TestFeaturesRemoteRequireError checks that we throw an error if remote peer
 // has required feature which we don't support.
 func TestFeaturesRemoteRequireError(t *testing.T) {
-	var (
+	const (
-		first featureName = "first"
+		first  = "first"
-		second featureName = "second"
+		second = "second"
 	)
 
-	var localFeaturesMap = FeaturesMap{
+	localFeatures := NewFeatureVector([]Feature{
-		first: 0,
+		{first, OptionalFlag},
-	}
+	})
 
-	var remoteFeaturesMap = FeaturesMap{
+	remoteFeatures := NewFeatureVector([]Feature{
-		first:  0,
+		{first,  OptionalFlag},
-		second: 1,
+		{second, RequiredFlag},
-	}
+	})
-
-	localFeatures := NewFeatureVector(localFeaturesMap)
-	localFeatures.SetFeatureFlag(first, OptionalFlag)
-
-	remoteFeatures := NewFeatureVector(remoteFeaturesMap)
-	remoteFeatures.SetFeatureFlag(first, RequiredFlag)
-	remoteFeatures.SetFeatureFlag(second, RequiredFlag)
 
 	if _, err := localFeatures.Compare(remoteFeatures); err == nil {
 		t.Fatal("error wasn't received")
@@ -38,26 +31,19 @@ func TestFeaturesRemoteRequireError(t *testing.T) {
 // TestFeaturesLocalRequireError checks that we throw an error if local peer has
 // required feature which remote peer don't support.
 func TestFeaturesLocalRequireError(t *testing.T) {
-	var (
+	const (
-		first featureName = "first"
+		first  = "first"
-		second featureName = "second"
+		second = "second"
 	)
 
-	var localFeaturesMap = FeaturesMap{
+	localFeatures := NewFeatureVector([]Feature{
-		first:  0,
+		{first,  OptionalFlag},
-		second: 1,
+		{second, RequiredFlag},
-	}
+	})
 
-	var remoteFeaturesMap = FeaturesMap{
+	remoteFeatures := NewFeatureVector([]Feature{
-		first: 0,
+		{first, OptionalFlag},
-	}
+	})
-
-	localFeatures := NewFeatureVector(localFeaturesMap)
-	localFeatures.SetFeatureFlag(first, OptionalFlag)
-	localFeatures.SetFeatureFlag(second, RequiredFlag)
-
-	remoteFeatures := NewFeatureVector(remoteFeaturesMap)
-	remoteFeatures.SetFeatureFlag(first, RequiredFlag)
 
 	if _, err := localFeatures.Compare(remoteFeatures); err == nil {
 		t.Fatal("error wasn't received")
@@ -67,16 +53,13 @@ func TestFeaturesLocalRequireError(t *testing.T) {
 // TestOptionalFeature checks that if remote peer don't have the feature but
 // on our side this feature is optional than we mark this feature as disabled.
 func TestOptionalFeature(t *testing.T) {
-	var first featureName = "first"
+	const first = "first"
 
-	var localFeaturesMap = FeaturesMap{
+	localFeatures := NewFeatureVector([]Feature{
-		first: 0,
+		{first, OptionalFlag},
-	}
+	})
 
-	localFeatures := NewFeatureVector(localFeaturesMap)
+	remoteFeatures := NewFeatureVector([]Feature{})
-	localFeatures.SetFeatureFlag(first, OptionalFlag)
-
-	remoteFeatures := NewFeatureVector(FeaturesMap{})
 
 	shared, err := localFeatures.Compare(remoteFeatures)
 	if err != nil {
@@ -89,17 +72,32 @@ func TestOptionalFeature(t *testing.T) {
 	}
 }
 
+// TestSetRequireAfterInit checks that we can change the feature flag after
+// initialization.
+func TestSetRequireAfterInit(t *testing.T) {
+	const first = "first"
+
+	localFeatures := NewFeatureVector([]Feature{
+		{first, OptionalFlag},
+	})
+	localFeatures.SetFeatureFlag(first, RequiredFlag)
+	remoteFeatures := NewFeatureVector([]Feature{})
+
+	_, err := localFeatures.Compare(remoteFeatures)
+	if err == nil {
+		t.Fatalf("feature was set as required but error wasn't "+
+			"returned: %v", err)
+	}
+}
+
 // TestDecodeEncodeFeaturesVector checks that feature vector might be
 // successfully encoded and decoded.
 func TestDecodeEncodeFeaturesVector(t *testing.T) {
-	var first featureName = "first"
+	const first = "first"
 
-	var localFeaturesMap = FeaturesMap{
+	f := NewFeatureVector([]Feature{
-		first: 0,
+		{first, OptionalFlag},
-	}
+	})
-
-	f := NewFeatureVector(localFeaturesMap)
-	f.SetFeatureFlag(first, OptionalFlag)
 
 	var b bytes.Buffer
 	if err := f.Encode(&b); err != nil {

lnwire/init_message.go

@@ -86,12 +86,12 @@ func (msg *Init) MaxPayloadLength(uint32) uint32 {
 //
 // This is part of the lnwire.Message interface.
 func (msg *Init) Validate() error {
-	if msg.GlobalFeatures.SerializedSize() > maxAllowedSize {
+	if msg.GlobalFeatures.serializedSize() > maxAllowedSize {
 		return errors.Errorf("global feature vector exceed max allowed "+
 			"size %v", maxAllowedSize)
 	}
 
-	if msg.LocalFeatures.SerializedSize() > maxAllowedSize {
+	if msg.LocalFeatures.serializedSize() > maxAllowedSize {
 		return errors.Errorf("local feature vector exceed max allowed "+
 			"size %v", maxAllowedSize)
 	}

lnwire/init_test.go

@@ -7,15 +7,14 @@ import (
 )
 
 func TestInitEncodeDecode(t *testing.T) {
-	fm := FeaturesMap{
+	const somefeature = "somefeature"
-		"somefeature": 0,
-	}
 
-	gf := NewFeatureVector(fm)
+	gf := NewFeatureVector([]Feature{
-	gf.SetFeatureFlag("somefeature", OptionalFlag)
+		{somefeature, OptionalFlag},
-
+	})
-	lf := NewFeatureVector(fm)
+	lf := NewFeatureVector([]Feature{
-	lf.SetFeatureFlag("somefeature", OptionalFlag)
+		{somefeature, OptionalFlag},
+	})
 
 	init1 := &Init{
 		GlobalFeatures: gf,
@@ -35,10 +34,10 @@ func TestInitEncodeDecode(t *testing.T) {
 	}
 
 	// We not encode the feature map in feature vector, for that reason the
-	// init messages will differ. Initialize decoded feature map in
+	// init messages will differ. Set feature map with nil in
 	// order to use deep equal function.
-	init2.GlobalFeatures.features = fm
+	init1.GlobalFeatures.featuresMap = nil
-	init2.LocalFeatures.features = fm
+	init1.LocalFeatures.featuresMap = nil
 
 	// Assert equality of the two instances.
 	if !reflect.DeepEqual(init1, init2) {

peer.go

@@ -199,8 +199,8 @@ func newPeer(conn net.Conn, server *server, addr *lnwire.NetAddress,
 		localCloseChanReqs:  make(chan *closeLinkReq),
 		remoteCloseChanReqs: make(chan *lnwire.CloseRequest),
 
-		localSharedFeatures:  lnwire.NewSharedFeatures(localFeaturesMap),
+		localSharedFeatures:  nil,
-		globalSharedFeatures: lnwire.NewSharedFeatures(globalFeaturesMap),
+		globalSharedFeatures: nil,
 
 		queueQuit: make(chan struct{}),
 		quit:      make(chan struct{}),

server.go

@@ -137,8 +137,8 @@ func newServer(listenAddrs []string, notifier chainntnfs.ChainNotifier,
 		broadcastRequests: make(chan *broadcastReq),
 		sendRequests:      make(chan *sendReq),
 
-		globalFeatures: lnwire.NewFeatureVector(globalFeaturesMap),
+		globalFeatures: globalFeatures,
-		localFeatures: lnwire.NewFeatureVector(localFeaturesMap),
+		localFeatures: localFeatures,
 
 		queries: make(chan interface{}),
 		quit:    make(chan struct{}),