Add HODL Invoices

[txalkan]

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.

• Primary API additions include: /invoice/hodl, /invoice/settle, and /invoice/cancel.

Other adjustments to workflows and documentation ensure the feature is well integrated.

• src/routes.rs added 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 into src/main.rs.
• src/ldk.rs upgraded 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.rs is 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.

[zoedberg]

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 required for example)
• drop docstrings from route methods (all other methods do not have it) and keep doc in openapi.yaml
• in src/test/hodl_invoice.rs move imports to src/test/mod.rs
• in src/test/hodl_invoice.rs please 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.rs keep methods only if used more than once

[txalkan]

• drop changes to regtest.sh (shouldn't be necessary) or explain why they are necessary

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.

[zoedberg]

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.

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.

The rest I believe has been addressed, let me know.

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.

[Arshia-r-m]

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.

I experienced the same race condition only on mac, every thing is smooth on llinux.

[txalkan]

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.

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?

[txalkan]

there are 2 failing tests

Regarding the tests:

• test::hodl_invoice::settling_while_settling_fails -- another race condition.
• test::swap_roundtrip_buy_same_channel::swap_roundtrip_buy_same_channel -- unrelated to this PR; it seems like an RGB transport endpoint issue on CI.

All tests pass locally.

[zoedberg]

so it doesn't hurt to wait for bitcoin before running the indexers, does it?

No, let's keep it but instead of adding a new method just change the until logic of the already existing _wait_for_bitcoind please

test::hodl_invoice::settling_while_settling_fails -- another race condition

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

test::swap_roundtrip_buy_same_channel::swap_roundtrip_buy_same_channel -- unrelated to this PR; it seems like an RGB transport endpoint issue on CI.

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

[txalkan]

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.

[zoedberg]

@txalkan Could you point me to the items that are not fully ordered please?

[txalkan]

@txalkan Could you point me to the items that are not fully ordered please?

• rgb-lightning-node/src/routes.rs

  Line 2189 in 75f9ac5

  pub(crate) async fn get_payment(

• rgb-lightning-node/src/routes.rs

  Line 2332 in 75f9ac5

  pub(crate) async fn get_swap(

Could be nicer to order mod.rs as well, so it’s clearer where new functions should be added.

[zoedberg]

• rgb-lightning-node/src/routes.rs

  Line 2189 in 75f9ac5

  pub(crate) async fn get_payment(

• rgb-lightning-node/src/routes.rs

  Line 2332 in 75f9ac5

  pub(crate) async fn get_swap(

@txalkan They seem ordered to me.

Could be nicer to order mod.rs as well, so it’s clearer where new functions should be added.

To me in src/test/mod.rs there's no need for an alphabetical order.

[txalkan]

They seem ordered to me.

What do you mean? This is the current position:

[zoedberg]

Ah now I understood, I thought you were saying that get_payment and get_swap methods where unordered between them, but you meant between other methods around. Thanks for reporting this. I just merged PR #94 and fixed this on a commit on top. Please rebase your PR on the updated master

[txalkan]

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?

[zoedberg]

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)?

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.

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?

I think what makes more sense is just to have a commit on your fork that changes the workflow.

[txalkan]

The reason is that maintaining many APIs is expensive and there's no reason to keep 2 APIs that basically do the same thing.

If the goal is to avoid maintaining multiple APIs, why not extend /sendasset to support the new multi-transfer behavior, instead of introducing a new endpoint and removing /sendasset? From a contributor/client perspective, it’s more work to delete all the existing /sendasset code paths and data structures than to evolve them to support the new functionality.

I think what makes more sense is just to have a commit on your fork that changes the workflow.

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.

[txalkan]

Submarine Swap of BTC+RGB

Leaving this here for context, as it builds directly on the HODL invoice support: #85 (comment)

[zoedberg]

If the goal is to avoid maintaining multiple APIs, why not extend /sendasset to support the new multi-transfer behavior, instead of introducing a new endpoint and removing /sendasset? From a contributor/client perspective, it’s more work to delete all the existing /sendasset code paths and data structures than to evolve them to support the new functionality.

It's just an API rename, sendasset wasn't accurate anymore. On the client side this should be just a rename the same way it has been on the server side. If this translates to many changes then probably you have some duplication issues on your side.

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.

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 master, and the CI is defined around the upstream repository’s structure. A fork choosing a different default branch is already a divergence in repository policy, and adapting CI behavior to that is part of maintaining the fork rather than something upstream should account for. Adding main to the upstream workflow wouldn't actually improve upstream behavior, it would only accommodate forks using a different branch naming convention, and upstream would then carry configuration for branches that don’t exist here. Also in practice, a one-line workflow change does not meaningfully complicate rebases: CI files rarely conflict unless upstream modifies the same trigger section, and if that happens the adjustment is trivial. So I think the cleanest approach remains to keep the upstream workflow aligned with the upstream repository, and let forks adjust it to their layout when needed.