Remove calls to debug_assert #953

tcharding · 2022-04-18T00:10:06Z

Done after reading discussion on #853.

Audit the code for calls to the various debug_assert macros. Check if the call is likely to be add any performance costs and if not use the equivalent assert macro.

Done as separate patches so that each change can be fully described by the commit message.

With this PR applied there is one instance of debug_assert! left, the call checks against the result of tweak_add_check. I do not know if this call is fast or slow so I left it as it is.

elichai · 2022-04-18T15:41:12Z

src/consensus/encode.rs

@@ -240,7 +240,6 @@ 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


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

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)

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

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

src/util/endian.rs

apoelstra

ACK e2fff70

I also don't see the value in removing the size checks, but OTOH I don't see a lot of value in keeping them.

TheBlueMatt

At the risk of being a dick for jumping in late, I'm not sure why we'd want this - I don't have a deep understanding of PSBT, but do we really want to panic (and likely crash the user application) if we end up accidentally serializing a PSBT slightly wrong? I view these kinds of debug assertions as test cases that are slipped into the main code body because its easier to put tests there sometimes. At least on the LDK end, our stance is "panic if we're gonna lose money, return error or continue awkwardly otherwise".

tcharding · 2022-04-19T23:08:01Z

hmm, maybe an implicit benefit of using assert! over debug_assert is it forces devs to think 'do we really need this assertion' instead of, as @TheBlueMatt says, treating the statement like a test. I would argue that the debug_assert -> assert changes in this PR are a step forward and we should still, but later, decide if these assert statements should exist at all.

tcharding · 2022-04-19T23:21:09Z

Changes in force push: Use @elichai's const_assert macro idea in the first patch. Remaining patches unchanged. Please await a response from @TheBlueMatt before considering this mergable.

TheBlueMatt · 2022-04-19T23:24:59Z

hmm, maybe an implicit benefit of using assert! over debug_assert is it forces devs to think 'do we really need this assertion' instead of, as @TheBlueMatt says, treating the statement like a test.

Yea, totally fair. Sometimes it is just easier to write a test by slipping a debug assertion into the code right where you need to check something, though, though maybe such changes are better in #[cfg(fuzzing)] rather than debug assertions.

I would argue that the debug_assert -> assert changes in this PR are a step forward and we should still, but later, decide if these assert statements should exist at all.

Yea, fair, no opinion on these specific assertions, I'm not super in depth on the PSBT part of the codebase so don't really have an opinion.

apoelstra

ACK dc97d37

By the way I think const_assert could be used repeatedly in the same scope if you added an extra scope inside the macro.

sanket1729

utACK dc97d37

sanket1729 · 2022-04-20T02:22:06Z

I don't have a deep understanding of PSBT, but do we really want to panic (and likely crash the user application) if we end up accidentally serializing a PSBT slightly wrong?

This PR is not dealing with any Psbt code.

tcharding · 2022-04-20T04:01:50Z

By the way I think const_assert could be used repeatedly in the same scope if you added an extra scope inside the macro.

Nice one!

Changes in force push are to the first patch, added additional scope as suggested. No other changes.

Kixunil

Firstly, changing asserts to const is great!

Personal opinion follows: If an assert guards the inputs to the function (prevents invariants breaking) which can not be or would be to cumbersome to be implemented using newtypes it has to be non-debug assert!. All else can be debug_ just fine. However if people like non-debug asserts since they are cheap, I'm not going to block it.

I prefer to remove the redundant one.

Kixunil · 2022-04-20T16:32:05Z

src/consensus/encode.rs

@@ -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());


Opinion: debug_assert_eq was fine here.

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?

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.

Kixunil · 2022-04-20T16:39:57Z

src/network/message.rs

@@ -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);


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. :)

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

Kixunil · 2022-04-20T16:40:42Z

src/util/taproot.rs

@@ -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);


Opinion: debug was fine here.

elichai · 2022-04-20T17:23:02Z

ACK dc97d37

By the way I think const_assert could be used repeatedly in the same scope if you added an extra scope inside the macro.

That's cool, the downside of that though is that it can't be used out of scope :) (outside of a function)

sanket1729

ACk 7acaa4b. I can see arguments for both debug_assert and assert, but I think we all agree that for this PR it is an improvement?.

apoelstra

ACK 7acaa4b

But sounds like there are still some open issues?

tcharding · 2022-04-20T22:13:30Z

Changing to draft, this was only meant to be a quick thing and as it has uncovered a can of worms I'll put it on the back burner till after the MSRV chaos is over.

Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false. Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro.

