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
| pub(crate) fn validate_and_parse_payment_hash( | ||
| payment_hash_str: &str, | ||
| ) -> Result<PaymentHash, APIError> { | ||
| if payment_hash_str.is_empty() { | ||
| return Err(APIError::InvalidPaymentHash("missing payment_hash".into())); | ||
| } | ||
| let hash_vec = hex_str_to_vec(payment_hash_str) | ||
| .ok_or_else(|| APIError::InvalidPaymentHash(payment_hash_str.to_string()))?; | ||
| if hash_vec.len() != 32 { | ||
| return Err(APIError::InvalidPaymentHash(payment_hash_str.to_string())); | ||
| } | ||
| let hash_bytes: [u8; 32] = hash_vec | ||
| .try_into() | ||
| .map_err(|_| APIError::InvalidPaymentHash(payment_hash_str.to_string()))?; | ||
| Ok(PaymentHash(hash_bytes)) | ||
| } | ||
|
|
||
| pub(crate) fn validate_and_parse_payment_preimage( | ||
| payment_preimage_str: &str, | ||
| payment_hash: &PaymentHash, | ||
| ) -> Result<PaymentPreimage, APIError> { | ||
| let preimage_vec = | ||
| hex_str_to_vec(payment_preimage_str).ok_or_else(|| APIError::InvalidPaymentPreimage)?; | ||
| if preimage_vec.len() != 32 { | ||
| return Err(APIError::InvalidPaymentPreimage); | ||
| } | ||
| let preimage = PaymentPreimage( | ||
| preimage_vec | ||
| .try_into() | ||
| .map_err(|_| APIError::InvalidPaymentPreimage)?, | ||
| ); | ||
| let computed_hash = PaymentHash(Sha256::hash(&preimage.0).to_byte_array()); | ||
| if computed_hash != *payment_hash { | ||
| return Err(APIError::InvalidPaymentPreimage); | ||
| } | ||
| Ok(preimage) | ||
| } |
There was a problem hiding this comment.
Please fix the validate_and_parse_payment_hash method by using the already existing code that validates the payment hash (e.g. see the get_payment_preimage route method) and then use the new validate method in ALL APIs that validate the payment hash. Same for validate_and_parse_payment_preimage
There was a problem hiding this comment.
validate_and_parse_payment_hash already follows the same validation semantics and has additional benefits, such as improved error handling. It is used multiple times, and also in the upcoming submarine swaps API. I forgot to call it from get_payment_preimage, but I'll refactor it to use this helper too.
I double-checked validate_and_parse_payment_preimage, and it's also used in the submarine swaps PR for the htlc_claim preimage input. I'd appreciate it if we could keep it.
There was a problem hiding this comment.
Replaced manual payment-hash parsing with validate_and_parse_payment_hash in: get_payment, get_payment_preimage, and get_swap.
There was a problem hiding this comment.
your version:
if payment_hash_str.is_empty() {
return Err(APIError::InvalidPaymentHash("missing payment_hash".into()));
}
let hash_vec = hex_str_to_vec(payment_hash_str)
.ok_or_else(|| APIError::InvalidPaymentHash(payment_hash_str.to_string()))?;
if hash_vec.len() != 32 {
return Err(APIError::InvalidPaymentHash(payment_hash_str.to_string()));
}
let hash_bytes: [u8; 32] = hash_vec
.try_into()
.map_err(|_| APIError::InvalidPaymentHash(payment_hash_str.to_string()))?;
Ok(PaymentHash(hash_bytes))my version:
let payment_hash_vec = hex_str_to_vec(&payload.payment_hash);
if payment_hash_vec.is_none() || payment_hash_vec.as_ref().unwrap().len() != 32 {
return Err(APIError::InvalidPaymentHash(payload.payment_hash));
}
Ok(PaymentHash(payment_hash_vec.unwrap().try_into().unwrap()))Error handling doesn't seem improved to me. Checking the string is empty is redundant, you already check the length is 32. Also hash_vec.try_into() cannot fail since you already checked the vector has length 32. Therefore I fail to see the improvements you mention.
There was a problem hiding this comment.
You're right that in this specific flow neither try_into() nor unwrap() can panic because the length is checked beforehand.
The goal of the helper is not to prevent a panic in this exact branch, but to enforce a consistent rule that API-layer parsing never uses unwrap() and always returns a structured APIError. This removes implicit assumptions and makes future refactors safer.
I agree the empty-string check is redundant and will remove it. Thanks.
There was a problem hiding this comment.
There are cases in Rust where the unwrap is safe to use. In this case not using it means having code that will never be executed and cannot be covered by the tests. Please change as requested
| .channel_id | ||
| } | ||
|
|
||
| async fn get_payment_preimage( |
There was a problem hiding this comment.
I don't see why we need the /getpaymentpreimage API, please drop it and in the test just use wait_for_ln_payment instead of wait_for_payment_preimage
There was a problem hiding this comment.
It is required to claim the funds on-chain as part of a submarine swap, or in other use cases where the release of the preimage after payment enables the counterparty to finalize the operation.
There was a problem hiding this comment.
We can just change the current Payment object to include an optional preimage, so that a user can call the already existing /getpayment API
| let _claimable = unlocked_state.mark_claimable_settling(&payment_hash, metadata.expiry)?; | ||
|
|
||
| unlocked_state.channel_manager.claim_funds(preimage); |
There was a problem hiding this comment.
I'm not sure I understand the logic behind the "claimable" concept, together with the "settling" one. Isn't the claim on the channel manager immediate? If so I think it's better to first claim funds and then mark them as claimed
There was a problem hiding this comment.
claimable and settling represent two different phases, and both are intentional.
claimable: we receivedEvent::PaymentClaimablefor a HODL invoice, so the HTLC is held and awaiting user action.settling:/settlehodlinvoicewas accepted andclaim_funds(...)was requested, butEvent::PaymentClaimedhas not been processed yet.
claim_funds(...) initiates claim handling, but the terminal state transition is event-driven and asynchronous. That creates a TOCTOU window between “checked as claimable” and “claim confirmed.” We set settling=true before claim_funds(...) to close that window and prevent concurrent conflicting operations (cancel/second settle) on the same HTLC.
When Event::PaymentClaimed arrives, we mark the payment Succeeded and remove the claimable record.
There was a problem hiding this comment.
I don't think we need ClaimablePayment, we could use PaymentInfo (adding missing fields there). I'm also not sure we need the InvoiceMetadata, I think we should find a way to avoid it.
I think it's incorrect to add the Cancelled status to the HTLCStatus, I think we could use the Failed one, do you see a reason why it's important to distinguish the 2?
I don't see why we need both settling and settling_since, it seems we can keep only the latter, also I would rename it to claiming_since to avoid confusion. Same for the API name, probably instead of /settlehodlinvoice a more fitting name would be /claimhodlinvoice.
Also please drop the mark_claimable_settling method, since it's used only in the route there's no reason to move the logic there, it just makes harder to understand what the API does.
txalkan
force-pushed
the
main
branch
2 times, most recently
from
668a57d to
24e6593
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
| let invoice = match payload.payment_hash { | ||
| Some(payment_hash) => { | ||
| let payment_hash = validate_and_parse_payment_hash(&payment_hash)?; | ||
|
|
||
| let hash_already_used = unlocked_state | ||
| .invoice_metadata() | ||
| .contains_key(&payment_hash) | ||
| || unlocked_state | ||
| .inbound_payments() | ||
| .contains_key(&payment_hash) | ||
| || unlocked_state.claimable_payment(&payment_hash).is_some(); | ||
| if hash_already_used { | ||
| return Err(APIError::PaymentHashAlreadyUsed); | ||
| }; | ||
| let invoice_params = Bolt11InvoiceParameters { | ||
| amount_msats: payload.amt_msat, | ||
| invoice_expiry_delta_secs: Some(payload.expiry_sec), | ||
| payment_hash: Some(payment_hash), | ||
| contract_id, | ||
| asset_amount: payload.asset_amount, | ||
| ..Default::default() | ||
| }; | ||
|
|
There was a problem hiding this comment.
I still see a lot of duplication here, please deduplicate
| #[derive(Deserialize, Serialize)] | ||
| pub(crate) struct SettleHodlInvoiceRequest { | ||
| pub(crate) payment_hash: String, | ||
| pub(crate) payment_preimage: String, |
There was a problem hiding this comment.
I don't think we need the payment preimage here, we can save it during the PaymentClaimable event. This way we reduce the possibility of errors and also we unlock swaps using HODL invoices, where a user creates a HODL invoice without knowing the preimage of the given payment hash
| let _claimable = unlocked_state.mark_claimable_settling(&payment_hash, metadata.expiry)?; | ||
|
|
||
| unlocked_state.channel_manager.claim_funds(preimage); |
There was a problem hiding this comment.
I don't think we need ClaimablePayment, we could use PaymentInfo (adding missing fields there). I'm also not sure we need the InvoiceMetadata, I think we should find a way to avoid it.
I think it's incorrect to add the Cancelled status to the HTLCStatus, I think we could use the Failed one, do you see a reason why it's important to distinguish the 2?
I don't see why we need both settling and settling_since, it seems we can keep only the latter, also I would rename it to claiming_since to avoid confusion. Same for the API name, probably instead of /settlehodlinvoice a more fitting name would be /claimhodlinvoice.
Also please drop the mark_claimable_settling method, since it's used only in the route there's no reason to move the logic there, it just makes harder to understand what the API does.
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.