Conversation
Summary of ChangesHello @darioAnongba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on upgrading the project's core Lightning Network Daemon (LND) dependency to its latest release candidate. This update is crucial for maintaining compatibility and leveraging the newest features and fixes from LND. The changes also include necessary adjustments to integration tests to ensure that existing functionalities remain stable and correctly interact with the updated LND version, particularly concerning channel policy parameters and the handling of channel closure events. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request bumps the LND dependency to its latest version and adjusts several integration tests to align with the changes. The modifications primarily involve updating TimeLockDelta values in tests and refining how closed channels are asserted to handle asynchronous processing. While most changes appear correct and necessary for compatibility, I've identified a potential issue in one of the firewall tests where the test logic seems to be broken by an updated value. My review provides a suggestion to fix this.
44802eb to
70ff1fc
Compare
go.mod
Outdated
| github.com/lightninglabs/taproot-assets v0.7.0 | ||
| github.com/lightninglabs/taproot-assets/taprpc v1.0.11 | ||
| github.com/lightningnetwork/lnd v0.20.0-beta | ||
| github.com/lightningnetwork/lnd v0.20.1-beta.rc1 |
There was a problem hiding this comment.
why only 20.1 tho, I would prefer referencing the current master commit ?
There was a problem hiding this comment.
me as well but that is forbidden by LiT policy apparently. It makes sense as referencing random commits that are not stable releases (or at least RCs) is not a great idea, but if tests pass then it should be okay.
cc: @ViktorT-11
On a side note, LiT doesn't yet support 0.20.1, that is not even released yet I guess the first step is to align LiT to 0.20.1 and then to 0.21.0 ? or is 0.20.1 just a patch?
There was a problem hiding this comment.
20.1 is just a patch so we go first 20.1 and then master commit LND, but then you also need to update your comments you made during this PR, because you were referencing 21
There was a problem hiding this comment.
Yes, we should preferebly not bump to an untagged version, so I'm in favour of using v0.20.1-beta.rc1 here.
The reason being is that if we need make hotfix release, which for example #1215 could motivate (as that solves issues for the AutoFees algo), we wouldn't be blocked by having an unreleased version of lnd merged to master.
By referencing v0.20.1-beta.rc1, we could instead make a litd RC release which ships with lnd v0.20.1-beta.rc1.
There was a problem hiding this comment.
this PR does target 0.21, I can have an other one for 0.20.1 but this one can stay open in draft until it becomes relevant (when we have our first 0.21 RC tag?). It is used for testing before LND officially releases.
See this PR as example: lightninglabs/taproot-assets#1958
There was a problem hiding this comment.
On a side note, LiT doesn't yet support
0.20.1
litd master now compiles with lightningnetwork/lnd#10556 if I manually replace it
There was a problem hiding this comment.
Sorry I meant doesn't support as it's not the targeted version in go.mod on master. As per @ziggie1984 comment, because it's only a patch, we can skip that version and go straight to 0.21 with this PR ( that will stay open waiting on the RC1 tag).
Bump lnd to v0.21.0, sqldb to v1.0.13.
Upgrading LND to 0.21.0 introduces a race condition in the assets_test. This race is introduced by changes in the chancloser. The channel may not be market as fully closed immediately after the close transaction is confirmed.
LND 0.21.0 introduces a new limit on the timelock delta of 24. Previous limit was 18 so a change is required to bump this limit in all tests to avoid errors.
70ff1fc to
be47c0b
Compare
7ca3d67 to
7817c56
Compare
7817c56 to
42ad4b0
Compare
|
ok the PR is now green targeting latest LND and TAPD. No need to approve but would be nice to get your thoughts on it. We can reference this branch anywhere we need latest LND (in taproot-assets or in tests, etc). |
Bump of LND to latest version requires:
assets_test. This race is introduced by changes in the chancloser. The channel may not be market as fully closed immediately after the close transaction is confirmed.24. Previous limit was 18 so a change is required to bump this limit in all tests to avoid errors.sqldbtaproot-assetsandtaprpc