-
Notifications
You must be signed in to change notification settings - Fork 407
Explicit Message Padding for OnionRealm0HopData #469
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
Explicit Message Padding for OnionRealm0HopData #469
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick turnaround! Looks good.
@@ -610,7 +611,7 @@ pub(crate) struct OnionRealm0HopData { | |||
pub(crate) short_channel_id: u64, | |||
pub(crate) amt_to_forward: u64, | |||
pub(crate) outgoing_cltv_value: u32, | |||
// 12 bytes of 0-padding | |||
pub(crate) padding: PhantomData<[u8; 12]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too bad RFC 2000 (rust-lang/rust#44580) isn't in the stable Rust release yet. Then we could type alias this as:
pub(crate) type BytePadding<const N: usize> = ::std::marker::PhantomData<[u8; N]>;
So this should be fine for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do something like this anyway without generic constants. PhantomData has a somewhat specific meaning in Rust (that we have a template type that we're not going to instantiate). We probably just want a new type in ser.rs like (but with better names):
pub(crate) struct PaddingData<T: Default + std::convert::AsRef<[u8]>> {
t: std::marker::PhantomData<T>,
}
impl<T: Default + std::convert::AsRef<[u8]>> Writeable for PaddingData<T> {
#[inline]
fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
let nt: T = Default::default();
w.write_all(nt.as_ref())
}
}
lightning/src/util/ser.rs
Outdated
|
||
impl Writeable for PhantomData<[u8; 12]> { | ||
fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> { | ||
w.write_all(&[0;12])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Space between ;
and 12
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
9a0a3b7
to
1b2a1ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to take this before 434, since it looks like that may get delayed a few days.
@@ -610,7 +611,7 @@ pub(crate) struct OnionRealm0HopData { | |||
pub(crate) short_channel_id: u64, | |||
pub(crate) amt_to_forward: u64, | |||
pub(crate) outgoing_cltv_value: u32, | |||
// 12 bytes of 0-padding | |||
pub(crate) padding: PhantomData<[u8; 12]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably do something like this anyway without generic constants. PhantomData has a somewhat specific meaning in Rust (that we have a template type that we're not going to instantiate). We probably just want a new type in ser.rs like (but with better names):
pub(crate) struct PaddingData<T: Default + std::convert::AsRef<[u8]>> {
t: std::marker::PhantomData<T>,
}
impl<T: Default + std::convert::AsRef<[u8]>> Writeable for PaddingData<T> {
#[inline]
fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
let nt: T = Default::default();
w.write_all(nt.as_ref())
}
}
@varunsrin #434 has been merged, so you should be able to rebase now. |
perfect, ill get to this by early next week |
Closing as abandoned. If you want to pick this back up just respond here and we can open it again :) |
Fixes #467