-
Notifications
You must be signed in to change notification settings - Fork 0
approach-1: Set outgoing signal in PendingHTLCInfo #23
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?
Conversation
Downside: - We create this struct early on, so have to set this value to none and then mutate it. Upside: - We coherently store the accountable signal with the other forwarding information that we'll be using later on.
d93db8f to
c62eb0f
Compare
|
@elnosh - first attempt at implementing blip 04! Please will you give me a sanity check on impl and then I'll write some tests + open upstream 🙏 |
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.
mostly one comment that I'm unsure about where nodes would provide their accountability signal from a reputation algorithm
| // Set the value for our outgoing accountable signal to copy the received | ||
| // incoming value (or just set zero if not present). This point is where we | ||
| // could introduce an interceptor that provides us with custom accountable | ||
| // values if desired. | ||
| pending_add.forward_info.outgoing_accountable = | ||
| pending_add.forward_info.incoming_accountable.or(Some(0)); | ||
|
|
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.
I'm not sure if this should be here or in process_forward_htlcs? IIUC, the outgoing channel on which the HTLC will end up being forwarded could potentially change there.
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.
Taken a shot at this here - I think that I prefer this approach for now, because it gives us good flexibility (see PR description). Also addresses the PendingHTLCRouting comment below nicely (in that we don't have to put it anywhere, which I like).
| /// An experimental field indicating whether the outgoing node's reputation would be held | ||
| /// accountable for the timely resolution of the offered HTLC. | ||
| pub outgoing_accountable: Option<u8>, |
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.
since this field won't apply for receives, would it be better to put it accordingly for each variant in PendingHTLCRouting?
Downside:
Upside:
Alternative is here, I like it less.