JssDWt commented 2 months ago

cln-grpc: add anchors/even to primitives

Description

The anchors/even channel type was missing from the grpc bindings. It is now the default channel type, so fundchannel over grpc fails with:

Failed to deserialize response : unknown variant `anchors/even`,
expected one of `static_remotekey/even`, `anchor_outputs/even`,
`anchors_zero_fee_htlc_tx/even`, `scid_alias/even`, `zeroconf/even`

This commit adds the anchors/even channel type to the grpc bindings.

Related Issues

Closes #7627

Changes Made

[ ] Feature: Brief description of the new feature or functionality added.
[x] Bug Fix: Brief description of the bug fixed and how it was resolved.
[ ] Refactor: Any code improvements or refactoring done without changing the functionality.

Checklist

Ensure the following tasks are completed before submitting the PR:

[x] Changelog has been added in relevant commit/s.
[x] Tests have been added or updated to cover the changes.
[x] Documentation has been updated as needed.
[x] Any relevant comments or TODOs have been addressed or removed.

Additional Notes

Any additional information or context about this PR that reviewers should know.

JssDWt commented 2 months ago

Note that I'm unsure whether the integer to enum conversion is only used in the transport over grpc. If it's also used in the transport with cln, the numbering may be wrong.

cdecker commented 2 months ago

Note that I'm unsure whether the integer to enum conversion is only used in the transport over grpc. If it's also used in the transport with cln, the numbering may be wrong.

No worries, these enums are only used in the grpc protocol, the json-rpc sees the stringified enum variants.

JssDWt commented 2 months ago

I don't think the CI failure is due to this change.

ErikDeSmedt commented 2 months ago

Thanks, this is a great PR.

I've been hit by this issue as well. It prevented me from opening a channel using grpc after upgrading.

called `Result::unwrap()` on an `Err` value: Status { code: Unknown, message: "Error calling method FundChannel: RpcError { code: None, message: \"Failed to deserialize response : unknown variant `anchors/even`, expected one of `static_remotekey/even`, `anchor_outputs/even`, `anchors_zero_fee_htlc_tx/even`, `scid_alias/even`, `zeroconf/even`\", data: None }", metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Mon, 02 Sep 2024 03:40:12 GMT", "content-length": "0"} }, source: None }

@ShahanaFarooqui : Could you please consider making a point-release?

JssDWt commented 2 months ago

@ShahanaFarooqui : Could you please consider making a point-release?

I agree!

JssDWt commented 2 months ago

Added changelog

cdecker commented 2 months ago

The release freeze will end in the coming days, and then we can start merging again. We are considering a late point release to get some of the features in that didn't quite make it, but that'd be in a couple of weeks, once these changes have settled a bit.

JssDWt commented 2 months ago

@ShahanaFarooqui Can this be added to the v24.08.1 milestone?

ShahanaFarooqui commented 1 month ago

Hi @JssDWt & @ErikDeSmedt, I apologize for missing your messages. Unfortunately, I haven’t been receiving any notifications from GitHub, even when tagged directly. I've been trying to resolve this issue for the past few months without success. In the meantime, if I don’t respond here, please feel free to reach out to me on Telegram, Discord, or BuildonL2.

kingonly commented 1 month ago

@ShahanaFarooqui so we missed 24.8.1 because you didn't receive the GitHub notification?

This is a very important fix for us (Breez), can we at least merge for now?

ShahanaFarooqui commented 1 month ago

@ShahanaFarooqui so we missed 24.8.1 because you didn't receive the GitHub notification?

I usually inform everyone that I am currently unable to receive notifications to avoid future confusion.

However, this does not affect PR review and merging process. I regularly monitor the PR list and ensure that all approved and CI-passing PRs are merged.

This is a very important fix for us (Breez), can we at least merge for now?

Regarding merging them now:

7611: Currently failing in the CI but might be replaced with #7679

7628: CI is failing.

7679: CI is failing.

7648: It has been approved and passed the CI too but it is based on #7494 which has not been merged yet.

Please note that we are already working towards merging them as soon as possible.

JssDWt commented 1 month ago

7611: Currently failing in the CI but might be replaced with #7679

7628: CI is failing.

7679: CI is failing.

7648: It has been approved and passed the CI too but it is based on #7494 which has not been merged yet.

@ShahanaFarooqui The CI failure for this PR is not due to the PR itself. Can it be rerun please?

ShahanaFarooqui commented 1 month ago

@ShahanaFarooqui The CI failure for this PR is not due to the PR itself. Can it be rerun please?

@JssDWt Rebased the PR on master to trigger the CI because rerun also did not finish the CI.

ElementsProject / lightning