It's conceptually a bit tortured to have an `Amount` type defined in a
private module, with an _unchecked method allowing you to set values out
of range, which needs to be used outside of the module to *define* the
range and the constructors that check it.
Move the constants and constructors inside the privacy module, where they
can be written directly. This is easier to understand and eliminates a couple
_unchecked calls.
This private function is used for string-parsing an amount. It returns a
sign boolean and a u64, but its surrounding logic can be simplified if
it just returns a i64 (which is OK now since the range of `Amount` fits
into the range of i64).
Along the way we eliminate some instances of from_sat_unchecked.
Annoyingly we still need the bool to distinguish -0 from +0; when
parsing Amount we reject -0 (and we have tests for this).
This causes a couple error constructors to no longer be used outside of
unit tests. May be worth looking at whether we need them, or whether we
should be using them in parse_signed_to_satoshi.
We have a ton of calls to `from_sat_unchecked` for small constants which
were clearly in range, e.g. in fee.rs. Add a new constfn for these
cases. Don't bother making a generic Into<u32>/Into<u16> variant because
there isn't an obvious name for it.
There are 7 instances where we're using this method with values that are
out of range, which we leave as from_sat_unchecked for now.
We have `from_int_btc_const` on both `Amount` and `SignedAmount` because
the "real" `from_int_btc` is generic over the integer type it accepts,
which means that it cannot be a constfn. But we do want a constfn.
However, just because `from_int_btc_const` exists for the sake of
constfn doesn't mean that that's what it *is*. So rename these methods
to reflect what they *are*.
The `from_int_btc_const` constructors are specifically designed for
easily creating amount types in const context but currently they return
an error which is annoying to handle in const context. If we make the
`whole_bitcoin` parameter a 16 bit integer this gives us a nicer const
constructor with the downside that it can only create values upto a
maximum of
- unsigned: 65_536
- signed: 32_767
That is plenty high enough for most use cases.
Then use the new `from_int_btc_const` in associated consts.
Note that because `from_sat` checks max (and min) values we must
define max and min from sats directly.
ab4ea7c13d Enforce the MAX_MONEY invariant in amount types (Tobin C. Harding)
Pull request description:
Enforcing the `MAX_MONEY` invariant is quite involved because it means multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is eliminated in various places
Details:
- Update `from_sat` to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate `unchecked_abs`
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use `?` in rustdoc examples (required by Rust API guidlines)
- Remove `TryFrom<Amount> for SignedAmount` because the conversion is now infallible. Add a `From` impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private `check_max` function as its no longer needed
Close#620
ACKs for top commit:
apoelstra:
ACK ab4ea7c13d08411044bd5f9c17457e926c80ed4d; successfully ran local tests
Tree-SHA512: bec963d8ea69e202f399cd19bca864b06f3e86323d376c2d2126d74093598f8bbbf19792b2327dba0862ef6f0201202778014a2be7a14991f02917d8ca312afb
93c6c8cef5 Use impl_op_for_references macro in fee module (Erick Cestari)
Pull request description:
This pr replaces the individual operator implementations in the fee module with the impl_op_for_references macro to handle reference operations. This removes the need to manually implement reference combinations for operands, simplifying the code and improving consistency.
### Changes:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
Closes#4173
ACKs for top commit:
tcharding:
ACK 93c6c8cef5
apoelstra:
ACK 93c6c8cef59ceed56932d62daeb212c2b40fc4a1; successfully ran local tests; yeah, I think the docs loss is fine -- the docs are hard to find and say exactly what users expect of the / operator
Tree-SHA512: 51d7643c2cecd16a0cb16afcd195fd87bc9eca9116e16518d888ba61a8edb5684162af987ea52611c9463f5299810f92a057dedc3fa8e89cdef21ef40528bca1
c707b959b7 Rename timestamp module to time (Tobin C. Harding)
e2dee4900f Re-name Timestamp to BlockTime (Tobin C. Harding)
Pull request description:
Done in two patches so we can bikeshed the name of the type and separately the name of the module.
- Rename type: `Timestamp` to `BlockTime`
- Rename module: `timestamp` to `time`
ACKs for top commit:
apoelstra:
ACK c707b959b72dd89ca6df581a6102f32daedb8368; successfully ran local tests
Tree-SHA512: de3855b38445a58b6767a6081919eecb81c6c12aee3f6699f3bfa10efaf5770b54fb412da23991a9ee734e14dfb642af670f0218d1886cdc8c8d3f393ef65d7e
Enforcing the MAX_MONEY invariant is quite involved because it means
multiple things:
- Constructing amounts is now fallible
- Converting from unsigned to signed is now infallible
- Taking the absolute value is now infallible
- Integer overflow is illuminated in various places
Details:
- Update from_sat to check the invariant
- Fix all docs including examples
- Use the unchecked constructor in test code
- Comment any other use of the unchecked constructor
- Deprecate unchecked_abs
- Fail serde (using the horrible string error variant)
- Try not to use the unchecked constructor in rustdocs, no need to encourage unsuspecting users to use it.
- Use ? in rustdoc examples (required by Rust API guidlines)
- Remove TryFrom<Amount> for SignedAmount because the conversion is now infallible. Add a From impl.
- Fix the arbitrary impls
- Maintain correct formatting
- Remove private check_max function as its no longer needed
This commit replaces the individual operator implementations in the fee
module with the impl_op_for_references macro to handle reference operations.
This removes the need to manually implement reference combinations for
operands, simplifying the code and improving consistency.
The change:
- Replaces direct implementations of operators with macro usage
- Adds tests to verify that reference operations work correctly
- Maintains the same semantics as the original implementation
Now that we have the `NumOpResult<Amount>` type that is used to show a
math calculation returned a valid amount we can use it when multiplying
weight and fee rates thus removing panics.
I royally botched the recent effort to make const amount constructors
use a smaller type. I left in an unnecessary panic and forgot to do
both of them.
Note these function return values will change again very shortly when we
start enforcing the MAX_MONEY invariant. However the 64 to 32 bit change
is unrelated to that and is easier to review if done separately.
Whole bitcoin can not in any sane environment be greater than 21,000,000
which fits in 32 bits so we can take a 32 bit integer in the whole
bitcoin constructors without loss of utility. Doing so removes the
potential panic.
This is a breaking API change. We elect not to deprecate because we want
to keep the same function names.
The unchecked-should-be-unsafe conversation is out of scope for this
patch. We want to bite off small chunks so the constructors are left as
they currently are - we are just doing the encapsulation here. This is
in preparation for enforcing the MAX_MONEY invariant which is not
currently enforced.
As per the sanity rules policy outline in:
https://github.com/rust-bitcoin/rust-bitcoin/discussions/4090
For both amount types create a private `encapsulate` module that
consists of exactly the type and a single constructor and a single
getter.
We are about to start enforcing the MAX_MONEY invariant. Doing so will
change constructors to return an error type.
In preparation use the `_unchecked` constructor for all the consts.
Internal change only, no logic changes.
There is an as yet unresolved discussion about the unchecked amount
constructor. In an effort to focus the amount of changes required later
and also to make the `tests` module uniform use the `sat` and `ssat`
constructor functions everywhere.
Internal change only, no logic changes.
Throughout the `amount::tests` module we use `sat` and `ssat` as aliases
to amount constructors but in on test we use them as `Denomination`
variables. To assist clarity and so we can introduce uniform usage of
the constructor aliases change the variable names to use the `den_`
prefix.
Internal change only, no logic changes.
0a9f14f7b0 Implement Div by amount for amount types (Tobin C. Harding)
b57bfb9bc5 Add missing Mul impls for amount types (Tobin C. Harding)
501c9ab89e Test amount ops that involve an integer (Tobin C. Harding)
851080d3b1 Add more add/sub tests (Tobin C. Harding)
47923957b1 Improve add/sub tests for amount types (Tobin C. Harding)
8bb9ce3e47 Add tests for amount op int (Tobin C. Harding)
Pull request description:
Improve the test coverage and add missing implementations of math operations for the amount types.
Along the way close#4030.
ACKs for top commit:
apoelstra:
ACK 0a9f14f7b036c5232449d058fb6d425c8376d87a; successfully ran local tests; nice!
Tree-SHA512: f303b2a90b5bb9e77091e047f8325821a5c89f52dfe242d849968dba0d097d3868d444009c2c05b9d7c0e91fa2ce6898cdc4733977699ca4b1ae226562878cdf
We just re-named `Timestamp` to `BlockTime`. We have a `units::block`
module but it currently holds abstractions (`BlockHeight` and
`BlockInterval`) that are not onchain abstractions and therefore
somewhat different from the `BlockTime`. Instead of making `block` a
block 'utils' module instead re-name the `timestamp` module to `time`.
We just added a `Timestamp` type without knowing that there was a push
by OpenTimestamps to also create a timestamp and that our new type may
lead to confusion. Our timestamp is explicitly for the `time` field in a
block so we can call it `BlockTime`. This name change makes the module
name stale but we will change that in a following patch to ease review.
97453ef9bc units: Prevent casting pub enums as ints (Tobin C. Harding)
Pull request description:
A public enum with simple variants gets an automatic integer variant that can be cast by library consumers. This puts a unnecessary maintenance burden upon us because we cannot then add variants in the middle of others.
Add a hidden variant to the single public non-error enum in `units`.
ACKs for top commit:
Kixunil:
ACK 97453ef9bc
apoelstra:
ACK 97453ef9bc2b99a67252419ff015f13679df7312; successfully ran local tests
Tree-SHA512: 2515152107fb21a2dbdef9b46308fef6bd45f4a9719da7a39149b3bdbce6a93dc0f98e112ac246eb32dbe4df1210d5e6328c26ea8678e3da15276e893b39cc9c
90d909becc Kill mutants in primitives and units (Shing Him Ng)
Pull request description:
This kills 15 mutants found with the mutants workflow. Ran `cargo mutants` locally to confirm
Closes#4156Closes#4106
ACKs for top commit:
jamillambert:
ACK 90d909becc
tcharding:
ACK 90d909becc
apoelstra:
ACK 90d909becc4638c03003845154e9cc1eb5f3ad41; successfully ran local tests
Kixunil:
ACK 90d909becc
Tree-SHA512: e5c95a1c4054cf1c60c940ea605eec84dffcbff292f9c7c4d96813c6389e807c318f6c5f8f69ceeb9ffcab3c3e45aa0d5a8fda7335d540c6f070aab92bae7a0f
b3f122b399 Add Timestamp newtype (Tobin C. Harding)
Pull request description:
Bitcoin block headers have a timestamp. Currently we are using a `u32`. While this functions correctly it gives the compiler no chance to enforce type safety.
Add a `Timestamp` newtype that is a thin wrapper around a `u32`. Document it and test the API surface in `api.rs`.
ACKs for top commit:
apoelstra:
ACK b3f122b3996c1a73479be2f95b7f2ae642c9c56f; successfully ran local tests
Kixunil:
ACK b3f122b399
Tree-SHA512: 6f4a4a588bc836243ae28f3d36be6c0ae264cb2b7a0061277910b107d05e5ca0e679497d2890208f5d8ec148f37bf263bcd0b0410f9e5e6370d8e763ff30b78a
2434afc40d Make Weigth::from_kwu const (Tobin C. Harding)
1fe984552e Improve Weight rustdocs (Tobin C. Harding)
Pull request description:
Do two minor improvements to the `weight` module. Docs and add `const`.
ACKs for top commit:
apoelstra:
ACK 2434afc40dadd213f8695c5ce91d397c03f337be; successfully ran local tests
Kixunil:
ACK 2434afc40d
Tree-SHA512: e3e9653d5fcd060c27a2313e642d7b96f51b9342953505a30a9748cb7f0c19a87bcb1faadb1b07ecc770aaec45496e5a750ac48e3e9141e379c554f0875df6a1
A public enum with simple variants gets an automatic integer variant
that can be cast by library consumers. This puts a unnecessary
maintenance burden upon us because we cannot then add variants in the
middle of others.
Add a hidden variant to the single public non-error enum in `units`.
8007840676 Add a test for remainder (Jamil Lambert, PhD)
4787aa1f89 Implement Rem for Weight (Jamil Lambert, PhD)
Pull request description:
Weight implements `Div` but not `Rem`.
Add the `Rem` implementation.
Add a test for the remainder operation on `Weight`
Close#4171
ACKs for top commit:
Kixunil:
ACK 8007840676
tcharding:
ACK 8007840676
apoelstra:
ACK 80078406768aa5f8e4d21d42cc1d1fe9abeed1ea; successfully ran local tests
Tree-SHA512: cfcbc49a944146f7ba24a2f7b7f79ed2e2758ba2a23e9e69d9662afb5379dd74c646ff787c0e8218053e70410353cb74115e4c03bfdaafc8a1f9a4b0e51a3e15
83d071e54b chacha20: Add whitespace (Tobin C. Harding)
4451724d31 chacha20: Add a docs heading (Tobin C. Harding)
d4417f9666 io: Improve crate docs heading (Tobin C. Harding)
c466554948 hashes: Improve crate docs heading (Tobin C. Harding)
6f4eb60936 Improve docs crate headings (Tobin C. Harding)
Pull request description:
Make them all uniform after taking 2 minutes online to try find a nice format.
ACKs for top commit:
apoelstra:
ACK 83d071e54be0bc4ebd760a490a3ca887c0bf90a8; successfully ran local tests; lgtm
Tree-SHA512: 6f08c6cda91a7a870f1080b497f89607ac3d6b3c0234cbd2ba2da8710d46816398acac0bca2a49a3bc9466b814ae446842d3d304a3735df9f983e3ff5df005db
ce19d40a80 chore: fix some typos in comments (looklose)
Pull request description:
fix some typos in comments
ACKs for top commit:
apoelstra:
ACK ce19d40a805e2b67f6199cce61a73c6592019b5c; successfully ran local tests
tcharding:
ACK ce19d40a80
Tree-SHA512: 4bf5d339e4e73a6f1378881ad62b17f78796e6000f24eea6b3a381d3e03f4fd8b9775710a7eb5a087bfe0d557d1ab1a8291dcc7b86a2c9ded3318774cf2e5b68
I took a look at the rendered HMTL of `bitcoin`, `primitives`, `units`,
`serde`, and `tokio` and picked a header style that I thought looked
good.
Use it for `primitives` and `units`.
It is semantically valid to divide an amount by another amount. The
result of the operation is an integer.
Note that we cannot implement `Div` by `NumOpResult` because there is no
way to show the div by invalid case.
Implement `Div` by amount for both amount types.
Add a few macros to test `Add` and `Sub` impls for both amount types,
all combos of type and res (eg `Amount` and `NumOpResult<Amount>`), and
all combos of references.
bb8f833ca0 Update instruction.rs (kilavvy)
0ce622e668 Update message.rs (kilavvy)
f61941bbe6 Update serialized_signature.rs (kilavvy)
1d2de62e01 Update mod.rs (kilavvy)
Pull request description:
This PR fixes several typos in comments across multiple files:
- Fixed typo `interpretted` -> `interpreted` in `blockdata/script/instruction.rs`
- Fixed typo `neccessity` -> `necessity` in `p2p/message.rs`
- Fixed typo `underlflow` -> `underflow` in `taproot/serialized_signature.rs`
- Fixed typo `ambigous` -> `ambiguous"` in `units/src/amount/mod.rs`
These changes only affect comments and documentation, no functional code changes.
ACKs for top commit:
apoelstra:
ACK bb8f833ca01688eaae75e0fa322f698d34243185; successfully ran local tests; though all these commits could be squashed IMO
Tree-SHA512: d73dc2a86b20de87c0c5efb3e5042e3901c846236670e3a6501f4c93fd54328fef08bfeca276b93642e7b51d04cb8b9c8e1af558f3aabc3c924d60a61e58b031
8787304425 units: Improve code comment on macros (Tobin C. Harding)
Pull request description:
We have two macros that are hidden because they are code de-duplication tools. However the output they produce is, and has to be, stable so that we can use them in `units` and `primitives` without inadvertently breaking semver in `primitives`.
ACKs for top commit:
apoelstra:
ACK 8787304425c83cccfb1107bde0b0d82901470d38; successfully ran local tests
Tree-SHA512: 564bb27dcb608f391be8e50cbf2da02d2f339ddf575e5b5fd432a93df96233b50541616167c7c84a134ed31455a64149c822aea72e61c042fb1aeb8fab20d4ce
Andrew Poelstra
Tobin C. Harding
Fmt Bot
Erick Cestari
yancy
Shing Him Ng
Jamil Lambert, PhD
kilavvy