Uh oh!
There was an error while loading. Please reload this page.
Follow-ups to #2688 - #2791
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@## main #2791 +/- ##
==========================================
+ Coverage 88.64% 88.65% +0.01%
==========================================
Files 115 115 Lines 91894 91887 -7 Branches 91894 91887 -7 ==========================================
+ Hits 81458 81467 +9 + Misses 7953 7941 -12 + Partials 2483 2479 -4 ☔ View full report in Codecov by Sentry. |
shaavan
left a comment
There was a problem hiding this comment.
The follow-up changes look great!
I have one question regarding the changes and one suggestion that can further improve the PR!
| sha256_of_onion: self.0, | ||
| failure_code: self.1 |
There was a problem hiding this comment.
I think we should revert this change.
In the reading and writing of tlvs, sha256_of_onion is followed by failure_code.
write_tlv_fields!(w,{(0, htlc_id, required),(1, failure_code, required),(2, dummy_err_packet, required),(3, sha256_of_onion, required),});And if we want to prevent tuple destructing, I think we should follow tlvs order in the rest of the codebase.
There was a problem hiding this comment.
I think in principle I agree with you, we should have gone with (failure_code, sha256) rather than the opposite, but sadly we can't (easily) change the InboundHTLCRemovalReason serialization at this point, and IMO we should prefer to match our existing order everywhere rather than have a different order here. Its not all that critical in any direction, of course, though, we're talking about moving around 32+2 bytes.
| if let ChannelError::Ignore(msg) = e { | ||
| log_trace!(logger, "Failed to fail HTLC with ID {} backwards to short_id {}: {}", htlc_id, short_chan_id, msg); | ||
| } else { | ||
| panic!("Stated return value requirements in queue_fail_{{malformed_}}htlc() were not met"); |
There was a problem hiding this comment.
We are achieving a DRY code here, and that's great!
But we are also combining the panic messages that can be generated from two different sources here.
Would this be an optimal thing to do?
There was a problem hiding this comment.
That's a good point (interpreting this as "our stack trace from the panic message won't be quite as useful as we won't be able to differentiate between the two cases"), however in this case I'm not sure we care too much about being able to differentiate, so the DRYing is likely worth more. Further, we should generally be able to differentiate the cases anyway, as logs should show different messages being received.
vladimirfomene
left a comment
There was a problem hiding this comment.
ACK c8aa0b3. LGTM. Just had a question and suggestion.
| .map(|fail_msg_opt| fail_msg_opt.map(|_| ()))) | ||
| } | ||
| }; | ||
| match fail_htlc_res { |
There was a problem hiding this comment.
Let me know what you think but I was thinking having something like the following might me more readable:
if let Some(result) = fail_htlc_res {
match result {
Ok(fail_msg_opt) => {
debug_assert!(fail_msg_opt.is_some());
update_fail_count += 1;
},
Err(ChannelError::Ignore(_)) => {},
Err(e) => {
panic!("Got a non-IgnoreError action trying to fail holding cell HTLC");
},
}
}
There was a problem hiding this comment.
Sure, added a commit cleaning this up.
| let res = chan.queue_fail_malformed_htlc( | ||
| htlc_id, failure_code, sha256_of_onion, &&logger | ||
| ); | ||
| Some((res, htlc_id)) |
There was a problem hiding this comment.
Is there a reason for storing the result of the function call in a variable res instead of putting the function call in the tuple as done on L4400?
There was a problem hiding this comment.
I think I thought it was more readable this way since it wouldn't all fit in one line like with L4400
Introduced due to a rebase error.
This helps avoid destructuring the tuple.
This name is more accurate since the method has been generalized to support malformed HTLCs.
c8aa0b3 to
3ec4d52Compare
No description provided.