fedimint / fedimint

Federated E-Cash Mint
https://fedimint.org/
MIT License
536 stars 209 forks source link

chore: upgrade to ldk-node 0.2.1 #4354

Closed tvolk131 closed 2 months ago

tvolk131 commented 3 months ago

Requires #4086 since we're using bitcoin30_to_bitcoin29_public_key() which is added there. Would be simple enough to circumvent this requirement but probably not worth the time.

Edit: #4086 is submitted

This cannot be merged because it adds openssl as a dependency. This is a dependency of vss-client which is added as a dependency to ldk-node in 0.2.0.

Perhaps we should make a branch in our fork of ldk node which disables vss and use that instead of the official crate. Also see this PR in the ldk-node codebase where they addressed a problem similar to what is mentioned above.

Edit: See the discussion below on how we removed openssl

m1sterc001guy commented 2 months ago

Is it possible to use the cfg flag that was introduced in ldk-node to disable vss? The whole point of upgrading for us is to eliminate the dependency on our forked repo

elsirion commented 2 months ago

Opened https://github.com/lightningdevkit/ldk-node/issues/254 in ldk-node

tnull commented 2 months ago

@tvolk131 Could you kick CI to run the lint again? The just-released vss-client 0.2.2 should have fixed the issue, i.e., openssl isn't mentioned in Cargo.lock anymore.

tvolk131 commented 2 months ago

Thanks @elsirion and @tnull for ironing this out! Just rebased and bumped the version of vss-client. This PR is now just waiting on #4086, since we're using one of the helper functions added there (bitcoin30_to_bitcoin29_public_key()). If we don't expect that PR to be merged soon, I can add it separately in this PR so we can get this merged.

tvolk131 commented 2 months ago

4086 is merged! This PR should be safe to submit.

elsirion commented 2 months ago

I don't think we need to rebase, the updated CI should be used in the merge queue.

bradleystachurski commented 2 months ago

I don't think we need to rebase, the updated CI should be used in the merge queue.

Ah yep, good call 👍

We should remove the #[ignore] in test_gateway_can_pay_ldk_node https://github.com/fedimint/fedimint/pull/4388/files#diff-b1fe1eec690ca593c767be467f8b226e8335b51733d649b91e2fa23e4ba774d5R204

@tvolk131 I heard you have a busy week with limited availability. @elsirion do you know if there's a way to add a commit to this PR that removes the #[ignore]? If not, I can open a separate PR that keeps Tommy's commits along with a commit that removes the ignore.

EDIT: I'm running this PR plus the test_gateway_can_pay_ldk_node test in https://github.com/fedimint/fedimint/pull/4342. If that passes, we can merge then do a quick follow to remove the ignore on master.

elsirion commented 2 months ago

~@bradleystachurski I don't see the test being ignored on @tvolk131's branch, was that introduced on master in the meantime? Then I'd say let's do it in a follow-up.~ Never mind :laughing:

tvolk131 commented 2 months ago

I just rebased and removed the ignore!

elsirion commented 2 months ago

You may need 3c2491f6a9456575d11cbb10b9e6e406476c46a9, seems like @bradleystachurski ran into a timeout.

bradleystachurski commented 2 months ago

I just rebased and removed the ignore!

Beautiful, thanks!

You may need 3c2491f, seems like @bradleystachurski ran into a timeout.

Yep, I can tackle running test_gateway_can_pay_ldk_node in parallel in a followup to keep CI running quickly.

tvolk131 commented 2 months ago

You may need 3c2491f, seems like @bradleystachurski ran into a timeout.

Sounds good

@tvolk131 I heard you have a busy week with limited availability.

Yep I do, but I have time for a quick rebase here and there 🙂

bradleystachurski commented 2 months ago

The other PR just reported a failure on the 5x run

140477   │ 2024-02-23T16:31:08.9430756Z fedimint-test-all-ci> 00:03:39 thread 'test_gateway_can_pay_ldk_node' panicked at gateway/ln-gateway/tests/integration_tests.rs:187:13:
140478   │ 2024-02-23T16:31:08.9434079Z fedimint-test-all-ci> 00:03:39 assertion failed: `Canceled { error: OutgoingPaymentError { error_type: LightningPayError { lightning_error
        │ : FailedPayment { failure_reason: "FailureReasonNoRoute" } }, contract_id: 3020f715ce95408aac4b10e349315c8f70d1d418b81e183b8773e9d0de80da70, contract: Some(OutgoingCon
        │ tractAccount { amount: Amount { msats: 250000 }, contract: OutgoingContract { hash: 41f3bc42f70b0153733ea5a7efbafb301cb5ae5ac5b10b219c688fe5f96b494d, gateway_key: Publ
        │ icKey(68d41ca743e8eef78d3b3ed00fb986240f940ec173855d6f91d190d005df508c30fb78bb54ca76bbeaed5fe13a91e8b436dc59aec96f84335b728d2217d1ef6e), timelock: 629, user_key: Publi
        │ cKey(a996b990fb243997852c734b0f0581ed05ad8be65f39a28ef6d6c6bd38bc177305732f0bed779b10deb6195d04c41f4ead07e1ccf1cd300b5963303f60361401), cancelled: false } }) } }` does
        │  not match `GatewayExtPayStates::Preimage { .. }`
140561   │ 2024-02-23T16:31:08.9507414Z fedimint-test-all-ci> 00:04:24      Summary [ 254.900s] 10/22 tests run: 9 passed, 1 failed, 0 skipped
140562   │ 2024-02-23T16:31:08.9508095Z fedimint-test-all-ci> 00:04:24         FAIL [ 162.878s] fedimint-ln-gateway::gatewayd-integration-tests test_gateway_can_pay_ldk_node
m1sterc001guy commented 2 months ago

On second thought, I'd prefer to just work towards supporting ldk-node in the gateway, rather than also supporting it in fedimint-testing. I opened a PR to remove it https://github.com/fedimint/fedimint/pull/4521

If we're cool with that, I think we can close this.

m1sterc001guy commented 2 months ago

Closing in favor of https://github.com/fedimint/fedimint/pull/4521