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.
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.
a273814d23 Replace underflow with overflow in doc comments (yancy)
2f897e2109 Remove warning section (yancy)
Pull request description:
The use of underflow is misleading. Adding one to MAX and subtracting one from MIN are both considered an overflow.
Note I tried to keep to 80 column line length so a paragraph needed some shuffling.
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/4187
ACKs for top commit:
Kixunil:
ACK a273814d23
tcharding:
ACK a273814d23
apoelstra:
ACK a273814d23f7f804f60bc260e6939a75ed9f1790; successfully ran local tests
Tree-SHA512: 3d1fd3a6c3e11694d189086b6f9e14d55b912497dca8bac2153cc54afbbbee527cae354e60781f8b61591b13aa85fb9e490cea2e7aed3dd9d3e4335502ffb84b
0428554585 primitives: Feature gate import (Tobin C. Harding)
Pull request description:
Feature gate the `Infallible` import. Found with `clippy`.
ACKs for top commit:
apoelstra:
ACK 0428554585de668e2a59fa61e44b473f4085d717; successfully ran local tests
Tree-SHA512: 23bf7d62d81af87b67395ddee251cef82140895be76026c43c774997c29f2ed2707bf42e82d0a155a5cff83412538cb2ed5251f10c0e0cb746434a8d7db8e554
This commit adds additional validation checks when decoding extended private keys:
1. Verifies that byte 45 is zero as required by BIP-32 specification
2. For master keys (depth=0), ensures parent fingerprint is zero
3. For master keys (depth=0), ensures child number is zero
These checks improve security by rejecting malformed keys that could
potentially lead to unexpected behavior. Added corresponding error types
and unit tests to verify each validation rule.
ddb6dd520e Move module out of fuzz_target directory (Jamil Lambert, PhD)
Pull request description:
#4185 introduced a new file `fuzz/fuzz_targets/bitcoin/fuzz_utils.rs` which is not a valid fuzz target. This causes the daily fuzz workflow to fail on `verify-execution`.
Move the module to the `src/` directory. Create a `lib.rs` file.
ACKs for top commit:
brunoerg:
ACK ddb6dd520e
tcharding:
ACK ddb6dd520e
Tree-SHA512: da05ab92cbb934ee90c2dc11b9edbb95e6ae66cbbeec1481ff2e2d225b43c59a7bf75a22578ca3f62c9fdda2ad195159e8a8d45529948382369a85c8718954b3
df500e9b71 primitives: Enable pedantic lints (Tobin C. Harding)
Pull request description:
Draft to check the subjective ones please, then I'll squash.
ACKs for top commit:
apoelstra:
ACK df500e9b71187fe658da76adafdb3300a51de2ef; successfully ran local tests
Tree-SHA512: 8cc8c9b369a63c1b2b26461e288a818e3b74e0f9b7359c964c1650028d3161db1d79369c74f18e79958873bf4d223ee72fa481708600f0297d79377d97a84dda
02a9efddc4 Automated update to Github CI to rustc nightly-2025-03-06 (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 02a9efddc4
Tree-SHA512: 747376061e7cc8a3a44be33996aafd3b2866c95a71325946b54770354888adc9a5711a38e6766744ba9da9154c7a8b2deb3f8ca3b103fddec2988b839a278c2b
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
2aac5a1f81 Fix some comments (NinaLua)
Pull request description:
I fixed some typos in the comments, please review it.
ACKs for top commit:
Kixunil:
ACK 2aac5a1f81
apoelstra:
ACK 2aac5a1f81a9bb217c4dfb7e45b96188ea60e35b; successfully ran local tests
Tree-SHA512: 50a55451b166189e8ca3d2725ed7bb8ff95a8f1ebef0296c0003414871f1b211e6ffcc3b7225302dd3d6760bfc3f65cf8ed730327ceab60cd55b868ccb0cea9a
d1c758f5a4 Add fee_rate::serde re-export (Tobin C. Harding)
Pull request description:
When we added the `fee_rate::serde` module we forgot to re-export it. This is needed so downstream can do specify serde attributes on struct fields.
```rust
#[serde(with = "bitcoin::fee_rate::serde::as_sat_per_kwu")]
rate: FeeRate,
```
ACKs for top commit:
Kixunil:
ACK d1c758f5a4
apoelstra:
ACK d1c758f5a472a4a67cf9c7afa9ef9c0d793a2e16; successfully ran local tests
Tree-SHA512: 6e6f7879d8a0dab59d79f0e41dd5f9f791b72dfb5a1583d0c87ec04216c0a9c0e5c4fb328b93f5298af47b56d898f48717b1641f51295314423e6a569b4677fe
Fmt Bot
merge-script
Tobin C. Harding
Erick Cestari
Jamil Lambert, PhD
kevincatty
Bruno Garcia
planetBoy
ChrisCho-H
AM
RiceChuan
wgyt
yancy