9a572dabde refactor: use path dependencies for workspace members in bitcoin/Cargo.toml (Eval EXEC)
Pull request description:
This PR want to:
1. make all workspace members use `workspace = true` syntax to import dependencies.
2. use `path = [balabala]` to define dependencies, instead of useing `[patch.crates-io.balabala]` , fix: https://github.com/rust-bitcoin/rust-bitcoin/issues/4283
ACKs for top commit:
Kixunil:
ACK 9a572dabde
apoelstra:
ACK 9a572dabdeb077f96b2ab66be1a80fcec3e805e3; successfully ran local tests
Tree-SHA512: 834ef881ed3fd324a9ecca440e8e591984a7e474eb6aeab86a0301cbd08b6dc96ecdc34b306ad146b11b50f7488024c289b8f8c7c6de1a2bdba7aec515b722ee
3ae21d5111 Use impl_add/sub_assign for block interval (Tobin C. Harding)
9d55922952 Use impl_op_for_references for block height/interval (Tobin C. Harding)
f5e17914b6 Move Assign impls together (Tobin C. Harding)
cc66838e80 units: Remove unnecessary code comments (Tobin C. Harding)
Pull request description:
Improve the ops impls in the `block` module using the already present macros.
ACKs for top commit:
apoelstra:
ACK 3ae21d5111444f5e01f6cfb1a2b9b314f66418a3; successfully ran local tests; nice!
Tree-SHA512: 6565426a06bb47d337d21cf5c59acca43e69228dbec8319fc95373025d220d8ec6273c54f214f312c4229603c455d08e4c6a8c108663c6db5086df36266979de
e744347022 Make usage of Self and type uniform across both modules (Erick Cestari)
dfb49f014c Rename impl_try_from_array to impl_from_array (Erick Cestari)
Pull request description:
This PR makes two main changes:
1. Standardizes the function signatures in the `Amount` and `SignedAmount` implementations by consistently using `Self` as the return type instead of the concrete type names. This improves code consistency, maintainability, and follows Rust's idiomatic practices.
2. Renames `impl_try_from_array` to `impl_from_array` to better reflect its functionality.
### Changes
**Consistent usage of Self instead of concrete types**
- Replace all occurrences of `-> Amount` with `-> Self `in unsigned.rs
- Replace all occurrences of `-> SignedAmount` with `-> Self` in signed.rs
- Make similar replacements for Option/Result return types
- Use `Self::` instead of explicit type name for static method calls
**Function rename**
Renamed `impl_try_from_array` to `impl_from_array` for better clarity
### Related Issues
Closes#4210Closes#4241
ACKs for top commit:
Kixunil:
ACK e744347022
tcharding:
ACK e744347022
apoelstra:
ACK e744347022d1ad1e0ca0a83ec9350501af08297b; successfully ran local tests
Tree-SHA512: 3113f3ccf595b298afe6b23514f1de790284df7fcb55a13658aabe3ef4fcea0e401b65b0a2c67ac18da87a1bcd247bd1f1484856fe03470b98dfa2614958a3bb
This commit standardizes the function signatures in the Amount and SignedAmount
implementations by consistently using Self as the return type instead of the concrete
type names. This makes the code more consistent, easier to maintain, and follows Rust's
idiomatic practices.
Changes:
Replace all occurrences of -> Amount with -> Self in unsigned.rs
Replace all occurrences of -> SignedAmount with -> Self in signed.rs
Make similar replacements for Option/Result return types
Use Self:: instead of the explicit type name for static method calls
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`.
We have a new macro for implementing ops with a bunch of reference
combos. Lets use it for block `Height` and `Interval`.
This patch is strictly additive.
merge-script
Eval EXEC
jike
Erick Cestari
Tobin C. Harding
Fmt Bot
yancy
Shing Him Ng
Jamil Lambert, PhD