According to the Rust docs for `debug_assert` it should only be used if there a proved performance costs to using `assert`. Checking the length of a `BinaryHeap` is equivalent to checking the length of a `Vec` which is fast, therefore its unlikely that using `assert_eq` will have any noticeable performance costs.

According to the Rust docs for `debug_assert` it should only be used if there a proved performance costs to using `assert`. Checking the length of the slice returned by `as_bytes` is fast, therefore its unlikely that using `assert_eq` will have any noticeable performance costs.

According to the Rust docs for `debug_assert` it should only be used if there a proved performance costs to using `assert`. Checking the length of a vector is fast, therefore its unlikely that using `assert_eq` will have any noticeable performance costs.

sanket1729

ACK 15d3800 . Range-diff between 7acaa4b and 15d3800 is nil.

TheBlueMatt

I'm not really sure I understand why do this? These assertions mostly don't seem like the kind of thing that's going to result in a user losing money, and in such a case it doesn't seem worth crashing the users' app?

apoelstra · 2022-05-21T14:00:38Z

@TheBlueMatt if these ever trigger then we (the developers) have gotten very confused about what state the library is in. I don't expect that they will ever trigger.

You are correct that these are essentially unit tests, and maybe shouldn't be inline in the code (though they are easier to find and understand this way), which is why they were originally debug_asserts. I also don't really understand the principle behind turning them into real asserts, but I think it's totally harmless (i.e. will not hurt perf and will not cause crashes).

apoelstra

ACK 15d3800

but still looks like there is disagreement about whether we want this PR

TheBlueMatt · 2022-05-21T16:50:51Z

if these ever trigger then we (the developers) have gotten very confused about what state the library is in. I don't expect that they will ever trigger.

Yep, we're on the same page.

You are correct that these are essentially unit tests, and maybe shouldn't be inline in the code (though they are easier to find and understand this way)

Yep, we're on the same page.

I also don't really understand the principle behind turning them into real asserts, but I think it's totally harmless (i.e. will not hurt perf and will not cause crashes).

Yep, I'm fine with it, I just wanted to point out that I don't think "never debug_assert unless its a performance loss" is the right line to draw. While rust-bitcoin is lower level so isn't as clear in general, on the LDK/rust-lightning side of things we try to stick with "asserts are for where we're in a state where we may have lost/may lose money, debug_asserts are for everything else". Indeed, because sometimes its just way easier to slip in unit tests in-line, we have some nontrivial debug assertions in-line to check consistency of somewhat complex state. Indeed, some of them used to be regular asserts and we had users hit them in production.

Again, rust-bitcoin is a little harder to make that distinction with - if a serialization call returns the wrong length can a user end up signing something with too little fee and lose money cause they had some CLTV timelock they had to meet? You can arguably make a similar argument for most parts of rust-bitcoin, so I'm not really against rust-bitcoin going down the "always assert" path, just making sure we're drawling a line that makes sense.

tcharding · 2022-05-24T06:01:01Z

Closing this since it is contentious.

Kixunil · 2022-05-24T08:36:23Z

Perhaps reconsider doing const asserts only, I think they'd be non-contentious. (At least I think it'd be great and never heard anyone arguing against them.)

tcharding · 2022-05-24T21:44:51Z

Done: #1000

PR 1000, BOOM!

@elichai

7a3bb7d Replace runtime size check with compile time check (Tobin C. Harding) Pull request description: Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false. Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro. ## Note This PR is the first patch from a [recently closed PR](#953). Props to @elichai for the macro idea in the review of that PR. ACKs for top commit: Kixunil: ACK 7a3bb7d apoelstra: ACK 7a3bb7d Tree-SHA512: cfd4dcf6c66e06796cab6dc49445f0f8c5d4e686893a17735420dccedd75ad7c632d240a5ab92ee47ce459b799daeaf3fdf9c6b77c1b81b09e87197a9f86c5ba

@elichai

… compile time check 7a3bb7d Replace runtime size check with compile time check (Tobin C. Harding) Pull request description: Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false. Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro. ## Note This PR is the first patch from a [recently closed PR](rust-bitcoin/rust-bitcoin#953). Props to @elichai for the macro idea in the review of that PR. ACKs for top commit: Kixunil: ACK 7a3bb7d apoelstra: ACK 7a3bb7d Tree-SHA512: cfd4dcf6c66e06796cab6dc49445f0f8c5d4e686893a17735420dccedd75ad7c632d240a5ab92ee47ce459b799daeaf3fdf9c6b77c1b81b09e87197a9f86c5ba

