diff --git a/feature/manager.go b/feature/manager.go index f788f9892..eaaa0294c 100644 --- a/feature/manager.go +++ b/feature/manager.go @@ -221,7 +221,9 @@ func (m *Manager) UpdateFeatureSets( return err } - if err := m.Get(set).ValidateUpdate(newFeatures); err != nil { + if err := m.Get(set).ValidateUpdate( + newFeatures, set.Maximum(), + ); err != nil { return err } diff --git a/feature/set.go b/feature/set.go index c70637031..9be2c4315 100644 --- a/feature/set.go +++ b/feature/set.go @@ -1,5 +1,11 @@ package feature +import ( + "math" + + "github.com/lightningnetwork/lnd/lnwire" +) + // Set is an enum identifying various feature sets, which separates the single // feature namespace into distinct categories depending what context a feature // vector is being used. @@ -56,3 +62,22 @@ func (s Set) String() string { return "SetUnknown" } } + +// Maximum returns the maximum allowable value for a feature bit in the context +// of a set. The maximum feature value we can express differs by set context +// because the amount of space available varies between protocol messages. In +// practice this should never be a problem (reasonably one would never hit +// these high ranges), but we enforce these maximums for the sake of sane +// validation. +func (s Set) Maximum() lnwire.FeatureBit { + switch s { + case SetInvoice, SetInvoiceAmp: + return lnwire.MaxBolt11Feature + + // The space available in other sets is > math.MaxUint16, so we just + // return the maximum value our expression of a feature bit allows so + // that any value will pass. + default: + return math.MaxUint16 + } +} diff --git a/lnwire/features.go b/lnwire/features.go index 21b7fbc02..dd0abc392 100644 --- a/lnwire/features.go +++ b/lnwire/features.go @@ -16,6 +16,10 @@ var ( // set of features are made. ErrFeatureStandard = errors.New("feature is used in standard " + "protocol set") + + // ErrFeatureBitMaximum is returned when a feature bit exceeds the + // maximum allowable value. + ErrFeatureBitMaximum = errors.New("feature bit exceeds allowed maximum") ) // FeatureBit represents a feature that can be enabled in either a local or @@ -227,17 +231,18 @@ const ( // TODO: Decide on actual feature bit value. ScriptEnforcedLeaseOptional FeatureBit = 2023 - // maxAllowedSize is a maximum allowed size of feature vector. + // MaxBolt11Feature is the maximum feature bit value allowed in bolt 11 + // invoices. // - // NOTE: Within the protocol, the maximum allowed message size is 65535 - // bytes for all messages. Accounting for the overhead within the feature - // message to signal the type of message, that leaves us with 65533 bytes - // for the init message itself. Next, we reserve 4 bytes to encode the - // lengths of both the local and global feature vectors, so 65529 bytes - // for the local and global features. Knocking off one byte for the sake - // of the calculation, that leads us to 32764 bytes for each feature - // vector, or 131056 different features. - maxAllowedSize = 32764 + // The base 32 encoded tagged fields in invoices are limited to 10 bits + // to express the length of the field's data. + //nolint:lll + // See: https://github.com/lightning/bolts/blob/master/11-payment-encoding.md#tagged-fields + // + // With a maximum length field of 1023 (2^10 -1) and 5 bit encoding, + // the highest feature bit that can be expressed is: + // 1023 * 5 - 1 = 5114. + MaxBolt11Feature = 5114 ) // IsRequired returns true if the feature bit is even, and false otherwise. @@ -354,8 +359,12 @@ func (fv *RawFeatureVector) Merge(other *RawFeatureVector) error { // new feature vector provided, checking that it does not alter any of the // "standard" features that are defined by LND. The new feature vector should // be inclusive of all features in the original vector that it still wants to -// advertise, setting and unsetting updates as desired. -func (fv *RawFeatureVector) ValidateUpdate(other *RawFeatureVector) error { +// advertise, setting and unsetting updates as desired. Features in the vector +// are also checked against a maximum inclusive value, as feature vectors in +// different contexts have different maximum values. +func (fv *RawFeatureVector) ValidateUpdate(other *RawFeatureVector, + maximumValue FeatureBit) error { + // Run through the new set of features and check that we're not adding // any feature bits that are defined but not set in LND. for feature := range other.features { @@ -363,6 +372,12 @@ func (fv *RawFeatureVector) ValidateUpdate(other *RawFeatureVector) error { continue } + if feature > maximumValue { + return fmt.Errorf("can't set feature bit %d: %w %v", + feature, ErrFeatureBitMaximum, + maximumValue) + } + if name, known := Features[feature]; known { return fmt.Errorf("can't set feature "+ "bit %d (%v): %w", feature, name, diff --git a/lnwire/features_test.go b/lnwire/features_test.go index 84053b963..c77ddd4d1 100644 --- a/lnwire/features_test.go +++ b/lnwire/features_test.go @@ -2,6 +2,7 @@ package lnwire import ( "bytes" + "math" "reflect" "sort" "testing" @@ -423,6 +424,7 @@ func TestValidateUpdate(t *testing.T) { name string currentFeatures []FeatureBit newFeatures []FeatureBit + maximumValue FeatureBit err error }{ { @@ -479,6 +481,30 @@ func TestValidateUpdate(t *testing.T) { }, err: nil, }, + { + name: "at allowed maximum", + currentFeatures: []FeatureBit{ + StaticRemoteKeyOptional, + }, + newFeatures: []FeatureBit{ + StaticRemoteKeyOptional, + 100, + }, + maximumValue: 100, + err: nil, + }, + { + name: "above allowed maximum", + currentFeatures: []FeatureBit{ + StaticRemoteKeyOptional, + }, + newFeatures: []FeatureBit{ + StaticRemoteKeyOptional, + 101, + }, + maximumValue: 100, + err: ErrFeatureBitMaximum, + }, } for _, testCase := range testCases { @@ -491,7 +517,13 @@ func TestValidateUpdate(t *testing.T) { ) newFV := NewRawFeatureVector(testCase.newFeatures...) - err := currentFV.ValidateUpdate(newFV) + // Set maximum value if not populated in the test case. + maximumValue := testCase.maximumValue + if testCase.maximumValue == 0 { + maximumValue = math.MaxUint16 + } + + err := currentFV.ValidateUpdate(newFV, maximumValue) require.ErrorIs(t, err, testCase.err) }) }