discovery+lnd: make param ProofMatureDelta configurable#9405
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
bb8b92f to
abeda3b
Compare
yyforyongyu
added
code health
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Do you know why we had this Delta in the first place ?
yyforyongyu
changed the title
ProofMatureDeltaProofMatureDelta configurable
cb3b77a to
c1bda26
Compare
|
@yyforyongyu, remember to re-request review from reviewers when ready |
|
|
||
| * [Make](https://github.com/lightningnetwork/lnd/pull/9405) the param | ||
| `ProofMatureDelta` used in gossip to be configurable via | ||
| `--gossip.announcement-conf`, with a default value of 5. |
c1bda26 to
f6547b3
Compare
| 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 |
There was a problem hiding this comment.
Nit: Separate release notes in own commit
| } | ||
|
|
||
| // Validate checks the Gossip configuration to ensure that the input values are | ||
| // sane. |
There was a problem hiding this comment.
Can you add a rationale why not less than 3 ?
| return s.genNodeAnnouncement(nil) | ||
| }, | ||
| ProofMatureDelta: 0, | ||
| ProofMatureDelta: cfg.Gossip.AnnouncementConf, |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
I don't think we ever block announcements - we cache them when they haven't reached 6 confs.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
3309469 to
863f29a
Compare
We add a new config option to set the `ProofMatureDelta` so the users can tune their graphs based on their own preference over the num of confs found in the announcement signatures.
863f29a to
ae2bcfe
Compare
5 participants
We add a new config option to set the
ProofMatureDeltaso the userscan tune their graphs based on their own preference over the num of
confs found in the announcement signatures.