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.

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

Clarify onions part 2: a bit deeper rework #1182

Merged
merged 8 commits into from
Aug 13, 2024
Merged

Clarify onions part 2: a bit deeper rework #1182

merged 8 commits into from
Aug 13, 2024

Conversation

rustyrussell
Copy link
Collaborator

This is based on #1181 (first four commits), and replaces #1179 based on feedback there.

You're going to want to review one commit at a time: I rewrote it to be managable to review.

  1. Move Route Blinding Section up a level, put blinded_path type there
    • It's not just for onions any more
  2. Update path construction requirements to explicitly refer to blinded_path elements
    • It's now an actual type, so we can name fields.
  3. Requirements for using a blinded path (esp. payments where we need to route non-blinded to the introduction node) as well as forwarding them.
  4. Attempt to merge @t-bast and my high-level description
  5. Add dummy hops as a MAY in requirements

Basically, nothing changes, but hopefully it will be more digestable in future!

t-bast
t-bast reviewed Jul 11, 2024
View reviewed changes
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Thanks, this is now much better! The individual commits are easy to follow and all make sense, and the end result looks good to me!

04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
04-onion-routing.md Outdated Show resolved Hide resolved
@t-bast t-bast mentioned this pull request Jul 12, 2024
Closed
22 tasks
@rustyrussell rustyrussell mentioned this pull request Jul 17, 2024
Merged
@rustyrussell
BOLT 4: move Route Blinding section up a level, and put `blinded_path…
14ebb44 
…` type there.

It's currently buried in the onion message section, but it applies to payments too.

We now have a separate sub-section for the encrypted_data_tlv definition.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: concretely refer to the blinded_path type and field when co…
6c0f0d8 
…nstructing.

This spec was initially written before the `blinded_path` type
existed.  Be precise (and we no longer need to say "MUST communicate"!).

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: clarify blinded path requirements.
9c15a5c 
Writer parts:
1. Be explicit that the writer creates a route.
2. Make it clear we create shared secrets, then derive blinded points.
3. Refer explicitly to all `blinded_path` fields.

Split reader into the *two* readers:
1. The reader of the blinded path, who uses it to make an onion (which wasn't described at all!)
2. The reader of the encrypted_recipient_data, who decrypts it.

In the latter case, we don't have to discuss unblinding the onion since
that's now covered in the "Onion Decryption" section.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: try to improve blinded path description.
d91642e 
It's a bit complex, but try to convey the idea of an introduction point,
blinded node ids and encrypted blobs.  Since the requirements detail the
two ways to reach the introduction node, I handwaved on that a bit.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: put dummy hop recommendation into the requirements.
43725d7 
It was mentioned in the rationale only.

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: rename onionmsg_hop to blinded_path_hop
6654c57 
It's used for blinded payment paths, too, not just onion messages.

Suggested-by: @t-bast
Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
Copy link
Collaborator Author

OK:

  1. rebased on master (now Clarify onion spec: part 1 (the uncontroversial bits) #1181 is applied)
  2. Applied @t-bast typo fixes
  3. Renamed onionmsg_hop to blinded_path_hop as per @t-bast recommendation

t-bast
t-bast reviewed Jul 17, 2024
View reviewed changes
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good! Can you also update route-blinding-test.json? It contains fields called ephemeral_pubkey and ephemeral_privkey, which is confusing and should be replaced by path_key and path_privkey.

04-onion-routing.md Outdated Show resolved Hide resolved
@t-bast t-bast mentioned this pull request Jul 23, 2024
Closed
23 tasks
gijswijs
gijswijs reviewed Jul 26, 2024
View reviewed changes
04-onion-routing.md Outdated Show resolved Hide resolved
@rustyrussell
BOLT 4: More clarifying changes.
3c0fd9a 
1. Always use the term `encrypted_recipient_data` for the encrypted field:
   it's confusing enough without multiple names!
2. Don't give an option for joining blinded payments, since everyone will
   use an unblinded payment to the introduction node (at least, for now).
3. Avoid the term "payer" and at least note that encrypted_recipient_data
   can be made by the sender themselves, pointing out that the forwarding
   node cannot tell.

Thanks t-bast and gijswijs for this feedback!

Signed-off-by: Rusty Russell <[email protected]>
@rustyrussell
BOLT 4: fix nomenclature in bolt04/route-blinding-test.json
0aae209 
Bastien points out we didn't update this when we changed terms.

Signed-off-by: Rusty Russell <[email protected]>
t-bast
t-bast approved these changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK 0aae209

@t-bast
Copy link
Collaborator

t-bast commented Aug 7, 2024

@Roasbeef @arik-so @jkczyz could you please take a look at this? It would be nice to land it before the next spec meeting.

@valentinewallace valentinewallace mentioned this pull request Aug 7, 2024
Merged
1 task
valentinewallace
valentinewallace approved these changes Aug 7, 2024
View reviewed changes
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.

Reads a lot more clearly to me! The new section on path concatenation clarifies a lot. A good amount of the additions to BOLT 4 seem a bit redundant to what's in proposals/route_blinding.me but that seems ok.

@t-bast t-bast mentioned this pull request Aug 9, 2024
Closed
21 tasks
@t-bast
Copy link
Collaborator

t-bast commented Aug 13, 2024

@Roasbeef can you take a quick look at this PR? As discussed during yesterday's spec meeting, we should merge this once we have your ACK :)

Roasbeef
Roasbeef approved these changes Aug 13, 2024
View reviewed changes
Copy link
Collaborator

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🎫

04-onion-routing.md

The writer of a `blinded_path`:

- MUST create a viable path to itself ($`N_r`$) i.e. $`N_0 \rightarrow N_1 \rightarrow ... \rightarrow N_r`$.
Copy link
Collaborator

Choose a reason for hiding this comment

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

😎

@Roasbeef Roasbeef merged commit 22b9c87 into lightning:master Aug 13, 2024
@t-bast t-bast mentioned this pull request Sep 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gijswijs gijswijs gijswijs left review comments

@Roasbeef Roasbeef Roasbeef approved these changes

@valentinewallace valentinewallace valentinewallace approved these changes

@t-bast t-bast t-bast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rustyrussell @t-bast @Roasbeef @gijswijs @valentinewallace