Skip to content

Add a std IO protocol interface #125

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

Merged
merged 3 commits into from
Jul 8, 2025
Merged

Conversation

nyonson
Copy link
Collaborator

@nyonson nyonson commented Jul 4, 2025

Add a std library I/O (aka blocking) high level Protocol interface. Also experiments with some new API refactors which could be good for the existing async version.

@nyonson nyonson force-pushed the std-io branch 4 times, most recently from 43bd048 to cac6368 Compare July 7, 2025 19:32
@nyonson nyonson marked this pull request as ready for review July 7, 2025 19:42
pub fn packet_type(&self) -> PacketType {
self.packet_type
PacketType::from_byte(&self.data[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Both of these will panic for an empty payload. Not sure if that is possible or not, but pointing it out for reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops good point, shoulda thought more about this change.

@rustaceanrob
Copy link
Collaborator

Overall looks good and I like the new flow. Would just have to think for a while if someone could somehow send an empty payload contents, in which case contents would panic.

@nyonson
Copy link
Collaborator Author

nyonson commented Jul 8, 2025

Would just have to think for a while if someone could somehow send an empty payload contents, in which case contents would panic.

I think it would be difficult to not send a header byte, but still feels like this should be more defensive.

@rustaceanrob
Copy link
Collaborator

rustaceanrob commented Jul 8, 2025

Could someone just send [0, 0, 0] as the length field? I need to actually re-read the spec but that was my gut reaction

@nyonson
Copy link
Collaborator Author

nyonson commented Jul 8, 2025

Could someone just send [0, 0, 0] as the length field? I need to actually re-read the spec but that was my gut reaction

I think that is against the spec because a header byte and a 16 byte tag are required. But a bug in a client or a bad actor could send it anyways. Right now, that case specifcally is handled by this impl in the low level decrypt function with a CiphertextTooSmall error, we check that at least a tag (16 bytes) is in the ciphertext. But maybe that should be bumped to 17 since there must also be a header byte to tell us if it is a decoy or not.

Packet payloads should always include a header byte according to the
spec, but check that it actually does to better handle buggy clients
or bad actors.
@rustaceanrob
Copy link
Collaborator

Interesting we didn't run into this scenario before. I think everything checks out though. All cases abuse should now lead to an EOF if I'm understanding correctly.

@nyonson nyonson merged commit f470fb3 into rust-bitcoin:main Jul 8, 2025
19 of 20 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.

2 participants