From ae15a193e2e1af046ac6c6935bff00f3bcc3819e Mon Sep 17 00:00:00 2001 From: Andrey Samokhvalov Date: Fri, 17 Feb 2017 17:28:11 +0300 Subject: [PATCH] lnwire+features: transition to the user friendly list of features --- features.go | 14 +++--- lnwire/features.go | 85 +++++++++++++++++++++++------------- lnwire/features_test.go | 96 ++++++++++++++++++++--------------------- lnwire/init_message.go | 4 +- lnwire/init_test.go | 21 +++++---- peer.go | 4 +- server.go | 4 +- 7 files changed, 125 insertions(+), 103 deletions(-) diff --git a/features.go b/features.go index 43b253c2..bdc1eee8 100644 --- a/features.go +++ b/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 -// index. The index is just an order of the feature and the final binary -// representation of feature vector is determined by decode function. -var globalFeaturesMap = lnwire.FeaturesMap{} +// globalFeatures feature vector which affects HTLCs and thus are also +// advertised to other nodes. +var globalFeatures = lnwire.NewFeatureVector([]lnwire.Feature{}) -// localFeaturesMap is a map which binds the name of the local feature with it -// index. The index is just an order of the feature and the final binary -// representation of feature vector is determined by decode function. -var localFeaturesMap = lnwire.FeaturesMap{} +// localFeatures is an feature vector which represent the features which +// only affect the protocol between these two nodes. +var localFeatures = lnwire.NewFeatureVector([]lnwire.Feature{}) diff --git a/lnwire/features.go b/lnwire/features.go index e53d50ab..fbaa72ec 100644 --- a/lnwire/features.go +++ b/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 - flags indexToFlag // index -> flag + // featuresMap is the map which stores the correspondence between + // 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, - flags: make(indexToFlag), + featuresMap: featuresMap, + 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 -// vector in byte format. -func (f *FeatureVector) SerializedSize() uint16 { +// serializedSize returns the number of bytes which is needed to represent +// feature vector in byte format. +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. -func NewSharedFeatures(features FeaturesMap) *SharedFeatures { - return &SharedFeatures{NewFeatureVector(features)} +// newSharedFeatures creates new shared features instance. +func newSharedFeatures(f *FeatureVector) *SharedFeatures { + 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 } diff --git a/lnwire/features_test.go b/lnwire/features_test.go index 86a9eb83..b51e3da4 100644 --- a/lnwire/features_test.go +++ b/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 ( - first featureName = "first" - second featureName = "second" + const ( + first = "first" + second = "second" ) - var localFeaturesMap = FeaturesMap{ - first: 0, - } + localFeatures := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + }) - var remoteFeaturesMap = FeaturesMap{ - first: 0, - second: 1, - } - - localFeatures := NewFeatureVector(localFeaturesMap) - localFeatures.SetFeatureFlag(first, OptionalFlag) - - remoteFeatures := NewFeatureVector(remoteFeaturesMap) - remoteFeatures.SetFeatureFlag(first, RequiredFlag) - remoteFeatures.SetFeatureFlag(second, RequiredFlag) + remoteFeatures := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + {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 ( - first featureName = "first" - second featureName = "second" + const ( + first = "first" + second = "second" ) - var localFeaturesMap = FeaturesMap{ - first: 0, - second: 1, - } + localFeatures := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + {second, RequiredFlag}, + }) - var remoteFeaturesMap = FeaturesMap{ - first: 0, - } - - localFeatures := NewFeatureVector(localFeaturesMap) - localFeatures.SetFeatureFlag(first, OptionalFlag) - localFeatures.SetFeatureFlag(second, RequiredFlag) - - remoteFeatures := NewFeatureVector(remoteFeaturesMap) - remoteFeatures.SetFeatureFlag(first, RequiredFlag) + remoteFeatures := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + }) 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{ - first: 0, - } + localFeatures := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + }) - localFeatures := NewFeatureVector(localFeaturesMap) - localFeatures.SetFeatureFlag(first, OptionalFlag) - - remoteFeatures := NewFeatureVector(FeaturesMap{}) + remoteFeatures := NewFeatureVector([]Feature{}) 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{ - first: 0, - } - - f := NewFeatureVector(localFeaturesMap) - f.SetFeatureFlag(first, OptionalFlag) + f := NewFeatureVector([]Feature{ + {first, OptionalFlag}, + }) var b bytes.Buffer if err := f.Encode(&b); err != nil { diff --git a/lnwire/init_message.go b/lnwire/init_message.go index 1801229c..bdb34874 100644 --- a/lnwire/init_message.go +++ b/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) } diff --git a/lnwire/init_test.go b/lnwire/init_test.go index cf2b519f..23b3fbc6 100644 --- a/lnwire/init_test.go +++ b/lnwire/init_test.go @@ -7,15 +7,14 @@ import ( ) func TestInitEncodeDecode(t *testing.T) { - fm := FeaturesMap{ - "somefeature": 0, - } + const somefeature = "somefeature" - gf := NewFeatureVector(fm) - gf.SetFeatureFlag("somefeature", OptionalFlag) - - lf := NewFeatureVector(fm) - lf.SetFeatureFlag("somefeature", OptionalFlag) + gf := NewFeatureVector([]Feature{ + {somefeature, OptionalFlag}, + }) + lf := NewFeatureVector([]Feature{ + {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 - init2.LocalFeatures.features = fm + init1.GlobalFeatures.featuresMap = nil + init1.LocalFeatures.featuresMap = nil // Assert equality of the two instances. if !reflect.DeepEqual(init1, init2) { diff --git a/peer.go b/peer.go index e79a00c5..270aec32 100644 --- a/peer.go +++ b/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), - globalSharedFeatures: lnwire.NewSharedFeatures(globalFeaturesMap), + localSharedFeatures: nil, + globalSharedFeatures: nil, queueQuit: make(chan struct{}), quit: make(chan struct{}), diff --git a/server.go b/server.go index be5fb565..dabaa370 100644 --- a/server.go +++ b/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), - localFeatures: lnwire.NewFeatureVector(localFeaturesMap), + globalFeatures: globalFeatures, + localFeatures: localFeatures, queries: make(chan interface{}), quit: make(chan struct{}),