Uh oh!
There was an error while loading. Please reload this page.
Avoid storing a full FinalOnionHopData in OnionPayload::Invoice - #1447
Conversation
Codecov Report
@@ Coverage Diff @@## main #1447 +/- ##
==========================================
- Coverage 90.90% 90.89% -0.01%
==========================================
Files 75 75 Lines 41616 41624 +8 Branches 41616 41624 +8 ==========================================
+ Hits 37829 37834 +5 - Misses 3787 3790 +3
Continue to review full report at Codecov.
|
| if total_value >= msgs::MAX_VALUE_MSAT || total_value > claimable_htlc.total_msat { | ||
| log_trace!(self.logger, "Failing HTLCs with payment_hash {} as the total value {} ran over expected value {} (or HTLCs were inconsistent)", | ||
| log_bytes!(payment_hash.0), total_value, $payment_data_total_msat); | ||
| fail_htlc!(claimable_htlc); |
There was a problem hiding this comment.
Gah, this is wrong and our test coverage is really lacking here. I'm working on some improved testing here but in the mean time the fail_htlc call has to stay and the htlcs.push calls need to go back in the branches instead of being unconditional.
There was a problem hiding this comment.
Do you intend to resolve it in this PR before merging it?
valentinewallace
left a comment
There was a problem hiding this comment.
Looks pretty good! Do we wanna rebase it on #1451 to get the test coverage there?
| value: u64, | ||
| onion_payload: OnionPayload, | ||
| timer_ticks: u8, | ||
| total_msat: u64, |
There was a problem hiding this comment.
Could be nice to document the difference between value and total_msat
There was a problem hiding this comment.
Agreed; in fact, would be necessary imo
There was a problem hiding this comment.
Is this close to accurate?
/// HTLCs that are to us and can be failed/claimed by the user
struct ClaimableHTLC {
prev_hop: HTLCPreviousHopData,
cltv_expiry: u32,
/// The amount (in msats) of this MPP part
value: u64,
onion_payload: OnionPayload,
timer_ticks: u8,
/// The sum total of all MPP parts
total_msat: u64,
}
It was not super clear to me, but I am going to walk through the code more and try to figure it out.
There was a problem hiding this comment.
That looks good to me!
andozw
commented
Apr 27, 2022
I'll give it a go |
b4ba546 to
d0cf6b8Compare| } | ||
| } | ||
| if total_value >= msgs::MAX_VALUE_MSAT || total_value > $payment_data_total_msat { | ||
| if total_value >= msgs::MAX_VALUE_MSAT || total_value > claimable_htlc.total_msat { |
There was a problem hiding this comment.
@TheBlueMatt - should this be || total_value > $payment_data.total_msat ? The tests pass either way. I haven't rebased against #1451 yet but it looks like that one might be ready to merge?
There was a problem hiding this comment.
Yea, its definitely more readable that way. That said I believe they're the same, though its a good bit of careful reading to see that, and I'm not even 100% confident of that after reading it five times.
d0cf6b8 to
4da7916Compareandozw
commented
Apr 29, 2022
rebased |
| return false; | ||
| } | ||
| if let OnionPayload::Invoice(ref final_hop_data) = htlcs[0].onion_payload { | ||
| if let OnionPayload::Invoice { _legacy_hop_data: ref final_hop_data } = htlcs[0].onion_payload { |
There was a problem hiding this comment.
Not a blocker, but it looks like we can get rid of this usage as well by accessing htlcs[0].total_msat instead of final_hop_data? @TheBlueMatt
4da7916 to
16a6ad2Compare
TheBlueMatt
left a comment
There was a problem hiding this comment.
Okay, sorry for the delay on this one, LGTM modulo two trivial comments.
| Invoice(msgs::FinalOnionHopData), | ||
| Invoice { | ||
| /// This is only here for backwards-compatibility in serialization, in the future it can be | ||
| /// removed, breaking clients running 0.0.104 and earlier. |
There was a problem hiding this comment.
heh, well that's stale - should read "0.0.106", assuming this lands for 107, which it should.
| (incoming_cltv_expiry, OnionPayload::Invoice(payment_data), phantom_shared_secret), | ||
| let (cltv_expiry, onion_payload, payment_data, phantom_shared_secret) = match routing { | ||
| PendingHTLCRouting::Receive { payment_data, incoming_cltv_expiry, phantom_shared_secret } => { | ||
| let _legacy_hop_data = msgs::FinalOnionHopData { |
There was a problem hiding this comment.
nit: replace the explicit instantiation with just let _legacy_hop_data = payment_data.clone(), given that's what we're doing anyway.
...instead of accessing it via the `OnionPayload::Invoice` form. This may be useful if we add MPP keysend support, but is directly useful to allow us to drop `FinalOnionHopData` from `OnionPayload`.
16a6ad2 to
1973f1fCompareandozw
commented
May 2, 2022
rebased |
1973f1f to
c611affCompareandozw
commented
May 2, 2022
Addressed the 2 minor fixups as an amend on the last commit since they belong there and were trivial. Should be ready for another 👀 |
| /// Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a | ||
| /// payment_secret which prevents path-probing attacks and can associate different HTLCs which | ||
| /// are part of the same payment. |
There was a problem hiding this comment.
Sorry should've seen this sooner, not a blocker but I think this can be removed and just say "Indicates this incoming onion payload is for the purpose of paying an invoice" or something like that
There was a problem hiding this comment.
So replace Contains a total_msat (which may differ from value if this is a Multi-Path Payment) and a payment_secret which prevents path-probing attacks and can associate different HTLCs which are part of the same payment.
with:Indicates this incoming onion payload is for the purpose of paying an invoice ?
We only use it to check the amount when processing MPP parts, but store the full object (including new payment metadata) in it. Because we now store the amount in the parent structure, there is no need for it at all in the `OnionPayload`. Sadly, for serialization compatibility, we need it to continue to exist, at least temporarily, but we can avoid populating the new fields in that case.
c611aff to
fc77c57Compare
Splits these commits from #1221 and #1445 because they should be separate.
See #1445 (review)