Skip to content

Inbound user_channel_id randomization follow-up #1855

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Nov 16, 2022

This is a follow-up to #1790, which addresses two issues:

  1. While the randomized user_channel_id was indeed set in Channel::new_from_req, it was actually overridden with a 0 value just a few lines below. In the first commit, we address this oversight.
  2. We now mention as of which version the user can expect the user_channel_id to be randomized, addressing Randomize user_channel_id for inbound channels #1790 (comment).

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Base: 90.68% // Head: 91.51% // Increases project coverage by +0.82% 🎉

Coverage data is based on head (7f6713c) compared to base (8d8ee55).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1855      +/-   ##
==========================================
+ Coverage   90.68%   91.51%   +0.82%     
==========================================
  Files          89       90       +1     
  Lines       47947    53434    +5487     
  Branches    47947    53434    +5487     
==========================================
+ Hits        43481    48900    +5419     
- Misses       4466     4534      +68     
Impacted Files Coverage Δ
lightning/src/onion_message/functional_tests.rs 95.83% <ø> (ø)
lightning/src/util/events.rs 25.25% <ø> (ø)
lightning/src/ln/channelmanager.rs 87.62% <100.00%> (+2.55%) ⬆️
lightning/src/ln/functional_test_utils.rs 95.96% <100.00%> (+2.49%) ⬆️
lightning-net-tokio/src/lib.rs 76.73% <0.00%> (-0.31%) ⬇️
lightning/src/ln/reorg_tests.rs 100.00% <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%) ⬆️
lightning/src/ln/payment_tests.rs 99.34% <0.00%> (+0.45%) ⬆️
lightning/src/ln/onion_route_tests.rs 98.35% <0.00%> (+0.80%) ⬆️
... and 7 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.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

I checked that we always set inbound channel user_ids to a random value by asserting that it's non-0 in channel.funding_created and channel.funding_signed and ensuring tests pass

@tnull
Copy link
Contributor Author

tnull commented Nov 16, 2022

I checked that we always set inbound channel user_ids to a random value by asserting that it's non-0 in channel.funding_created and channel.funding_signed and ensuring tests pass

Yeah, I'm a little unsure whether to include a regression test checking for non-0 as it technically is indeterministic. Then again, it's veeery unlikely we'd ever hit the 0 in a u128. Any preferences?

@valentinewallace
Copy link
Contributor

No strong preference. I'm fine rolling the dice on that, though, if we think a regression test would be good.

@tnull tnull force-pushed the 2022-11-inbound-user-channel-id-randomization-fixup branch from 60d0a10 to bd281dd Compare November 16, 2022 17:37
@tnull
Copy link
Contributor Author

tnull commented Nov 16, 2022

No strong preference. I'm fine rolling the dice on that, though, if we think a regression test would be good.

Yeah, after having it written out it seems kinda obvious that it's fine to take the chance 😅
Added some asserts with cdb6c3b.

@TheBlueMatt
Copy link
Collaborator

Grrr, good catch. Feel free to squash.

As it was previously omitted, we clarify here starting from which version users can expect the `user_channel_id` to be randomized for inbound channels.
@tnull tnull force-pushed the 2022-11-inbound-user-channel-id-randomization-fixup branch from bd281dd to 7f6713c Compare November 16, 2022 17:50
@tnull
Copy link
Contributor Author

tnull commented Nov 16, 2022

Grrr, good catch. Feel free to squash.

Squashed without further changes.

@TheBlueMatt TheBlueMatt merged commit 7269fa2 into lightningdevkit:main Nov 16, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants