Add HODL Invoices#91
Conversation
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
Thank you!
This is just a first review, will do a more detailed one after these changes are addressed:
- revert changes to the github workflows
- keep a single signed commit called
add support for HODL invoices - drop nix/flake related files
- drop changes to
regtest.sh(shouldn't be necessary) or explain why they are necessary - rename APIs as requested in the initial google doc shared at the beginning of the task
- please keep alphabetical order (in README, openapi.yaml, main.rs and routes.rs)
- drop HODL Invoices explanation from the README, let's keep API documentation in the openapi.yaml (consise doc is preferred)
- in openapi.yaml use the same syntax we used for other objects (in
requiredfor example) - drop docstrings from route methods (all other methods do not have it) and keep doc in openapi.yaml
- in
src/test/hodl_invoice.rsmove imports tosrc/test/mod.rs - in
src/test/hodl_invoice.rsplease cleanup the code, keep the API calls logic in the mod.rs file (as we did for all other tests), avoid methods that are called only once and repeating utility methods that are already defined in the mod.rs file - in
src/utils.rskeep methods only if used more than once
It waits for the Bitcoin RPC to be fully ready, preventing race conditions previously observed where Electrs or other services started too early and failed intermittently. The rest I believe has been addressed, let me know. |
Are you sure about this? Please provide more details because we never witnessed this and have run the tests hundreds of times on multiple different machines.
Not exactly, code and especially tests are still quite messy. Still seeing a couple of TODOs in the code, tests that could be merged to save the initialization time and reduce code, very long sleeps that could be replaced with smart waiting functions, expirations that can be shortened, mixed documentation style (some parts with very long and detailed comments and some without any comment), alphabetical order is not respected and some parts are commented in an inconsistent way (with respect to surrounding code) or excessive (e.g. just repeating the name of the object, documenting other objects where they're used) Also, there are 2 failing tests. |
I experienced the same race condition only on mac, every thing is smooth on llinux. |
Right, thanks @Arshia-r-m -- so it doesn't hurt to wait for bitcoin before running the indexers, does it? |
Regarding the tests:
All tests pass locally. |
No, let's keep it but instead of adding a new method just change the
Not sure what are you saying here. If the test has a race condition you need to fix it, we cannot have tests with an nondeterministic behavior
Also here not sure what are you referring to. Is the test failing? I've never seen it fail so please share some logs if so |
|
Hi @zoedberg, Regarding the alphabetical order: in some files the existing items are not fully ordered, so aligning everything would require moving other functions as well. I can do that if you prefer, but since it affects pre-existing code, it might be safer for maintainers to move around. Please let me know how you’d like to proceed. |
|
@txalkan Could you point me to the items that are not fully ordered please? |
Could be nicer to order |
@txalkan They seem ordered to me.
To me in |
What do you mean? This is the current position: 
|
|
Ah now I understood, I thought you were saying that |
|
Hi @zoedberg, rebase done. Regarding removing /sendasset in #94: this is a breaking change on our side (we already use this endpoint from a client). What was the rationale for removing it instead of keeping the route and extending the request/handler to support multi-transfer (e.g., upgrading the function signature / request schema)? Another question: we’re considering introducing our own branch (e.g. utexo-master) so that master can stay strictly aligned with upstream. At the moment, GitHub Actions are only enabled for master. Would you consider updating the workflows to also run on a more general branch such as main, so we can use that for internal development while keeping master clean? |
The reason is that maintaining many APIs is expensive and there's no reason to keep 2 APIs that basically do the same thing. Moreover RLN is still in alpha phase where several breaking changes are still expected. I hope/assume updating your side will not cost that much.
I think what makes more sense is just to have a commit on your fork that changes the workflow. |
If the goal is to avoid maintaining multiple APIs, why not extend
I understand the suggestion to change the workflow on our fork. My concern is that any fork-only commits (even CI-only) make the fork drift from upstream over time, which makes future rebases and contributions more cumbersome to maintain. Since this is just broadening the workflow trigger (and doesn’t change the node behavior), it seems like a reasonable upstream tweak rather than something we should carry uniquely in our fork. |
|
Submarine Swap of BTC+RGB Leaving this here for context, as it builds directly on the HODL invoice support: #85 (comment) |
It's just an API rename,
I understand the concern about forks drifting from upstream, but in this case the workflow file is exactly the kind of configuration that is expected to be fork-specific. The project’s default branch is |
|
Hi @zoedberg, can we move forward with this merge? |
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
Here another small review, will do a deeper one later on, please first address the requested changes
txalkan
force-pushed
the
main
branch
3 times, most recently
from
24e6593 to
17ea616
Compare
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
I answered to the pending conversations, please also rebase this PR on top of the updated master
|
Hi @zoedberg, I addressed the rest of the review points:
For status semantics, I’d prefer to keep
Important clarification on HODL lifecycle: Also, claiming is an explicit user action, same as canceling: the user reveals/provides the preimage only when they decide to accept the payment. |
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
Important clarification on HODL lifecycle:
PaymentClaimabledoes not always reveal the preimage. In the hash-based HODL flow (invoice created from a provided payment hash),payment_preimagecan beNone, so we cannot rely on that event to store/use the preimage.
Oh correct, sorry I got confused during the review.
Left some other changes requests. Generally speaking please try to keep the diff as small as possible. Cosmetic changes make the review harder.
Also I see you didn't change Payment, therefore when listing payments it's impossible to distinguish hodl invoices from auto claim ones. I propose to replace the inbound bool with an enum like
enum PaymentType {
Outbound,
InboundAutoClaim,
InboundHodl,
}| let now_ts = get_current_timestamp(); | ||
| if let Some(expiry) = invoice_payment.expires_at { | ||
| if now_ts >= expiry { | ||
| tracing::warn!( | ||
| "Received HTLC for expired invoice {payment_hash:?} (expiry {expiry})" | ||
| ); | ||
| unlocked_state | ||
| .channel_manager | ||
| .fail_htlc_backwards(&payment_hash); | ||
| unlocked_state.upsert_inbound_payment( | ||
| payment_hash, | ||
| HTLCStatus::Failed, | ||
| payment_preimage, | ||
| payment_secret, | ||
| Some(amount_msat), | ||
| unlocked_state.channel_manager.get_our_node_id(), | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is incorrect, the event should be triggered only if the invoice is not expired
There was a problem hiding this comment.
I don't think we can rely on PaymentClaimable implying the invoice is still within expiry. LDK's docs explicitly say it may generate PaymentClaimable for some time after expiry, and that exact expiry semantics should be enforced upon receipt of the event.
There was a problem hiding this comment.
could you please point me to where this is documented in LDK?
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
I'm sorry I still haven't found the time to re-review this and answer to the pending comments. In the meatime though please add a test that uses HODL invoices to perform a swap. This test should show an issue reported in RGB-Tools/rust-lightning#20 and RGB-Tools/rust-lightning#24
It seems like @free-free-6 was right to point out this issue. I've been investigating it here (txalkan@0c97f7e), and BTC-only payouts fail when an RGB pay-in with the same hash is already present. This does not affect the RGB/USDT use case (e.g., async payments) where the outbound leg is RGB-backed. To support the latter, I made |
zoedberg
left a comment
zoedberg
left a comment
There was a problem hiding this comment.
Please address the requested changes.
The hodl_invoice::claim_hodl_invoice_btc_rgb test is failing on my machine but passing on the CI, this means it's not deterministic. Please fix it. Here the fail logs:
thread 'test::hodl_invoice::claim_hodl_invoice_btc_rgb' (3551184) panicked at src/test/hodl_invoice.rs:473:5:
assertion failed: matches!(invoice_status(node2_addr, &invoice).await, InvoiceStatus::Pending)
Generally speaking tests are very messy and hard to read, could you please add some comments to separate subtests that are in the same test and briefly explain what that subtest is checking/doing?
Please also lint the code and rebase this on top of the latest master tip.
It seems like @free-free-6 was right to point out this issue. I've been investigating it here (txalkan/rgb-lightning-node@0c97f7e), and BTC-only payouts fail when an RGB pay-in with the same hash is already present. This does not affect the RGB/USDT use case (e.g., async payments) where the outbound leg is RGB-backed. To support the latter, I made get_payment lookup type-aware, so inbound and outbound records sharing the same hash are no longer conflated.
Could you please add the swap test to this PR? About the fix I would like to discuss it after seeing the test reproducing the reported issue.
| asset_id: Option<&str>, | ||
| asset_amount: Option<u64>, | ||
| expiry_sec: u32, | ||
| payment_hash: Option<String>, |
There was a problem hiding this comment.
HODL invoices are the ones that have they payment_has set, so the parameter here must be non optional
| asset_amount: Option<u64>, | ||
| expiry_sec: u32, | ||
| payment_hash: Option<String>, | ||
| invoice_type: InvoiceType, |
There was a problem hiding this comment.
no need for this invoice_type parameter here, just log the optional payment hash and it will be clear if it's HODL or not
| check_response_is_nok(res, expected_status, expected_message, expected_name).await | ||
| } | ||
|
|
||
| async fn run_auto_claim_invoice_regression_case(node1_addr: SocketAddr, node2_addr: SocketAddr) { |
There was a problem hiding this comment.
no need for this, since there are already other tests with normal invoices and payments. please remove it
| let now_ts = get_current_timestamp(); | ||
| if let Some(expiry) = invoice_payment.expires_at { | ||
| if now_ts >= expiry { | ||
| tracing::warn!( | ||
| "Received HTLC for expired invoice {payment_hash:?} (expiry {expiry})" | ||
| ); | ||
| unlocked_state | ||
| .channel_manager | ||
| .fail_htlc_backwards(&payment_hash); | ||
| unlocked_state.upsert_inbound_payment( | ||
| payment_hash, | ||
| HTLCStatus::Failed, | ||
| payment_preimage, | ||
| payment_secret, | ||
| Some(amount_msat), | ||
| unlocked_state.channel_manager.get_our_node_id(), | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
could you please point me to where this is documented in LDK?
| } | ||
| let deadline_passed = payment | ||
| .claim_deadline_height | ||
| .map(|h| current_height >= h) |
There was a problem hiding this comment.
by checking LDK you can see that claim_deadline is always set to Some, I think it's optional only to support old LDK versions. so please use expect here. I would also rename claim_deadline_height to claim_deadline so that's easier to see the connection between the 2
3 participants
This PR adds support for HODL invoices to the RGB Lightning Node per the #50 issue.
The main functional addition is the ability to create and manage HODL invoices, requiring updates across the API, core logic, error handling, persistence layer, and a new test suite. Incoming HTLCs are held and only settled or cancelled explicitly.
/invoice/hodl,/invoice/settle, and/invoice/cancel.Other adjustments to workflows and documentation ensure the feature is well integrated.
src/routes.rsadded new routes to create HODL invoices, settle or cancel them. It includes request validation and JSON responses that comply with the updated OpenAPI spec. Wired the new routes intosrc/main.rs.src/ldk.rsupgraded how relevant events are processed. This includes new handlers for holding and settling payments, and updates to invoice retrieval and status checks.src/test/hodl_invoice.rsis a comprehensive test suite verifying HODL invoice creation, payment flows, and settlement/cancellation scenarios.More information and diagrams to support advanced flows such as submarine swaps are documented in: feat_hodl_invoice_v0.1.pdf.
A working submarine swap PoC (from Signet to Regtest) is available at https://github.com/UTEXO-Protocol/thunder-swap.