-
Notifications
You must be signed in to change notification settings - Fork 421
Set and relay experimental accountable signal #4232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Set and relay experimental accountable signal #4232
Conversation
|
I've assigned @tankyleo as a reviewer! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There's a test making the CI unhappy
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be good to check that the signal from the sender is set to 0
aa7abc3 to
5accf65
Compare
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:
|
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).