Set and relay experimental accountable signal #4232
Conversation
|
I've assigned @tankyleo as a reviewer! |
| nodes[1].node.handle_update_add_htlc(nodes[0].node.get_our_node_id(), &htlc_ab); | ||
| do_commitment_signed_dance(&nodes[1], &nodes[0], &updates_ab.commitment_signed, false, false); | ||
| expect_and_process_pending_htlcs(&nodes[1], false); | ||
| check_added_monitors!(nodes[1], 1); |
There was a problem hiding this comment.
nit: these can use the identically-named function instead of the macro.
| nodes[0] | ||
| .node | ||
| .send_payment(payment_hash, onion_fields, payment_id, route_params, Retry::Attempts(0)) | ||
| .unwrap(); | ||
| check_added_monitors!(nodes[0], 1); | ||
|
|
||
| let updates_ab = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id()); | ||
| assert_eq!(updates_ab.update_add_htlcs.len(), 1); |
There was a problem hiding this comment.
would it be good to check that the signal from the sender is set to 0
carlaKC
force-pushed
the
4181-experimental-accountable
branch
from
aa7abc3 to
5accf65
Compare
carlaKC
force-pushed
the
4181-experimental-accountable
branch
from
5accf65 to
9e14c82
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4232 +/- ##
==========================================
- Coverage 89.34% 89.32% -0.02%
==========================================
Files 180 181 +1
Lines 138620 138751 +131
Branches 138620 138751 +131
==========================================
+ Hits 123846 123941 +95
- Misses 12149 12187 +38
+ Partials 2625 2623 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tankyleo! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tankyleo! This PR has been waiting for your review. |
| } | ||
|
|
||
| /// Converts the accountable signal on the wire to a boolean signal. | ||
| pub fn accountable_into_bool(accountable: ExperimentalAccountable) -> Option<bool> { |
There was a problem hiding this comment.
When reading from the wire, would you consider converting directly into a bool here instead of Option<bool>, mapping None to false per the spec ?
Looks like we never really use the None variant of Option<bool>; we always call unwrap_or(false). So we could consolidate all of these calls into a single unwrap_or(false) in this function, get rid of any use of Option<bool> across the patchset, and jump only between Option<u8> the wire format, and bool, the internal LDK format.
| do_commitment_signed_dance(&nodes[2], &nodes[1], &updates_bc.commitment_signed, false, false); | ||
| expect_and_process_pending_htlcs(&nodes[2], false); | ||
| check_added_monitors(&nodes[2], 0); | ||
| expect_payment_claimable!(nodes[2], payment_hash, payment_secret, 100_000); |
There was a problem hiding this comment.
didn't give it a shot mysef, could dig into the PendingHTLCInfo of node 2 here to cover the create_recv_pending_htlc_info case ?
| outgoing_amt_msat: onion_amt_msat, | ||
| outgoing_cltv_value: onion_cltv_expiry, | ||
| skimmed_fee_msat: counterparty_skimmed_fee_msat, | ||
| incoming_accountable, |
There was a problem hiding this comment.
Mentioned in the test, can we easily get test coverage for this spot here?
4 participants
Fixes: #4181. This PR adds relay of the experimental
accountablesignal to LDK, as outlined in blip-04.After this PR LDK will:
accountablesignal set.accountablesignal is set, it'll copy the incoming value to the outgoingupdate_add_htlc.One note here is that we don't keep track of the incoming
accountablesignal once we've forwarded it on our outgoing link. This isn't a requirement for our existing reputation algorithm, so this seems fine (we could add toHTLCSourceif we needed it).