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

Remove calls to debug_assert #953

Closed
wants to merge 4 commits into from
Closed

Remove calls to debug_assert #953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
93c4151
Replace runtime size check with compile time check
tcharding Apr 19, 2022
5485e69
Use assert_eq for len check
tcharding Apr 17, 2022
7f79afd
Use assert_eq for string bytes length check
tcharding Apr 17, 2022
15d3800
Use assert_eq for encoded data length check
tcharding Apr 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/consensus/encode.rs
View file
Open in desktop
Expand Up @@ -140,7 +140,7 @@ impl From<psbt::Error> for Error {
pub fn serialize<T: Encodable + ?Sized>(data: &T) -> Vec<u8> {
let mut encoder = Vec::new();
let len = data.consensus_encode(&mut encoder).expect("in-memory writers don't error");
debug_assert_eq!(len, encoder.len());
assert_eq!(len, encoder.len());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Opinion: debug_assert_eq was fine here.

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR this is one of the ones that @TheBlueMatt alluded to in saying this is really a test of the consensus_encode function and as such belongs in a test not here in a debug build.

re debug_assert vs assert, and I'm just learning this this week, I have been attempting to follow the Rust docs which state that debug_assert should only be used if the code was profiled and assert was found to be a bottle neck. Do you disagree with this @Kixunil?

Copy link
Member

Choose a reason for hiding this comment

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

debug_assert should only be used if the code was profiled and assert was found to be a bottle neck.

I'm not sure I like that line? I mean I'm not sure why we should be crashing the users' application because we're doing something wrong. Its not really a big deal, like hopefully this stuff is all right and we wont hit it anyway, but I've never heard of "only use asserts if they're slow" - coming from C, -DNDEBUG is a thing that (usually) disables all assert()s.

encoder
}

Expand Down Expand Up @@ -240,7 +240,7 @@ macro_rules! decoder_fn {
($name:ident, $val_type:ty, $readfn:ident, $byte_len: expr) => {
#[inline]
fn $name(&mut self) -> Result<$val_type, Error> {
debug_assert_eq!(::core::mem::size_of::<$val_type>(), $byte_len); // size_of isn't a constfn in 1.22
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?
Also, I'm pretty sure after the MSRV bump we can make this a compile-time check:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=44cce9ee0d3113cdaaaf7c6f421d5699

Copy link
Member Author

@tcharding tcharding Apr 18, 2022 •
edited

Choose a reason for hiding this comment

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

Cool. I'm no macro expert but is this as good or am I missing something?

macro_rules! const_assert {
    ($x:expr) => {
        const _UNUSED: [(); 0 - !$x as usize] = [];
    };
}

This works with Rust 1.29 as well.

(_UNUSED instead of _ because of Rust 1.29)

Copy link
Member

Choose a reason for hiding this comment

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

OoO I didn't think that this can be done in 1.29!
That will work but will limit the macro to work only once in every scope, otherwise, you'll get multiple definitions of _UNUSED. but it doesn't seem like we ever const-assert more than one thing in the same scope

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to double check, is the simplification of the original

[(); 0 - !{ const ASSERT: bool = $x; ASSERT } as usize] = []

to

[(); 0 - !$x as usize] = []

ok? I'm going to force push this change but wanted to explicitly double check with you. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Yes :)

const_assert!(::core::mem::size_of::<$val_type>() == $byte_len);
let mut val = [0; $byte_len];
self.read_exact(&mut val[..]).map_err(Error::Io)?;
Ok(endian::$readfn(&val))
Expand Down
10 changes: 10 additions & 0 deletions src/internal_macros.rs
View file
Open in desktop
Expand Up @@ -564,3 +564,13 @@ macro_rules! user_enum {
}
);
}

/// Asserts a boolean expression at compile time.
macro_rules! const_assert {
($x:expr) => {
// Use `_UNUSED` and extra scope instead of `_` due to Rust 1.29
{
const _UNUSED: [(); 0 - !$x as usize] = [];
}
};
}
2 changes: 1 addition & 1 deletion src/network/message.rs
View file
Open in desktop
Expand Up @@ -78,7 +78,7 @@ impl Encodable for CommandString {
fn consensus_encode<S: io::Write>(&self, s: S) -> Result<usize, io::Error> {
let mut rawbytes = [0u8; 12];
let strbytes = self.0.as_bytes();
debug_assert!(strbytes.len() <= 12);
assert!(strbytes.len() <= 12);
rawbytes[..strbytes.len()].copy_from_slice(strbytes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Slicing already performs this check. assert would've been better if the resulting error message was clearer than the default which I don't think is the case here. AFAIR out-of-bounds slicing prints the lengths while assert-ing like this doesn't. Thus this assert only has disadvantages and it's best to remove it completely.
(Side note: an assert like this could have been good before a loop.)

OT: maybe we could serialize the slice directly and fill in extra zeroes to avoid a bunch of copies. Something for opt nerds. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will revisit this one after other discussion no the PR resolves. Thanks.

rawbytes.consensus_encode(s)
}
Expand Down
4 changes: 2 additions & 2 deletions src/util/endian.rs
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ macro_rules! define_be_to_array {
($name: ident, $type: ty, $byte_len: expr) => {
#[inline]
pub fn $name(val: $type) -> [u8; $byte_len] {
debug_assert_eq!(::core::mem::size_of::<$type>(), $byte_len); // size_of isn't a constfn in 1.22
elichai marked this conversation as resolved.
Show resolved Hide resolved
const_assert!(::core::mem::size_of::<$type>() == $byte_len);
let mut res = [0; $byte_len];
for i in 0..$byte_len {
res[i] = ((val >> ($byte_len - i - 1)*8) & 0xff) as u8;
Expand All @@ -41,7 +41,7 @@ macro_rules! define_le_to_array {
($name: ident, $type: ty, $byte_len: expr) => {
#[inline]
pub fn $name(val: $type) -> [u8; $byte_len] {
debug_assert_eq!(::core::mem::size_of::<$type>(), $byte_len); // size_of isn't a constfn in 1.22
elichai marked this conversation as resolved.
Show resolved Hide resolved
const_assert!(::core::mem::size_of::<$type>() == $byte_len);
let mut res = [0; $byte_len];
for i in 0..$byte_len {
res[i] = ((val >> i*8) & 0xff) as u8;
Expand Down
2 changes: 1 addition & 1 deletion src/util/taproot.rs
View file
Open in desktop
Expand Up @@ -429,7 +429,7 @@ impl TaprootBuilder {
}
// Every iteration of the loop reduces the node_weights.len() by exactly 1
// Therefore, the loop will eventually terminate with exactly 1 element
debug_assert_eq!(node_weights.len(), 1);
assert_eq!(node_weights.len(), 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Opinion: debug was fine here.

let node = node_weights.pop().expect("huffman tree algorithm is broken").1;
Ok(TaprootBuilder{branch: vec![Some(node)]})
}
Expand Down