-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
invoice+rpc: add exit hop InvoiceAcceptor sub-systems and RPC calls #8669
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
c3f4d8a
to
dee0336
Compare
@jharveyb and @GeorgeTsagk will review |
// SafeCallback is a thread safe wrapper around an InterceptorClientCallback. | ||
type SafeCallback struct { | ||
// mu is a mutex that protects the callback field. | ||
mu sync.Mutex |
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.
Could also maybe use: https://pkg.go.dev/sync/atomic#Pointer?
Not sure it fits better tho. Still getting through the diff.
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 you like me to change this?
"github.com/lightningnetwork/lnd/lntypes" | ||
) | ||
|
||
// SafeCallback is a thread safe wrapper around an InterceptorClientCallback. |
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.
Where's InterceptorClientCallback
defined?
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.
Note that for this we also need to be able to feed in the wire HTLC TLV blobs. Today we don't store those on disk for accepted invoice HTLCs:
Lines 1981 to 2002 in aee0294
tlvStream, err := tlv.NewStream( | |
tlv.MakePrimitiveRecord(chanIDType, &chanID), | |
tlv.MakePrimitiveRecord(htlcIDType, &key.HtlcID), | |
tlv.MakePrimitiveRecord(amtType, &amt), | |
tlv.MakePrimitiveRecord( | |
acceptHeightType, &htlc.AcceptHeight, | |
), | |
tlv.MakePrimitiveRecord(acceptTimeType, &acceptTime), | |
tlv.MakePrimitiveRecord(resolveTimeType, &resolveTime), | |
tlv.MakePrimitiveRecord(expiryHeightType, &htlc.Expiry), | |
tlv.MakePrimitiveRecord(htlcStateType, &state), | |
tlv.MakePrimitiveRecord(mppTotalAmtType, &mppTotalAmt), | |
tlv.MakeDynamicRecord( | |
htlcAMPType, amp, amp.PayloadSize, | |
record.AMPEncoder, record.AMPDecoder, | |
), | |
tlv.MakePrimitiveRecord(htlcHashType, hash32), | |
tlv.MakePrimitiveRecord(htlcPreimageType, preimage32), | |
) | |
if err != nil { | |
return nil, err | |
} |
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.
Where's
InterceptorClientCallback
defined?
In the same package. I think it might be introduced in a later commit. I'll see if I can change that.
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.
The invoice settlement interceptor and interceptor interface are now in the same commit.
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.
Note that for this we also need to be able to feed in the wire HTLC TLV blobs. Today we don't store those on disk for accepted invoice HTLCs
Feed the wire HTLC TLV blobs into the interceptor? Or do you mean that we should store them to disk?
lnrpc/invoicesrpc/invoices.proto
Outdated
// request the client to accept an invoice. | ||
message InvoiceAcceptorRequest { | ||
// invoice is the invoice that the client should consider accepting. | ||
lnrpc.Invoice invoice = 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.
Cool, so this will have InvoiceHTLC
which is really what the interceptor wants.
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.
we might wanna add the wire level custom records on InvoiceHTLC
too, currently we only have
// Custom tlv records.
map<uint64, bytes> custom_records = 9;
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.
Yes. I think that's still needed. Not sure if in this PR.
dee0336
to
6cadf00
Compare
I think I also need to pass circuit key for latest HTLC along with invoice to settlement interceptor client. This way the client can inspect the correct HTLC. |
6cadf00
to
48c11c5
Compare
48c11c5
to
550d8ce
Compare
This is included in the latest commits. |
I'm in the process of writing an itest for this PR. I noticed that providing the exit HTLC circuit key to the acceptor interceptor client is insufficient. The invoice will only include the exit HTLC if it is a replayed HTLC. I will therefore need to provide fields from the invoice update context to the interceptor client. I will make changes to that effect. |
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.
Looks good! Can re-review once the itest is up.
} | ||
|
||
// Exec calls the client callback function. | ||
func (sc *SafeCallback) Exec(req InterceptClientRequest) error { |
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.
Do we want a separate context or timeout wrapping this callback? IIUC this would block notifyExitHopHtlcLocked
indefinitely if the interceptor goes offline, crashes, etc.
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.
In the latest commits, Exec
runs in a separate goroutine. And we wait for quit or a client response in notifyExitHopHtlcLocked
.
There shouldn't be a chance of it blocking if the invoice acceptor not engaged. It's not perfect. But it might do for now.
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 agree its fine for now & that there's no extra risk if there is no interceptor - I meant if we want to enforce a deadline on the interceptor response. Definitely non-blocking.
|
||
// Invoice is the invoice that is being intercepted. | ||
Invoice Invoice | ||
} |
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.
don't we want to add more data here? I'd imagine that for example in the tap channels case, you'd want to provide any custom blobs that would need to be checked here
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 think what precicely we need here will become more clear once we actually start using the InvoiceAcceptor
from tapd.
lnrpc/invoicesrpc/invoices.proto
Outdated
// request the client to accept an invoice. | ||
message InvoiceAcceptorRequest { | ||
// invoice is the invoice that the client should consider accepting. | ||
lnrpc.Invoice invoice = 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.
we might wanna add the wire level custom records on InvoiceHTLC
too, currently we only have
// Custom tlv records.
map<uint64, bytes> custom_records = 9;
reviewed diff, left out the WIP commits |
2f1825d
to
86d0c02
Compare
Latest state:
Things to consider:
|
86d0c02
to
0373b8d
Compare
In this commit, we add a new abstract message router. Over time, the goal is that this message router replaces the logic we currently have in the readHandler (the giant switch for each message). With this new abstraction, can reduce the responsibilities of the readHandler to *just* reading messages off the wire and handing them off to the msg router. The readHandler no longer needs to know *where* the messages should go, or how they should be dispatched. This will be used in tandem with the new `protofsm` module in an upcoming PR implementing the new rbf-coop close.
IIRC the plan was for the HTLC to be >= dust threshold / 330ish sats, so that seems fine. |
0373b8d
to
7d9d5cd
Compare
This commit introduces a new invoice settlement interceptor service that intercepts invoices during their settlement phase. It forwards invoices to subscribed clients to determine their settlement outcomes. This commit also introduces an interface to facilitate integrating the interceptor with other packages.
This commit introduces the `SkipAmountCheck` field to the `invoiceUpdateCtx` type. This field serves as a flag to determine whether to bypass the amount verification during the invoice settlement process. It is set based on the client's input, allowing the invoice to be settled even if the HTLC amount is less than the stated invoice amount.
This commit updates the invoice registry to utilize the settlement interceptor during the invoice settlement routine. It allows the interceptor to capture the invoice, providing interception clients an opportunity to determine the settlement outcome.
This commit initiates the invoice settlement interceptor during the main server startup, assigning it a handle within the server.
This commit integrates the settlement interceptor service into the invoices RPC server.
This commit introduces a singleton invoice acceptor RPC server and an endpoint to activate it. The server interfaces with the internal invoice settlement interpreter, handling the marshalling between RPC types and internal formats. Named "acceptor," it allows clients to accept invoice settlements, but not to reject them.
This commit enhances the itest LND node harness to include support for the new `InvoiceAcceptor` RPC endpoint.
This commit introduces a basic integration test for the invoice acceptor. The test covers scenarios where an invoice is settled with a payment that is less than the invoice amount, facilitated by the invoice settlement acceptor.
7d9d5cd
to
989cc26
Compare
15abb14
to
09b34f1
Compare
@Roasbeef: review reminder |
f9b366e
to
5a834c1
Compare
I need to double check whether there are modifications in the PoC branch which relate to this PR. |
3b45569
to
12d4694
Compare
Replaced by #8771. |
Change Description
Closes #8616
This pull request introduces the following components:
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.