elichai reviewed Apr 18, 2022

View reviewed changes

apoelstra previously approved these changes Apr 18, 2022

View reviewed changes

TheBlueMatt reviewed Apr 19, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via dc97d37 April 19, 2022 23:20

tcharding force-pushed the rm-debug-assert branch from e2fff70 to dc97d37 Compare April 19, 2022 23:20

apoelstra previously approved these changes Apr 20, 2022

View reviewed changes

sanket1729 previously approved these changes Apr 20, 2022

View reviewed changes

tcharding dismissed stale reviews from sanket1729 and apoelstra via 7acaa4b April 20, 2022 04:01

tcharding force-pushed the rm-debug-assert branch from dc97d37 to 7acaa4b Compare April 20, 2022 04:01

tcharding mentioned this pull request Apr 20, 2022

Add psbt watch-only/cold-storage example #940

Merged

Kixunil reviewed Apr 20, 2022

View reviewed changes

sanket1729 previously approved these changes Apr 20, 2022

View reviewed changes

apoelstra previously approved these changes Apr 20, 2022

View reviewed changes

tcharding marked this pull request as draft April 20, 2022 22:13

tcharding added 4 commits May 19, 2022 16:25

Replace runtime size check with compile time check

93c4151

Add a macro `const_assert` that uses some const declaration trickery to trigger a compile time error if a boolean expression is false. Replace runtime checks using `debug_assert_eq!` with the newly defined `const_asert!` macro.

tcharding dismissed stale reviews from apoelstra and sanket1729 via 15d3800 May 19, 2022 06:25

tcharding force-pushed the rm-debug-assert branch from 7acaa4b to 15d3800 Compare May 19, 2022 06:25

tcharding marked this pull request as ready for review May 19, 2022 21:33

sanket1729 approved these changes May 20, 2022

View reviewed changes

TheBlueMatt reviewed May 20, 2022

View reviewed changes

apoelstra approved these changes May 21, 2022

View reviewed changes

tcharding closed this May 24, 2022

tcharding mentioned this pull request May 24, 2022

Replace runtime size check with compile time check #1000

Merged

tcharding deleted the rm-debug-assert branch May 26, 2022 05:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove calls to debug_assert #953

Remove calls to debug_assert #953

tcharding commented Apr 18, 2022 •

edited

elichai Apr 18, 2022

tcharding Apr 18, 2022 •

edited

elichai Apr 19, 2022

tcharding Apr 19, 2022

elichai Apr 20, 2022

apoelstra left a comment

TheBlueMatt left a comment •

edited

tcharding commented Apr 19, 2022

tcharding commented Apr 19, 2022

TheBlueMatt commented Apr 19, 2022

apoelstra left a comment

sanket1729 left a comment

sanket1729 commented Apr 20, 2022

tcharding commented Apr 20, 2022

Kixunil left a comment

Kixunil Apr 20, 2022

tcharding Apr 20, 2022

TheBlueMatt May 20, 2022

Kixunil Apr 20, 2022

tcharding Apr 20, 2022

Kixunil Apr 20, 2022

elichai commented Apr 20, 2022

sanket1729 left a comment •

edited

apoelstra left a comment

tcharding commented Apr 20, 2022

sanket1729 left a comment

TheBlueMatt left a comment

apoelstra commented May 21, 2022

apoelstra left a comment

TheBlueMatt commented May 21, 2022

tcharding commented May 24, 2022

Kixunil commented May 24, 2022 •

edited

tcharding commented May 24, 2022

Remove calls to debug_assert #953

Remove calls to debug_assert #953

Conversation

tcharding commented Apr 18, 2022 • edited

Choose a reason for hiding this comment

tcharding Apr 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment • edited

Choose a reason for hiding this comment

tcharding commented Apr 19, 2022

tcharding commented Apr 19, 2022

TheBlueMatt commented Apr 19, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

sanket1729 commented Apr 20, 2022

tcharding commented Apr 20, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elichai commented Apr 20, 2022

sanket1729 left a comment • edited

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Apr 20, 2022

sanket1729 left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

apoelstra commented May 21, 2022

apoelstra left a comment

Choose a reason for hiding this comment

TheBlueMatt commented May 21, 2022

tcharding commented May 24, 2022

Kixunil commented May 24, 2022 • edited

tcharding commented May 24, 2022

tcharding commented Apr 18, 2022 •

edited

tcharding Apr 18, 2022 •

edited

TheBlueMatt left a comment •

edited

sanket1729 left a comment •

edited

Kixunil commented May 24, 2022 •

edited