Skip to content
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

Update to LDK 0.4.2 #53

Merged
merged 173 commits into from
Nov 4, 2024
Merged

Update to LDK 0.4.2 #53

merged 173 commits into from
Nov 4, 2024

Conversation

rolznz
Copy link

@rolznz rolznz commented Oct 22, 2024

Closes #52

NOTES:

  • The BDK old descriptor is not migrated. But there would have to be 20 unused addresses in a row for this to happen (very unlikely). BDK will continue to search on the initial sync if any within 20 addresses were used.
  • BDK now allows "incremental sync" in LDK-node which doesn't actually mean it does partial syncs, but it is better
  • BDK sqlite DB is no longer used. BDK data is now saved in ldk_node_data.sqlite.
  • VSS cannot currently be used by existing nodes.

TODOs:

  • update to 0.4.2
  • Create PR to update Alby Hub to consume these changes chore: update LDK-node dependency to 0.4.2 hub#753
  • add logging back in for syncs so we can see sync durations
  • Alby BDK fork was removed.
    • Is it still needed to ensure we don't sync too many addresses?
    • test with a brand new node
    • test with an imported seed
    • test with an existing node with funds on a far address (BDK old descriptor state will not be automatically migrated)
  • LDK now has partial syncs after the first full sync. Should this be disabled in our fork, or how can we benefit from it? disabled for now
  • Test with brand new node
  • Test with existing node
  • Ensure no Alby-specific functionality has been removed (I have reviewed code changes in this PR, I don't think any Alby-specific changes were removed except the BDK fork)
  • Test with existing node with channels, migrating from 0.3 -> 0.4
  • Test VSS
    • local implementation (no auth)
    • JWT auth
  • ensure no sync deadlocks and that individual esplora clients for different actions are no longer needed (due to BDK upgrade) - hard to test locally
  • check if static backup is still possible - yes,
  • Check if reset network graph / scorer still works

elnosh and others added 30 commits June 21, 2024 15:02
We add a `Node::start_with_runtime` method that allows to reuse a
pre-existing runtime, e.g., to avoid stacking runtime contexts when
running in a tokio async environment.
…-uniffi-target-folder

Ignore all `target` folders including bindings
…tart-with-runtime

Allow start from outer runtime
Revert "Pin `cc` to `1.0.105` for MSRV"
* Add `UnifiedQrPayment` module for BIP21 URIs

Firstly, I thought I staged and made commits for `unified_qr.rs`
so sorry if this is out of order!

But in `unified_qr.rs` I
  - I introduced the `UnifiedQrPayment` struct to handle creating
    and paying BIP21 URIs
  - `receive` generates a URI with an on-chain address and BOLT11
     invoice and returns the URI as a string
  - `send` will parse a given URI string and attempt to send the
    BOLT12 offer, BOLT11 invoice, then if those fail the fallback
    on-chain address will be paid to.
  - Then I included tests for URI generation and URI parsing
  - Also has logging and error handling for payment operations

* Add `unified_qr_payment` payment handler to `Node`

  - implement unified_qr_payment method to create the
    Unified QR  payment handler
  - Includes conditional UniFFI features and updates docs
    with BIP21 and BOLT11 links

* Add support for `unified_qr` in payment mod

  - Included unified_qr in payment module
  - Added `PaymentResult` and `UnifiedQrPayment`
    from unified_qr for public use

* Add bip21 crate to handle BIP21 URIs

* Add `UnifiedQrPayment` and `PaymentResult` to `ldk_node.udl`

  - Introduced `UnifiedQrPayment` method to `Node` interface
  - Add `UnifiedQrPayment` interface with `receieve and
    `send` methods
  - Add `PaymentResult` interface (enum) with `Onchain`,
    `Bolt11` and `Bolt12` fields

These changes add support for our UniFFI bindings and enable
the use of `unified_qr_payment` payment handler in Swift,
and Kotlin.

* Update `Error` enum with URI related errors

  - Add `UriParameterFailed` and `InvalidUri` fields to
    the `Error` enum
  - Added related error  messages in the Display impl for
    the new fields

* Add `PaymentResult` import for UniFFI bindings

  - Added `PaymentResult` so the .udl could access
    the enum
  - Added comment to explain the need to import any
    re-exported items to enure they're accessible in
    UniFFI. (becasue rustc says to add them in `lib.rs`

* Add Unified QR `send`/`receive` integration tests

  - Added `unified_qr_send_receive` test to verify the `UnifedQrPayment`
    functionality
  - Added logic to handle paying a `BOLT12` offer, `BOLT11` invoice,
    and if those fail `On-chain` tx from a URI.
  - Validated each payments successful event
  - Ensured the off-chain and on-chain balacnes reflected the payment
    attempts

* Update PR with optimizations and nit fixups

The changes include:
  - Fixed a handful of nits for better readability in
    docs and simple grammar errors and made various name
    changes that affected the committed files.
  - Added a helper function in unified_qr.rs called
    capitalize_qr_params to format the lightning param in
    the receive method
  - Removed the optional message in the receive method and
    made it a required &str
  - Adjusted UDL formatting to use tabs instead of spaces

These changes were made to improve code quality and
maintainability based on the review feedback

* Refactor URI parsing and add Bolt12 offer in receive

Changes include:
  - Modified serialize_params to serialize both invoices and offers
  - Refactored deserialize_temp by removing the code that was
    parsing based on the lightning invoice/offer prefix. I instead
    used for loop to iterate over each lightning parameter,
    attempting to parse the string as an offer first, and then as an
    invoice. May need to log an error if neither succeeds
  - Added support for Bolt12 offers in the receive method
  - Updated capitalize_params function to handle multiple lightning
    parameters
  - Added a generate_bip21_uri test to show what the uri looks
    like in integration_tests_rust
  - Adjusted integration tests. Still needs work

Still trying to figure out a bug related to Bolt12 offers being
"paid" when it should fall back to an on-chain tx

* Update BOLT12 offer to use `lno` key

In this commit:
  - In serialize_params, BOLT12 offers were changed
    to be serialized with the `lno` key rather than
    the `lightning` key
  - During deserializing, I had to make the same update.
    Used a match to check whether it was a `lightning`
    or `lno` key and then parsed accordingly.
  - Next, a small name change: capitalize_qr_params to
    format_uri. Previously I changed the value after
    "&lightning" to all caps, but the "&lno=" value
    wasn't being changed. So, added a helper method inside
    format_uri to capitalize the values given the key!
  - Updated corresponding tests with `lno` update

Small nits:
  - Updated QrPaymentResult with more thorough docs
  - Added a parsing test with an offer

* Refactor for clarity and improve error handling

This commit fixes a handful of minor comments/nits
that include:
  - Updates to set the `bip21`  crates default-features to false,
    to minimize dependencies.
  - Enable the `std` feature since we use/benefit from it.

  - In `receive` return `InvoiceCreationFailed` or `OfferCreationFailed`
    when creating an invoice or offer. Rather than silently logging the
    error.
  - Also in `receive` we first check if an amount is specified, and if
    not, return an error and abort.
  - Pass in `Config` to `UnifiedQrPayment` struct to use the users config
    network.
  - In `send` instead of checking each network for the `NetworkChecked`
    URI, we pass in the `Config::Network`.
  - Simplifed param parsing in `deserialize_temp` by directly finding
    the key and parsing the corresponding value.

  - General documentation fixes.
  - In parsing tests, moved longer invoice/offer strings into.
    variables that start with expected_ for clarity.

* Fix docs for clarity

Cleaned up the docs so they are easier to understand for the
user. Also changed the message param in receive to description.
.. rather than once per target.
Recently, Rust 1.80 introduced automatic checking of `cfg` flags (see
https://blog.rust-lang.org/2024/05/06/check-cfg.html). Here,
we add all custom `cfg`s to the expected list
…-test-cfg

Fix: added `cfg(cln_test)` to `Cargo.toml`
Pin `tokio` to v1.38.1 to fix MSRV build
…oads

Enable caching for `bitcoind`/`electrs` in CI
.. just to be sure.
…os-specific

Make cache `key`s OS-specific
... see if dropping $HOME works.
.. since it stopped working now?
@rolznz
Copy link
Author

rolznz commented Oct 23, 2024

Run VSS server locally:

  • Install gradle using sdkman
  • Install Java 17 using sdkman sdk install java 17.0.13-amzn

cd java
gradle wrapper --gradle-version 8.1.1
./gradlew --version
./gradlew build

docker compose up

Rebuild:

docker compose down

docker compose build

View the database:

psql postgresql://postgres:YOU_MUST_CHANGE_THIS_PASSWORD@localhost:5432/postgres

tnull and others added 7 commits October 28, 2024 11:24
…s-program-len

Fix invalid witness program length
We previously erroneously included the version byte trying to construct
a `WitnessProgram`, which was was recently fixed. Here we add some more
comments to the code explaining what went wrong, and also add a debug
assertion checking `list_unspent_utxos` retrieves at least one `Utxo`
when we see any confirmed balances.
@rolznz rolznz changed the title Feat/alby ldk 0.4.1 Update to LDK 0.4.2 Nov 2, 2024
@rolznz rolznz marked this pull request as ready for review November 4, 2024 17:42
@rolznz rolznz merged commit 7cb44ce into main Nov 4, 2024
14 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to 0.4.0
10 participants