8000 Avoid a few assertions that shouldn't be necessary by stevenroose · Pull Request #506 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Avoid a few assertions that shouldn't be necessary #506

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 6 commits into from
Oct 26, 2020

Conversation

stevenroose
Copy link
Collaborator

I went over all existing assertions, the ones I left are mostly wrong user input on methods that expect slices of a fixed size (like in decoding for endianness or so), I think those still make sense.

I just hit the PublicKey::write_into debug assertion running a long test involving programs sending messages over TCP connections (and not building with --release). So when one peer closes a TCP connection at the exact moment the other side is writing a public key, the assertion would cause the sender to panic.

@stevenroose stevenroose added the API break This PR requires a version bump for the next release label Oct 23, 2020
@stevenroose
Copy link
Collaborator Author
stevenroose commented Oct 23, 2020

EDIT: this is no longer the case, the API break is moved to #507

Since the change to Result<(), io::Error> is an API break, I'm willing to make a variant of this PR that just removes the debug_assert in PublicKey::write_into first together with all the other (not-breaking) changes and then PR the API change separately.

dr-orlovsky
dr-orlovsky previously approved these changes Oct 23, 2020
@apoelstra
Copy link
Member

@stevenroose Yeah, I'd prefer that. This way we can get a "fix" in 0.26 even before we properly fix it.

@stevenroose stevenroose removed the API break This PR requires a version bump for the next release label Oct 23, 2020
@stevenroose
Copy link
Collaborator Author

K, I changed this PR to no longer change the API. Then I'll make another one for that.

dr-orlovsky
dr-orlovsky previously approved these changes Oct 24, 2020
elichai
elichai previously approved these changes Oct 25, 2020
Copy link
Member
@elichai elichai left a comment

Choose a reason for hiding this comment

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

LGTM
FWIW most of these asserts (the size_of + bytes) will be optimized out in release builds anyway

@stevenroose
Copy link
Collaborator Author

LGTM
FWIW most of these asserts (the size_of + bytes) will be optimized out in release builds anyway

Oh is that like a cargo optimization? I didn't know about that, but well this approach more explicitly expresses that intent, IMO. And perhaps other Rust compilers aren't that smart 😅

@apoelstra
Copy link
Member

Can you add a commit which bumps the minor crate version and adds a changelog entry? I think this is the only thing we're targeting for 0.25.2.

apoelstra
apoelstra previously approved these changes Oct 26, 2020
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ack 90e1125

@stevenroose stevenroose dismissed stale reviews from apoelstra, elichai, and dr-orlovsky via e07ee51 October 26, 2020 15:39
@stevenroose
Copy link
Collaborator Author

Done.

@elichai
Copy link
Member
elichai commented Oct 26, 2020

LGTM
FWIW most of these asserts (the size_of + bytes) will be optimized out in release builds anyway

Oh is that like a cargo optimization? I didn't know about that, but well this approach more explicitly expresses that intent, IMO. And perhaps other Rust compilers aren't that smart sweat_smile

No, it's called "constant propogation", when the compiler see things that it can evaluate in compile time it will evaluate and change the branches accordingly,
i.e.

if 5< 3 {
    println!("One");
} else {
    println!("Two");
}

Will const-propogate to:

println!("Two");

so also:
assert_eq!(::std::mem::size_of::<$type>(), $byte_len); will macro expand into i.e.
assert_eq!(std::mem::size_of::<u64>(), 8); -> assert_eq!(8, 8); -> {if 8 != 8 {panic!("assertion failed....")}} -> {} :)

Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

re-ack e07ee51

Copy link
Member
@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comme 8000 nt to others. Learn more.

ACK e07ee51

@apoelstra apoelstra merged commit 46fdee5 into rust-bitcoin:master Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0