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

cln-grpc: add anchors/even to primitives #7628

Merged

Conversation

JssDWt
Copy link
Contributor

@JssDWt JssDWt commented Aug 30, 2024

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

Changes Made

  • Feature: Brief description of the new feature or functionality added.
  • 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:

  • Changelog has been added in relevant commit/s.
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated as needed.
  • 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
Copy link
Contributor Author

JssDWt commented Aug 30, 2024

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.

@JssDWt JssDWt marked this pull request as ready for review August 30, 2024 10:48
@JssDWt JssDWt requested a review from cdecker as a code owner August 30, 2024 10:48
Copy link
Member

@cdecker cdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I guess this is the downside with he manually maintained enums in primitives, but it makes for a much nicer API. I wish we'd have used a protobuf-like schema description, rather than the nested structure of json-schema.

@cdecker
Copy link
Member

cdecker commented Aug 30, 2024

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
Copy link
Contributor Author

JssDWt commented Sep 1, 2024

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

@ErikDeSmedt
Copy link
Contributor

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
Copy link
Contributor Author

JssDWt commented Sep 2, 2024

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

I agree!

@JssDWt
Copy link
Contributor Author

JssDWt commented Sep 3, 2024

Added changelog

@cdecker
Copy link
Member

cdecker commented Sep 5, 2024

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
Copy link
Contributor Author

JssDWt commented Sep 7, 2024

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

@cdecker cdecker added this to the v24.08.1 milestone Sep 18, 2024
@ShahanaFarooqui
Copy link
Collaborator

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.

@ShahanaFarooqui ShahanaFarooqui modified the milestones: v24.08.1, v24.08.2 Sep 20, 2024
@kingonly
Copy link

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

@JssDWt JssDWt mentioned this pull request Sep 20, 2024
4 tasks
@ShahanaFarooqui
Copy link
Collaborator

@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
Copy link
Contributor Author

JssDWt commented Sep 20, 2024

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

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`
```

Changelog-Changed: Channel type `anchors/even` was added
to the grpc bindings.
@ShahanaFarooqui
Copy link
Collaborator

@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.

@ShahanaFarooqui ShahanaFarooqui merged commit 2798b4b into ElementsProject:master Sep 21, 2024
38 checks passed
@JssDWt JssDWt mentioned this pull request Sep 21, 2024
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.

anchors/even is now default channel type, but not represented in grpc
5 participants