Value refactor #237
Value refactor #237
Conversation
uncomputable
force-pushed
the
2024-07-value-refactor
branch
2 times, most recently
from
11235bc to
1a81869
Compare
|
Fixed the jets-bench subcrate. |
src/value.rs
Outdated
| /// this means that the unit value doesn't contain any information. | ||
| /// | ||
| /// The unit value is wrapped in left and right values | ||
| /// to encode information. |
There was a problem hiding this comment.
In 7459af0:
"The unit value must be embedded in a sum type to encode information."
There was a problem hiding this comment.
I like the word "wrap" because we wrap values in Rust all the time. Values cannot be embedded inside types.
src/value.rs
Outdated
| /// | ||
| /// A left value is the first option for a sum type `A + B`. | ||
| /// It means we go left and wrap a value of type `A`. | ||
| /// The choice of going left encodes one bit. |
There was a problem hiding this comment.
In 7459af0:
I find this "go left" terminology confusing. I think it's clearer to just say "Encodes a value of type A as a sum type A + B, using a bit to specify that the left type is occupied." Similar with the below.
src/value.rs
Outdated
| /// It means we go both left and right and | ||
| /// that we wrap values of both type `A` and type `B`. | ||
| /// There is no choice, | ||
| /// so the information is contained solely in the wrapped values. |
There was a problem hiding this comment.
In 7459af0:
This "only option" business is definitely confusing. The number of values in a product type is equal to the product of the numbers of values of the factor types. I would drop this paragraph and say "Since there is no ambiguity about which types are occupied, no additional bits are used and the information is contained solely in the wrapped values.". If you want to say this at all.
| .collect::<Vec<u8>>() | ||
| .try_into() | ||
| .unwrap(); | ||
| let mid_state = Value::u256(&arr); |
There was a problem hiding this comment.
I switched to using Value::power_of_two, which is also internally called by Value::u256. We still have to allocate a Vec, like on master, but we no longer have to create an extra byte array.
|
utACK 1a81869. Let me know if you want to address the above comments, and I can ack/merge. |
Value::Left and Value::Right better reflect the parallel to Either::Left and Either::Right. Value::Product is more easily readable than Value::Prod.
Value::left matches Value::Left and Value::as_left. Value::right matches Value::Right and Value::as_right. Value::product matches Value::Product and Value::as_product.
uncomputable
force-pushed
the
2024-07-value-refactor
branch
from
1a81869 to
f308d5e
Compare
|
Rebased on master after #239 got merged |
jets-bench/src/data_structures.rs
Outdated
| @@ -169,14 +169,15 @@ impl SimplicityEncode for SimplicityCtx8 { | |||
| .iter() | |||
| .flat_map(|x| x.to_be_bytes()) | |||
| .collect::<Vec<u8>>(); | |||
| let mid_state = Value::u256_from_slice(&arr); | |||
| debug_assert_eq!(arr.len(), 32); | |||
| let mid_state = Value::power_of_two(&arr); | |||
There was a problem hiding this comment.
In c1527fa:
I'm confused as to what you're doing here. In the previous version of this commit you used Value::u256 because this is guaranteed to be 32 bytes. But now you are just adding another runtime assertion. And this still doesn't touch the unnecessary Vec in the line above.
There was a problem hiding this comment.
I can convert [u32; 8] directly into [u8; 32] if you want. The code will be more verbose because I will have to initialize the array and then assign values byte by byte. I cannot use Rust's iterator API, AFAIK.
I misunderstood your original comment. I thought you called the Vec unnecessary because I converted it into an [u8; 32] array on the stack. I removed the array, but you seem to have wanted me to remove the Vec altogether.
If we create a Value from &[u8; 32] then we assert the length of the
slice at compile time and cannot fail at runtime. Similar for &[u8; 64].
This is similar to rust-bitcoin's from_byte_array with the difference
that Value takes the bytes by reference.
It turns out, in rust-simplicity we can simply convert every call of
{u256, u512}_from_slice to a call of {u256, u512} without problem.
In the jets-bench subcrate, we have to convert [u32; 8] to [u8; 32]
or &[u8; 33] to &[u8; 32] via try_into. I think this is still worth
doing, and we can see in the code that these conversions will never fail.
Keep {u256, u512}_from_slice for backwards compatability.
I don't think we need these methods any more. A caller with an
arbitrarily-sized slice can use Value::power_of_two. A caller with a
precisely-sized slice can use the safer Value::{u256, u512}.
We can remove this commit from the PR if it turns out that we need
{u256, u512}_from_slice for some reason.
uncomputable
force-pushed
the
2024-07-value-refactor
branch
from
f308d5e to
85fd5b3
Compare
|
Removed the unnecessary vector. |
|
Ok, awesome. ACK 85fd5b3 except that I'd prefer that these new functions took arrays by value, not by reference. It is almost certainly slower, and more annoying for the programmer, to be passing 32-byte objects around by reference. |
|
I did not actually test this or use the github "approve" button. I did not intend my comment to be read as an ACK. |
|
The github-merge script from the bitcoin maintainer tools interpreted the comment as an ACK. Sorry for that. I will open a fixup PR to pass the 32 / 64 bytes by value. |
|
No worries -- I shouldn't have written ACK in all caps so that the script wouldn't pick it up. And because this is such a minor thing I should've been clearer that I wanted you to respond to it before merging. |
5c6a26f refactor: Rename power_of_two -> from_byte_array (Christian Lewe) 3df9475 feat: Pass byte arrays by value to construct Value (Christian Lewe) Pull request description: Fixes outstanding issues of #237 which I failed to address before merging. ACKs for top commit: apoelstra: ACK 5c6a26f nice\! Tree-SHA512: 9ab3acc83b5040c9c5a839682d4eb06e7b23bd91fb23a62cdec27433a34477d5b49c3b932822b180ea254022513ca458a1ca15d8f22873da767f719cd2cbcba2
Rename the variants of
Valueand construct {256, 512}-bit values safely.