Skip to content

Add session_id accessors to Protocol and async Protocol - #167

Merged
nyonson merged 1 commit into
rust-bitcoin:mainfrom
YoganshSharma:feat/expose-session_id
Jun 24, 2026
Merged

Add session_id accessors to Protocol and async Protocol#167
nyonson merged 1 commit into
rust-bitcoin:mainfrom
YoganshSharma:feat/expose-session_id

Conversation

@YoganshSharma

@YoganshSharma YoganshSharma commented May 17, 2026

Copy link
Copy Markdown
Contributor

Add Protocol::session_id() to sync and async Protocol
Update round trip tests to verify both peers derive the same session ID
Fixes #61

@YoganshSharma
YoganshSharma force-pushed the feat/expose-session_id branch 2 times, most recently from fe565f5 to 059bf60 Compare May 28, 2026 06:45
@rustaceanrob

Copy link
Copy Markdown
Member

#61 has already been resolved, closing

@YoganshSharma

YoganshSharma commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

#61 has already been resolved, closing

Following up on this as @rustaceanrob previously mentioned this contribution was welcome
A downstream project relies heavily on this feature.
As outlined in the BIP-324 specification the session ID is specifically designed to facilitate channel binding and detecting MitM attacks, which is exactly why we need access to it. Without a getter on high level io::Protocol, we are forced to manually wrap or reimplement the lower level Handshake, which introduces unnecessary risk and potential for bugs.
This is a minor, read-only addition. If you are open to it, could we reopen this (or a modified version) to accommodate this use case?

@rustaceanrob

Copy link
Copy Markdown
Member

Oh I see, it's not available on Protocol. I think we should add a newtype for this (SessionId([u8; 32]) since this will be a breaking change anyway. Otherwise this makes sense.

@rustaceanrob rustaceanrob reopened this Jun 19, 2026
@YoganshSharma
YoganshSharma force-pushed the feat/expose-session_id branch 2 times, most recently from cab79f9 to 24015b9 Compare June 20, 2026 03:28
Comment thread protocol/src/lib.rs

impl SessionId {
/// Returns the inner 32-byte array.
pub fn as_bytes(&self) -> &[u8; 32] {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[u8; 32] is Copy I think it makes more sense to return an owned [u8; 32]

@YoganshSharma YoganshSharma Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since [u8; 32] is Copy, callers can always do *session_id.as_bytes() when they need an owned array. This avoids unnecessary copies and is convention (I looked at similar usage of as_bytes in bitcoin crate ). To return owned data conventionally to_bytes is used which consumes self.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

At each callsite you've de-referenced it for use, which is some indication to me at least that callers will almost always want an owned version. In the case of primitives like u64, copying is almost always faster than a borrow. I can't imagine this copy would be very costly either. @nyonson do you have any opinion on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can add to_bytes to return owned [u8; 32] similar to bip32::ChainCode ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like where you have landed in 7804349, lines up with the C-CONV rule in the rust api guidelines which I wanna follow unless we have a real good reason not to.

@YoganshSharma
YoganshSharma force-pushed the feat/expose-session_id branch 3 times, most recently from ce49a25 to 7804349 Compare June 20, 2026 14:29
Comment thread protocol/src/lib.rs
- also modify test to verify matching BIP324 session IDs
@YoganshSharma
YoganshSharma force-pushed the feat/expose-session_id branch from 7804349 to f874bc9 Compare June 20, 2026 19:55

@rustaceanrob rustaceanrob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK f874bc9

@rustaceanrob

Copy link
Copy Markdown
Member

cc @nyonson for review and merge

@nyonson

nyonson commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

ACK f874bc9

@nyonson
nyonson merged commit d229102 into rust-bitcoin:main Jun 24, 2026
21 of 24 checks passed
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.

Allow retrieving session ID

3 participants