units: Improve code enforcing privacy boundry#4258
units: Improve code enforcing privacy boundry#4258apoelstra merged 2 commits intorust-bitcoin:masterfrom
Conversation
|
Can rebase now. |
| /// represent roughly -21.47 to 21.47 BTC. | ||
| pub const fn from_sat_i32(satoshi: i32) -> SignedAmount { | ||
| SignedAmount(satoshi as i64) // cannot use i64::from in a constfn | ||
| } |
There was a problem hiding this comment.
The reason for this is not entirely obvious but the change is correct. So to clarify: this being inside privacy boundary allows it to not have the "runtime" check (most likely optimized-away after inlining). But if we wanted to get rid of that check we should have _unchecked method instead. But we don't want that (yet), since the check here will have zero performance impact in optimized builds and it's not worth the cost of dealing with unchecked constructors to optimize debug builds.
Since you have to rebase anyway, please add it to the commit message or code comment.
There was a problem hiding this comment.
Thanks for more explanation. is my understanding correct that:
- Items inside encapsulate are in a privacy boundary?
- Items inside encapsulate are not "runtime" checked.
- moving from_sat_i32 outside of encapsulate means it is "runtime" checked?
- Items inside the privacy boundary access the inner-field while those outside the privacy boundary do not?
Wouldn't mod private be more descriptive than mod encapsulate if items inside encapsulate are insides a "privacy boundary"? Furthermore, I'm struggling with the notion of "constructors" that are in a "privacy boundary". Looking at these concepts from the outside of those developing has left me confused, although I'm sure to those implementing it it's perfectly logical.
There was a problem hiding this comment.
It's not really that much related to runtime checks. It's about which methods have access to the internal field. Ideally there should be only one so that if layout changes we don't have to modify a bunch of places that relied on something specific (like it being u64 here). Realistically, there need to be at least two, sometimes four methods.
One of the arguments to put more than one function into the privacy boundary is to bypass the checks for perf reason without introducing _unchecked constructor. This code move is about that argument being invalid here.
There was a problem hiding this comment.
I see now what the motivation is here, and that SignedAmount(satoshi as i64) is only usable inside the encapulate namespace. Reducing the reliance on the inner field would make changes and refactor less noisy.
There was a problem hiding this comment.
There's also the part about not breaking invariant by accident. Basically, to make sure nothing breaks the invariant we only have to audit the submodule and nothing else. That makes code review much easier.
bc2d154 to
6ca5f40
Compare
6ca5f40 to
3909aa2
Compare
|
Note please I added the |
|
Needs rebase |
Recently I wrote a panic message that included the maximum value of an integer however I used the max of a 16 bit value for both signed and unsigned - this is incorrect. Use the correct values for `u16::MAX` and `i16::MAX`.
3909aa2 to
29a8115
Compare
From my reading of the new sanity rules (rust-bitcoin#4090) we should only have a single constructor that accesses the inner field of the amount types. Furthermore we have one const constructor inside the privacy boundry and a couple outside. Move the const constructors outside of the privacy boundry. Internal change only. Please note The function being inside privacy boundary allows it to not have the "runtime" check (most likely optimized-away after inlining). But if we wanted to get rid of that check we should have _unchecked method instead. But we don't want that (yet), since the check here will have zero performance impact in optimized builds and it's not worth the cost of dealing with unchecked constructors to optimize debug builds.
29a8115 to
ca6c607
Compare
|
cc @Kixunil could you re-ack this? The rebase is nontrivial (I mean, it's conceptually trivial but there is still a bunch of stuff in the range-diff.) |
|
Merge please. |
This is a follow up to #4256 - onwards and upwards!
Close: #4140