Skip to content

Chan_utils helpers visibility relaxation #1839

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 30, 2022
Merged

Chan_utils helpers visibility relaxation #1839

merged 3 commits into from
Nov 30, 2022

Conversation

@ariard
Copy link

@ariard ariard commented Nov 8, 2022

See: https://gitlab.com/lightning-signer/validating-lightning-signer/-/blob/main/lightning-signer-core/src/tx/script.rs#L76

Allowing upstream projects to reuse our BOLT-compliant helpers reduce for them the risk of incompatibility or non-propagating transactions bugs.

@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2022 •
edited
Loading

Codecov Report

Base: 90.77% // Head: 91.99% // Increases project coverage by +1.21% 🎉

Coverage data is based on head (9978e1c) compared to base (505102d).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 9978e1c differs from pull request most recent head 605d30e. Consider uploading reports for the commit 605d30e to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1839      +/-   ##
==========================================
+ Coverage   90.77%   91.99%   +1.21%     
==========================================
  Files          87       90       +3     
  Lines       47595    59062   +11467     
  Branches    47595    59062   +11467     
==========================================
+ Hits        43204    54332   +11128     
- Misses       4391     4730     +339
Impacted Files Coverage Δ
lightning/src/ln/chan_utils.rs 93.97% <100.00%> (-0.02%) ⬇️
lightning/src/util/events.rs 34.91% <0.00%> (-2.72%) ⬇️
lightning/src/chain/mod.rs 66.66% <0.00%> (-1.52%) ⬇️
lightning/src/lib.rs 100.00% <0.00%> (ø)
lightning/src/ln/reorg_tests.rs 100.00% <0.00%> (ø)
lightning/src/util/string.rs 100.00% <0.00%> (ø)
lightning/src/offers/offer.rs 94.56% <0.00%> (ø)
lightning/src/ln/reload_tests.rs 95.24% <0.00%> (ø)
lightning/src/ln/chanmon_update_fail_tests.rs 97.72% <0.00%> (+0.05%) ⬆️
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TheBlueMatt
TheBlueMatt reviewed Nov 8, 2022
View reviewed changes
wpaulino
wpaulino reviewed Nov 8, 2022
View reviewed changes
Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

What about the other TODOs like ANCHOR_OUTPUT_VALUE_SATOSHI and get_delayed_redeemscript?

@ariard
Copy link
Author

ariard commented Nov 10, 2022

Updated at b57e19f

The last push increase the visibility of few other const encapsulating protocol-level values.

What about the other TODOs like ANCHOR_OUTPUT_VALUE_SATOSHI and get_delayed_redeemscript?

I think our ANCHOR_OUTPUT_VALUE_SATOSHI is already public and get_delayed_redeeemscript is logically equivalent to get_to_countersignatory_with_anchors_redeemscript(). The further cleanup should be on VLS side.

@wpaulino
Copy link
Contributor

wpaulino commented Nov 10, 2022

CI is failing now since the now public consts need docs.

@ariard
Copy link
Author

ariard commented Nov 11, 2022

Hope CI should be happy at 264bf5f

TheBlueMatt
TheBlueMatt reviewed Nov 11, 2022
View reviewed changes
lightning/src/ln/msgs.rs Outdated

/// 21 million * 10^8 * 1000
pub(crate) const MAX_VALUE_MSAT: u64 = 21_000_000_0000_0000_000;
pub const MAX_VALUE_MSAT: u64 = 21_000_000_0000_0000_000;
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Nov 11, 2022

Choose a reason for hiding this comment

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

This also belongs in rust-bitcoin, not here.

Copy link
Author

@ariard ariard Nov 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is though currently it's requiring a useless Network argument, sounds a legacy from multi-chain support of the library. I think downstream should be changed first to drop the argument: https://github.com/rust-bitcoin/rust-bitcoin/blob/1d0b0e6ed8e80767fef9b00a38c9e5fefc34baa0/bitcoin/src/blockdata/constants.rs#L63

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Nov 15, 2022

Choose a reason for hiding this comment

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

Sure, but either way we shouldn't be exporting fundamental Bitcoin constants from rust-lightning.

Copy link
Author

@ariard ariard Nov 16, 2022

Choose a reason for hiding this comment

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

Dropping the export for now and seeing with downstream to remove the argument.

@ariard
Copy link
Author

ariard commented Nov 12, 2022

Letting it at 264bf5f, if we would like to change downstream first or take the method as it is.

@ariard
Copy link
Author

ariard commented Nov 16, 2022

Updated at 9978e1c. PR is pending on this unsolved comment: #1839 (comment)

apoelstra added a commit to rust-bitcoin/rust-bitcoin that referenced this pull request Nov 19, 2022
@apoelstra
Merge #1399: Drop Network arg from max_money()
1696038 
64495cc Drop Network arg from max_money() (Antoine Riard)

Pull request description:

  Amount of coins available stay in the same across Bitcoin network: signet, testnet, mainet. From my understanding this is a leftover from some potential multi-chain support.

  For more context: lightningdevkit/rust-lightning#1839 (comment)

  If there is already an existent PR, it can be closed, however didn't find one.

ACKs for top commit:
  apoelstra:
    ACK 64495cc
  tcharding:
    ACK 64495cc

Tree-SHA512: 929011ee73c5eda903fb0140438ed5e88c8f5b7378036a87a6a660a8b9138bf204bf59a0ba822c0cd98e37e97d2d0dbbf8c9893a834da9acdf817ba43a5ed5b6
@ariard
Copy link
Author

ariard commented Nov 22, 2022

Updated at 6d61555

wpaulino
wpaulino reviewed Nov 23, 2022
View reviewed changes
@ariard
Copy link
Author

ariard commented Nov 23, 2022

Updated at 0c614b7

wpaulino
wpaulino previously approved these changes Nov 24, 2022
View reviewed changes
TheBlueMatt
TheBlueMatt reviewed Nov 24, 2022
View reviewed changes
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Some docs are confusing but LGTM.

@ariard
Copy link
Author

ariard commented Nov 30, 2022

Updated at 605d30e.

@TheBlueMatt TheBlueMatt merged commit 2f0ddf0 into lightningdevkit:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TheBlueMatt TheBlueMatt TheBlueMatt approved these changes

@wpaulino wpaulino wpaulino approved these changes

Assignees

@wpaulino wpaulino

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ariard @codecov-commenter @wpaulino @TheBlueMatt