Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

discovery+lnd: make param ProofMatureDelta configurable #9405

Merged
merged 2 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,7 @@ func DefaultConfig() Config {
MaxChannelUpdateBurst: discovery.DefaultMaxChannelUpdateBurst,
ChannelUpdateInterval: discovery.DefaultChannelUpdateInterval,
SubBatchDelay: discovery.DefaultSubBatchDelay,
AnnouncementConf: discovery.DefaultProofMatureDelta,
},
Invoices: &lncfg.Invoices{
HoldExpiryDelta: lncfg.DefaultHoldInvoiceExpiryDelta,
Expand Down Expand Up @@ -1754,6 +1755,7 @@ func ValidateConfig(cfg Config, interceptor signal.Interceptor, fileParser,
cfg.Invoices,
cfg.Routing,
cfg.Pprof,
cfg.Gossip,
)
if err != nil {
return nil, err
Expand Down
15 changes: 13 additions & 2 deletions discovery/gossiper.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ const (
// we'll maintain. This is the global size across all peers. We'll
// allocate ~3 MB max to the cache.
maxRejectedUpdates = 10_000

// DefaultProofMatureDelta specifies the default value used for
// ProofMatureDelta, which is the number of confirmations needed before
// processing the announcement signatures.
DefaultProofMatureDelta = 6
)

var (
Expand Down Expand Up @@ -1984,8 +1989,14 @@ func (d *AuthenticatedGossiper) addNode(msg *lnwire.NodeAnnouncement,
// NOTE: must be used inside a lock.
func (d *AuthenticatedGossiper) isPremature(chanID lnwire.ShortChannelID,
delta uint32, msg *networkMsg) bool {
// TODO(roasbeef) make height delta 6
// * or configurable

// The channel is already confirmed at chanID.BlockHeight so we minus
// one block. For instance, if the required confirmation for this
// channel announcement is 6, we then only need to wait for 5 more
// blocks once the funding tx is confirmed.
if delta > 0 {
delta--
}

msgHeight := chanID.BlockHeight + delta

Expand Down
5 changes: 5 additions & 0 deletions docs/release-notes/release-notes-0.19.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@
range TLVs provided with the existing set of records on the HTLC,
overwriting any conflicting values with those supplied by the API.

* [Make](https://github.com/lightningnetwork/lnd/pull/9405) the param
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Separate release notes in own commit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

`ProofMatureDelta` used in gossip to be configurable via
`--gossip.announcement-conf`, with a default value of 6.

## lncli Updates

## Code Health
Expand Down Expand Up @@ -214,6 +218,7 @@ config option](https://github.com/lightningnetwork/lnd/pull/9182) and introduce
a new option `channel-max-fee-exposure` which is unambiguous in its description.
The underlying functionality between those two options remain the same.


## Breaking Changes
## Performance Improvements

Expand Down
28 changes: 28 additions & 0 deletions lncfg/gossip.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,24 @@
package lncfg

import (
"fmt"
"time"

"github.com/lightningnetwork/lnd/discovery"
"github.com/lightningnetwork/lnd/routing/route"
)

// minAnnouncementConf defines the minimal num of confs needed for the config
// AnnouncementConf. We choose 3 here as it's unlikely a reorg depth of 3 would
// happen.
//
// NOTE: The specs recommends setting this value to 6, which is the default
// value used for AnnouncementConf. However the receiver should be able to
// decide which channels to be included in its local graph, more details can be
// found:
// - https://github.com/lightning/bolts/pull/1215#issuecomment-2557337202
const minAnnouncementConf = 3

//nolint:ll
type Gossip struct {
PinnedSyncersRaw []string `long:"pinned-syncers" description:"A set of peers that should always remain in an active sync state, which can be used to closely synchronize the routing tables of two nodes. The value should be a hex-encoded pubkey, the flag can be specified multiple times to add multiple peers. Connected peers matching this pubkey will remain active for the duration of the connection and not count towards the NumActiveSyncer count."`
Expand All @@ -18,6 +30,8 @@ type Gossip struct {
ChannelUpdateInterval time.Duration `long:"channel-update-interval" description:"The interval used to determine how often lnd should allow a burst of new updates for a specific channel and direction."`

SubBatchDelay time.Duration `long:"sub-batch-delay" description:"The duration to wait before sending the next announcement batch if there are multiple. Use a small value if there are a lot announcements and they need to be broadcast quickly."`

AnnouncementConf uint32 `long:"announcement-conf" description:"The number of confirmations required before processing channel announcements."`
}

// Parse the pubkeys for the pinned syncers.
Expand All @@ -35,3 +49,17 @@ func (g *Gossip) Parse() error {

return nil
}

// Validate checks the Gossip configuration to ensure that the input values are
// sane.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a rationale why not less than 3 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

func (g *Gossip) Validate() error {
if g.AnnouncementConf < minAnnouncementConf {
return fmt.Errorf("announcement-conf=%v must be no less than "+
"%v", g.AnnouncementConf, minAnnouncementConf)
}

return nil
}

// Compile-time constraint to ensure Gossip implements the Validator interface.
var _ Validator = (*Gossip)(nil)
2 changes: 2 additions & 0 deletions sample-lnd.conf
Original file line number Diff line number Diff line change
Expand Up @@ -1729,6 +1729,8 @@
; be broadcast quickly.
; gossip.sub-batch-delay=5s

; The number of confirmations required before processing channel announcements.
; gossip.announcement-conf=6

[invoices]

Expand Down
2 changes: 1 addition & 1 deletion server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1118,7 +1118,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr,

return s.genNodeAnnouncement(nil)
},
ProofMatureDelta: 0,
ProofMatureDelta: cfg.Gossip.AnnouncementConf,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I am not sure if we should block our own local anounccemnts, this should be regared as an error because why are we sending them in the first place to the gossiper ? So for the remote it makes sense but for the local I think we should just log something but not block it, wdyt ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we ever block announcements - we cache them when they haven't reached 6 confs.

Copy link
Collaborator

@ziggie1984 ziggie1984 Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

talking about this case here:

	needBlockHeight := ann.ShortChannelID.BlockHeight +
		d.cfg.ProofMatureDelta
	shortChanID := ann.ShortChannelID.ToUint64()

	prefix := "local"
	if nMsg.isRemote {
		prefix = "remote"
	}

	log.Infof("Received new %v announcement signature for %v", prefix,
		ann.ShortChannelID)

	// By the specification, channel announcement proofs should be sent
	// after some number of confirmations after channel was registered in
	// bitcoin blockchain. Therefore, we check if the proof is mature.
	d.Lock()
	premature := d.isPremature(
		ann.ShortChannelID, d.cfg.ProofMatureDelta, nMsg,
	)
	if premature {
		log.Warnf("Premature proof announcement, current block height"+
			"lower than needed: %v < %v", d.bestHeight,
			needBlockHeight)
		d.Unlock()
		nMsg.err <- nil
		return nil, false
	}
	d.Unlock()

I wonder why we do check for the local announcement here and return an error, I would expect that our own software produces announments which we want to skip this check. Basically it seems weird to catch the own channel_announcments here. We kinda don't trust our own sourced announcements ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that our own software produces announments which we want to skip this check.

yeah I think this makes sense, as we skip the maturity check elsewhere when it's from local. Also correct that the funding manager should never attempt to send this msg unless it has 6 confs. The only possible case when we hit an error is when the two subsystems are not in sync with block heights, which means they should also be block consumers.

The current behavior will cache the early msg and reprocess it when it reaches 6 conf, tho I think it's better if we just skip checking the maturity when it's from the local, so maybe a follow up PR?

TrickleDelay: time.Millisecond * time.Duration(cfg.TrickleDelay),
RetransmitTicker: ticker.New(time.Minute * 30),
RebroadcastInterval: time.Hour * 24,
Expand Down
Loading