Avoid generating redundant claims after initial confirmation #1753
Conversation
Codecov ReportBase: 90.72% // Head: 90.70% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1753 +/- ##
==========================================
- Coverage 90.72% 90.70% -0.02%
==========================================
Files 87 87
Lines 47361 47356 -5
Branches 47361 47356 -5
==========================================
- Hits 42968 42956 -12
- Misses 4393 4400 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
wpaulino
force-pushed
the
avoid-redundant-claims-after-initial-conf
branch
from
4ce1645 to
5b0ba3a
Compare
|
Feel free to squash. |
wpaulino
force-pushed
the
avoid-redundant-claims-after-initial-conf
branch
2 times, most recently
from
6c864b8 to
4e20072
Compare
|
Squashed and rebased on latest. |
lightning/src/chain/onchaintx.rs
Outdated
| .any(|event_entry| if let OnchainEvent::Claim { claim_request } = event_entry.event { | ||
| for outpoint in &request_outpoints { | ||
| if let Some(first_claim_txid_height) = self.claimable_outpoints.get(outpoint) { | ||
| if first_claim_txid_height.0 != claim_request { |
There was a problem hiding this comment.
Hmm, is this right? It seems what this is asking is "is there a single confirming claim package that spent all the outpoints of the one being generated", but instead I think we may want to answer "is there at least a single confirming claim package which spent the outpoint in question". This would imply an outer for outpoint in &request_outpoints { loop, with a claimable_outpoints.get lookup inside it.
There was a problem hiding this comment.
I believe we still want the former. The goal here is to prevent the same claims from being generated when nothing has materially changed after an initial confirmation.
There was a problem hiding this comment.
But won't this break if we, for example, split a package because one input has to be spent soon, but then the previous version of the package confirms?
There was a problem hiding this comment.
Ah, good point! Fixed.
These claims will never be valid as a previous claim has already confirmed. If a previous claim is reorged out of the chain, a new claim will be generated bypassing the new behavior. While this doesn't change much for our existing transaction-based claims, as broadcasting an already confirmed transaction acts as a NOP, it prevents us from yielding redundant event-based claims, which will be introduced as part of the anchors patchset.
wpaulino
force-pushed
the
avoid-redundant-claims-after-initial-conf
branch
from
4e20072 to
a089136
Compare
| // transaction is reorged out. | ||
| let mut all_inputs_have_confirmed_spend = true; | ||
| for outpoint in &request_outpoints { | ||
| if let Some(first_claim_txid_height) = self.claimable_outpoints.get(outpoint) { |
There was a problem hiding this comment.
btw, this variable name is a bit confusing, because it sounds like it's the height of the txid, but it's actually the (txid, height) tuple
There was a problem hiding this comment.
that said, I don't have any better ideas ¯_(ツ)_/¯
There was a problem hiding this comment.
Its copied from another of other places, so if we want to change it ISTM we should do it broadly through the file and in a followup.
These claims will never be valid as a previous claim has already confirmed. If a previous claim is reorged out of the chain, a new claim will be generated bypassing the new behavior.
While this doesn't change much for our existing transaction-based claims, as broadcasting an already confirmed transaction acts as a NOP, it prevents us from yielding redundant event-based claims, which will be introduced as part of the anchors patchset.