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 new unsized type is more flexible and so are the references to it.
Just like we pass around `&str` instead of `&String` we should be
passing `&TaprootMerkleBranch` instead of `&TaprootMerkleBranchBuf`.
`TaprootMerkleBranchBuf` previously derefed to a slice which lost the
information about length being valid. This commit changes the type
which, while API-breaking, is not disruptive because the type has API
very similar to slice.
`TaprootMerkleBranchBuf` being a vec introduced intermediate allocation
when creating or decoding `Witness`. However the representation on the
wire is the same as in-memory (aside from `#[repr(transparent)]`) so
this allocation wasn't really needed.
This commit introduces `TaprootMerkleBranch` type which is unsized and
can be used in place of `TaprootMerkleBranchBuf` within `ControlBlock`.
Aside from removing the intermediate allocation, this improves the API a
bit: the conversion from array to other type is no longer needed because
it's performed by `ControlBlock` in its methods. Thus, consumers who
have an array can simply set it as `merkle_branch` field and then encode
the `ControlBlock` into witness. A convenience method is also provided
to push the `ControlBlock` along with other parts at the end of the
`Witness`.
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.
f4f79f88eb Enable getting the network kind from an address (Tobin C. Harding)
Pull request description:
Users may wish to ask of an address 'what kind of address is this?' We have the `NetworkKind` struct that abstracts over the answer but currently no API to ask the question.
The address may have been parsed or constructed and weather the network has been checked already is immaterial. Hence we add the function for both `NetworkChecked` and `NetworkUnchecked` addresses.
Fix: #4247
ACKs for top commit:
apoelstra:
ACK f4f79f88eb2c6c80c46c95c69fcc43b17d306be2; successfully ran local tests
Kixunil:
ACK f4f79f88eb
Tree-SHA512: 57bdf7a0f2ae8bf599b3830d10201af3f6312a802ab72c0d86e346af660cbc4f430954e46d6698032a062514ec3ee1ee7edc732beff79af99a84ce718a519afa
Users may wish to ask of an address 'what kind of address is this?' We
have the `NetworkKind` struct that abstracts over the answer but
currently no API to ask the question.
The address may have been parsed or constructed and weather the network
has been checked already is immaterial. Hence we add the function for
both `NetworkChecked` and `NetworkUnchecked` addresses.
Fix: #4247
cacf734ccf Automated update to Github CI to cargo-semver-checks version-0.40.0 (Update cargo-semver-checks Bot)
Pull request description:
Automated update to Github CI workflow `semver-checks.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK cacf734ccf
Tree-SHA512: 7fb8c6b5c5d8614d39ee14b961f29371b135596cbbe6930183495baf81fa6dcaf53866f853986d5f924c0e2fbd0e7c06193bb65e5249d64d82c373e27644c064
058e8285f0 Automated update to Github CI to rustc nightly-2025-03-14 (Update Nightly Rustc Bot)
Pull request description:
Automated update to Github CI workflow `rust.yml` by [create-pull-request](https://github.com/peter-evans/create-pull-request) GitHub action
ACKs for top commit:
tcharding:
ACK 058e8285f0
Tree-SHA512: d3211e78dbb5717b9369a7d331f07d7c2c3560e24de8ffadf6c60170f60ffbb4a9991e5dba174ddb8e0cb11bbbfed91737b2fda04b7d199f383050a26ff442e1
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
c1ea20b958 Increase test coverage in block.rs (Jamil Lambert, PhD)
3e750d7ba1 Create test helper function to create a header (Jamil Lambert, PhD)
Pull request description:
Add tests to `primitive::block` module to increase test coverage.
Coverage tested with `cargo-tarpaulin`
`block.rs` coverage:
before 13.79%
after 100%
ACKs for top commit:
apoelstra:
ACK c1ea20b958f448a5a35073ce3d5bd5394918eb9b; successfully ran local tests
tcharding:
ACK c1ea20b958
Tree-SHA512: 70bbdce9c45e9bdcea76026b7a27582a32b796099329c429527d7dfe574788604020e53e417afd3cf53886ceb87df37b6dcb34a7ff40260b9d8a3811e22a478c
c0e20dbf2e test: add coverage for ServiceFlags::P2P_V2 (Bruno Garcia)
Pull request description:
Include `P2P_V2` on `service_flags_test`
ACKs for top commit:
tcharding:
ACK c0e20dbf2e
apoelstra:
ACK c0e20dbf2e1406a3f3df5538d1397b233078cd7c; successfully ran local tests
Tree-SHA512: 6ebb2f493bcc8fc6643cec67945a81692f03ceb976d75e10ce6052e775901846b9795f31817439b4484ef2d1eaae122d7da270cac2b59d7bb41cb2015593d1f5
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
8f74b823ab Add validation for private key format and master key constraints (Erick Cestari)
Pull request description:
This PR addresses issue #4195 by adding proper validation when decoding extended private keys:
### Changes
- Add validation to ensure byte 45 is zero as required by BIP-32 specification for private keys
- For master keys (depth=0), add validation to ensure parent fingerprint is zero
- For master keys (depth=0), add validation to ensure child number is zero
- Add corresponding error types to handle these validation failures
- Add unit tests to verify each validation rule
### Validation Rationale
These checks improve security by rejecting malformed extended keys that could potentially lead to unexpected behavior. As noted in the issue discussion, these validations are explicitly required by the BIP-32 specification.
### Testing
Added three new unit tests to verify each validation rule:
- test_reject_xpriv_with_non_zero_byte_at_index_45
- test_reject_xpriv_with_zero_depth_and_non_zero_index
- test_reject_xpriv_with_zero_depth_and_non_zero_parent_fingerprint
Fixes#4195
ACKs for top commit:
jrakibi:
ACK 8f74b823ab
tcharding:
ACK 8f74b823ab
apoelstra:
ACK 8f74b823ab8ef44bde7d003f8ba43fbe44dbef3e; successfully ran local tests
Tree-SHA512: 6a013e4917f83cfd7e39a2a18f7491853d791ab1d981a99eeea6204e1dab723fed7a168ff2a89e8850d512c3c381bfa1afef7fa32e5a0d246d949a46b01a3023
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
e4513bf925 feat: add MAX_BLOCK_SERIALIZED_SIZE existing in core (ChrisCho-H)
Pull request description:
fad0d9ea2d1e807806fa141238e279fddea6ae99: add `MAX_BLOCK_SERIALIZED_SIZE` as constant, which also exists in [bitcoin-core](59ff17e5af/src/consensus/consensus.h (L13)).
I originally thought it would be better to use this value for checking limit of push_bytes [here](0870cd1660/bitcoin/src/blockdata/script/push_bytes.rs (L31)), as it's the actual limit(`OP_PUSHDATA4` semantic says it could allow up to 4GB though). However, I'm not sure whether there might be need to push_bytes larger than `MAX_BLOCK_SERIALIZED_SIZE`, so just let developer use this constant to check the actual limit rather than enforcing it.
ACKs for top commit:
tcharding:
ACK e4513bf925
apoelstra:
ACK e4513bf9250799bc18a10728af184d6c86a561a4; successfully ran local tests
Tree-SHA512: 44c5a4882666ad286c1e1c40b9738929e2a8ad4bb44aaf48865fc395291185ae5aae351d26ac9334671e47a11e844bd037bd251a921b6b028a116d1b442b9183
5d851f1c3e Remove deprecated amount methods (Tobin C. Harding)
76a2d70b28 Make mul weight by fee return NumOpResult (Tobin C. Harding)
f9eb307953 Remove panic in dust value functions (Tobin C. Harding)
13595fbe7d Fix amount whole bitcoin constructors (Tobin C. Harding)
ac71680202 Pick one - MAX or MAX_MONEY (Tobin C. Harding)
6d70c77cf9 Enforce newtype sanity rules for amount types (Tobin C. Harding)
e6f7b26d80 Use _unchecked in amount const types (Tobin C. Harding)
ef0af8d62e Use sat/ssat constructors throughout tests (Andrew Poelstra)
8ecdc7c275 Use den_ prefix for local Denomination variable (Tobin C. Harding)
938461cc65 psbt: Use Amount::ZERO in unit test (Tobin C. Harding)
Pull request description:
We want to start enforcing MAX_MONEY as an invariant in the amount types. There are a few more steps we can do first to make that change easier to review.
ACKs for top commit:
jamillambert:
ACK 5d851f1c3e
apoelstra:
ACK 5d851f1c3e98d7d426e5897b2d734b77a299ccfb; successfully ran local tests
Tree-SHA512: 9e28b273d41fc143656e3a84736b6abe477fae5721b02bce7436551bd489cc235dc7e9fc68ffafa98f75a61065470ac514570a42bea94e90fedbb31f3cd61031
199f57849a Remove references to cfg(mutate) from lint allow - no longer allowed (AM)
a65d1d8b95 docs: Update README to replace use of mutagen with cargo-mutants (AM)
Pull request description:
Hey there!
I am just getting up to speed with the project and in following the README discovered that there are still references to the previous mutation testing tool `mutagen`. I updated the README to refer to the new tool, `cargo-mutation`.
I'm suggesting the user use the same command, `cargo mutants --in-place --no-shuffle`, as is run in the weekly CI workflow.
I noticed that there are still references to the old `mutate` attribute in the following files. I removed these as well as per [feedback](https://github.com/rust-bitcoin/rust-bitcoin/pull/4228#issuecomment-2709407253).
`primitives/Cargo.toml`:
```
[lints.rust]
unexpected_cfgs = { level = "deny", check-cfg = ['cfg(mutate)'] }
```
and
`bitcoin/Cargo.toml`:
```
unexpected_cfgs = { level = "deny", check-cfg = ['cfg(bench)', 'cfg(fuzzing)', 'cfg(kani)', 'cfg(mutate)'] }
```
Update to PR: removed incorrect understanding of logic in PR description as per [feedback](https://github.com/rust-bitcoin/rust-bitcoin/pull/4228#issuecomment-2709408598) and removed `cfg(mutate)` from above 2 files.
ACKs for top commit:
tcharding:
ACK 199f57849a
apoelstra:
ACK 199f57849acd9845902a8090ad6490a61ee03d24; successfully ran local tests
Tree-SHA512: e154c504aa5283f1da05d0120ea8dda97d1159389e692b0d57d7d864032ecb2b48c496054ede5500477367bc732dc34b0870f2709b8bd6e7b3f5c18a10f7a29e
a824b79206 docs: The quotation marks are incorrect. (RiceChuan)
Pull request description:
docs: The quotation marks are incorrect.
ACKs for top commit:
tcharding:
ACK a824b79206
apoelstra:
ACK a824b79206edb79fc990ff0022e10f4a5ebe4cb3; successfully ran local tests
Tree-SHA512: b5893fa564af72b31c0893236f29751702764e20e8f05576861e9a47b96c216066838bd48e38277ee1acbdb983a55edc64f9c2466826546f65d70ba309839927
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.
Calculating the minimum non-dust fee currently panics if either the
script is really big or the dust fee rate is really big.
Harden the API by returning an `Option` instead of panicing.
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.
Andrew Poelstra
Martin Habovstiak
Tobin C. Harding
Jamil Lambert, PhD
planetBoy
healthyyyoung
Fmt Bot
Erick Cestari
kevincatty
Bruno Garcia
ChrisCho-H