jets-bench: fix multiple issues with Ctx8 encoding #319
Conversation
|
Why the reduction on the limit of |
|
@roconnor-blockstream because I replaced n+1 with n. |
|
If the 8 is wrong now, the 16 was wrong before. (Which now that I think about it, is likely to be the case.) |
|
You can verify that if 0 <= n < 16 was the restriction for when the rule was 0 <= n < 16 Indeed n’ <= 16 is the assertion I think you should be using. Also the comment
is incorrect; this is not a consensus rule. This comes from where |
apoelstra
force-pushed
the
2025-11/benchmark-ctx8
branch
from
0d4c323 to
5021eca
Compare
|
Updated the comment and the assertion. |
jets-bench/src/data_structures.rs
Outdated
| /// Cannot represent >= 2**16 bytes 0 <= n < 16 as simplicity consensus rule. | ||
| /// Cannot represent >= 2**16 bytes 0 <= n < 16; higher values are unused | ||
| /// by the current set of jets and would cause trouble on 16-bit machines | ||
| /// that we'd rather not think about. |
There was a problem hiding this comment.
Let's just remove this comment entirely or say "n < 16" is a precondition.
P.S. I recommend you adjust the code so that "n <= 16" is the precondition.
Really the only reason not to go to as far as "n <= 32" is that I don't know if 1 << 32 fits in a usize.
There was a problem hiding this comment.
It will on a 64-bit machine but not on a 32-bit one. (In theory usize could even be 16 bits but probably our code is broken in many cases on such machines.)
apoelstra
force-pushed
the
2025-11/benchmark-ctx8
branch
from
5021eca to
5a46145
Compare
|
How about this? |
jets-bench/src/data_structures.rs
Outdated
| /// # Panics: | ||
| /// | ||
| /// Panics if the length of the slice is >= 2^(n + 1) bytes | ||
| /// Panics if the length of the slice is >= 2^n bytes, or if n >= 16. |
There was a problem hiding this comment.
panics if n > 16.
| n -= 1; | ||
| } | ||
| assert_eq!(iter.next(), None); | ||
| Ok(res.unwrap_or(Value::unit())) |
There was a problem hiding this comment.
So I'm going to actually recommend that you have precondition that 1 <= n, because 𝟙 is not actually a buffer type, and if people are passing 0 here it is probably a big mistake.
If you strongly disagree then you can keep the code as it is.
There was a problem hiding this comment.
Actually I take that back. Arguably 𝟙 is a buffer type for a degenerate empty buffer type. So you can leave the code as is.
| // Simplicity consensus rule for n < 16 while reading buffers. | ||
| assert!(n < 16); | ||
| assert!(v.len() < (1 << (n + 1))); | ||
| // This precondition can probably be removed or relaxed, though |
There was a problem hiding this comment.
It wasn't immediately obvious what this code was doing, so I had to read the doc comment, write it myself, then compare my code against this. It might've been clearer if while n > 0 was replaced with for i in (0..n).rev() (since the loop is just counting down to zero), plus that means you can get rid of (n - 1) (subtraction on usize always makes me do a double-take) and it changes two_two_n(n + 2) to two_two_n(i + 3) (where + 3 is obviously just a bytes-to-bits conversion).
That's all nit-picking though. Otherwise LGTM.
apoelstra
force-pushed
the
2025-11/benchmark-ctx8
branch
2 times, most recently
from
6fa6bcd to
690d5f2
Compare
|
Took @canndrew's suggestions. |
jets-bench/src/data_structures.rs
Outdated
| /// | ||
| /// Panics if the length of the slice is >= 2^(n + 1) bytes | ||
| /// Panics if the length of the slice is >= 2^n bytes or if n > 16. | ||
| pub fn var_len_buf_from_slice(v: &[u8], mut n: usize) -> Result<Value, EarlyEndOfStreamError> { |
There was a problem hiding this comment.
n is no longer mutable?
There was a problem hiding this comment.
oh, oops, I test-compiled the wrong crate. You're right (and the compiler warns about this).
There was a problem hiding this comment.
Whew. I was confused why this wasn't noticed by the compiler.
* changes `var_len_buf_from_slice` to interpret its `n` such that it's reading the type (2^8)^<2^n rather than (2^8)^<2^(n+1); subtract 1 from all instances of `n` except the loop counter. (Previously we were looping the wrong number of times, which this fixes.) * calls `var_len_buf_from_slice` with n = 6, which matches the type signature for Ctx8. Before we were calling with n = 8 which was just wrong * Changes `SimplicityCtx8::buffer` to have type `[u8; 64]` instead of the wrong `[u8; 512]` * Changes the nonsense "v.len() >= 1 << n" comparison to one that checks the bits of v.len() * Corrects various off-by-one errors See BlockstreamResearch#318 for more information.
apoelstra
force-pushed
the
2025-11/benchmark-ctx8
branch
from
690d5f2 to
934a787
Compare
3 participants
var_len_buf_from_sliceto interpret itsnsuch that it's reading the type (2^8)^<2^n rather than (2^8)^<2^(n+1); subtract 1 from all instances ofnexcept the loop counter. (Previously we were looping the wrong number of times, which this fixes.)var_len_buf_from_slicewith n = 6, which matches the type signature for Ctx8. Before we were calling with n = 8 which was just wrongSimplicityCtx8::bufferto have type[u8; 64]instead of the wrong[u8; 512]See #318 for more information.
Fixes #318