29f1a4613a Enable `ref_option` lint and allow individually (Jamil Lambert, PhD)
Pull request description:
The last `TODO` in #3825
Allow the `ref_option` lint in the cases where the lint wants us to change the API to ```use `Option<&T>` instead of `&Option<T>` ```
The other option would be to keep the lint as `allow` and add a comment to `Cargo.toml` as to why we are allowing it like the other cases that are allowed.
Close#3825
ACKs for top commit:
apoelstra:
ACK 29f1a4613a090cd23baded1ef577aa4e3da8d33f; successfully ran local tests; nice!
tcharding:
ACK 29f1a4613a
Tree-SHA512: 181d617060e8ae0e75f033b1f356106fc89bdee9841085c889b775e82711c147e5282ecb0e3bf80cdb058034365bbfc7289c029ba0bf8b0b0f26d22505c63aa2
b11ace359a Fix up ParsePublickeyError (Innocent Onyemaenu)
Pull request description:
Resolves#3835
In #945fcd09 we forgot to Update impl for ParsePublicKeyError Display and Error traits
ACKs for top commit:
tcharding:
ACK b11ace359a
apoelstra:
ACK b11ace359a52d9137b8fe5919d9825d61e3e9fad; successfully ran local tests; thanks!!
Tree-SHA512: 5c4e5e113605bc5b9c3c0d2ca65a0fdae80726a0f2f791255cc1d6567812a9132ff4f5bd8f30276fef916469d5fd29f6d72ddf4adb6340e52f4e0198ac4fb371
e316e6e719 Fix `missing_panics_doc` clippy lint in units (Jamil Lambert, PhD)
Pull request description:
Change the units `missing_panics_doc` clippy lint to `warn` and allow for the functions that can't panic.
One of the TODO lints in Issue #3825
ACKs for top commit:
tcharding:
ACK e316e6e719
apoelstra:
ACK e316e6e7195993b7dd0e005d2f22f29dfea023c5; successfully ran local tests
Tree-SHA512: 3db74e499dc590a5158869d1a80613d37b3fb09f2d428918dc23b9f1521496d8afff63988f022d6cfd7688d33295f976effe73adbadd8da93dae8fb5d2d13dc5
In #3847 we added an `InvalidCharError` into one of the variants of
`ParsePublicKeyError` but we forgot to update the trait
implementations.
Fix the `error::Error` and `Display` implementations for
`ParsePublicKeyError`. While we are at it match on `*self` as is
typical in this codebase.
With this applied #3835 is fully resolved.
Close: #3835
56286265a5 api: Run just check-api (Tobin C. Harding)
39523ea1f5 units: Remove InputString from the public API (Tobin C. Harding)
Pull request description:
Currently `InputString` is in the public API of `units` because of the trait bound on `parse::int()`. We can just do the monomorphisisation manually to remove it.
This patch renames `int` to have three different names, one for `&str` one for `String`, and one for `Box<str>` e.g., `units::parse::int_from_str`.
Close#3708
ACKs for top commit:
apoelstra:
ACK 56286265a55368572c61b03bcb81fe04fb7921c0; successfully ran local tests; this looks great!
Tree-SHA512: 5c16640fe4651fbbafd5e3558d8918414df1bb1579ca64b2256f3c10df410481ae29a77ab89f7a1571bfdd710dc6c6bd8ee9217f2c54eeef06e21ab6ce4aa735
Currently `InputString` is in the public API of `units` because of the
trait bound on `parse::int()`. We can just do the monomorphisisation
manually to remove it.
This patch renames `int` to have three different names, one for `&str`
one for `String`, and one for `Box<str>`.
668056fb36 Use Amount in examples (Tobin C. Harding)
Pull request description:
Use the `Amount` type as parameter in examples code instead of a `u64`.
Done as part of preparation for enforcing MAX_MONEY.
ACKs for top commit:
apoelstra:
ACK 668056fb36c87bb2eeeacc8fa6c44ed942c558b3; successfully ran local tests; good catch
Tree-SHA512: bbc35a2ae0313bcd0c6d6c4e22b855078ab71aa0df58f7f24e30839e66ba11a78440dfc4f15c5b38dfe3be1dbaa29ff7c2fd1dacf87dc6c73e42960940cc5618
ac59b25f2c io: Add unit tests for Take (Tobin C. Harding)
Pull request description:
Add two unit tests:
- Check we can read into an empty buffer as validation of args as part of C-VALIDATE
- Do basic read using `Take::read_to_end` since it is currently untested.
ACKs for top commit:
jamillambert:
ACK ac59b25f2c
apoelstra:
ACK ac59b25f2cfe8d65435ea9e6f4265e52ac06e92e; successfully ran local tests
Tree-SHA512: 19dcedaec05f0f8c028c59b5eb8771568a3708cb35793db9fadcc96147c00053ddc8239e0cad6eef5dd008bfb743bd0854e7b7674d6f77872a968bb00e928925
e8b157317e Automated update to Github CI to rustc stable-1.84.0 (Update Stable Rustc 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 e8b157317e
Tree-SHA512: 89bbae0772f2d40fbfc8923b30781ab1fe339357f394e2abd64d3e10dc0c2918753fff3a3dd3e6f36ab0f531c6227346aa4b27fb9ba4002fb07c0e0404155b62
Add two unit tests:
- Check we can read into an empty buffer as validation of args as
part of C-VALIDATE
- Do basic read using `Take::read_to_end` since it is currently
untested.
a431a13c24 Automated update to Github CI to rustc nightly-2025-01-10 (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 a431a13c24
Tree-SHA512: 390ba1676cb1532e6ef355e505f11989176b75bb56ad6ae39ed40973a70990fd073a9ac7c5835d39f36855e0ef87d19c092af1d9ff31f49c441e58e1561238a2
1e0c065740 typo: Address::is_valid_for_network (Jose Storopoli)
Pull request description:
I believe this is a typo.
ACKs for top commit:
apoelstra:
ACK 1e0c065740266a8ad207a95f4d964394ba3f07d7; successfully ran local tests
tcharding:
ACK 1e0c065740
Tree-SHA512: aff71b91dc707c0c6c723e35510d284fd9a60895b7f12828819eb0d4520216491a2cf054b59872ac4be175d566674a32bde8c170f43beb6fc15dd19e6be12e45
706a135de6 bitcoin: Add an example of doing I/O to encode/decode (Tobin C. Harding)
Pull request description:
In an effort to improve the documentation on `bitcoin_io` add an example in `bitcoin` crate that demonstrates a few things:
- Encode/Decode a `rust-bitcoin` type to/from a stdlib type.
- Encode to a custom type by implementing `bitcoin_io` traits.
- Encode to a foreign custom type by using the `bitcoin_io::bridge::FromStd` wrapper.
Later we can link to this example online in the `bitcoin_io` docs.
ACKs for top commit:
apoelstra:
ACK 706a135de6ca2be0231c2f62f4f2b156e08c9a49; successfully ran local tests
Tree-SHA512: cd3ff4067d5b86031255cb31fe17dea3fd22479699d32efad93c359de465ef6250f29aa4c43ce4218ae623fa6c355661960c11908a729a895655319141b82852
426f585a47 api: Run just check-api (Tobin C. Harding)
6cf90132bc io: Add traits (Tobin C. Harding)
Pull request description:
So that our `io` crate is not surprising it seems we should generally, unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`, and `Sink`. `Take` is correct already, just `Debug`.
Done while investigating C-COMMON-TRAITS
ACKs for top commit:
apoelstra:
ACK 426f585a479ca20b7c3390c589b836f8726b9b03; successfully ran local tests
Tree-SHA512: 0fdacfa0295c36e9ee2bdffd5e649b923f48eeff7baa3afc99fda0836ae30b794610a176c0e76341a268c712baab51139355976c23913a0744692cd1e38a4d11
e315aaf1c6 io: Allow setting position bigger than inner buffer (Tobin C. Harding)
Pull request description:
Currently if one calls `set_position` on a cursor setting the position greater than the total length of the inner buffer future calls to `Read` will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value. Make reads past the end of the buffer return `Ok(0)`.
ACKs for top commit:
apoelstra:
ACK e315aaf1c65225b89435d0a60907a3233caa5db1; successfully ran local tests
Tree-SHA512: 71b151218e96343a86c8b2c21c6e8212e2d1c2aea6517b4662da3ad01b3b598cec497028b863a8e36a6b1fae1d4f7f975c8610c3b6f39f1c256adc5751d06ea0
7a3df57659 Reorder assertions in units::amount::tests to follow got, want order (yhzlsm)
Pull request description:
Reorder assertions in `units::amount::tests` to follow got, want order.
Makes debugging easier, as there's no need to check the test to verify the order.
Close#3860
ACKs for top commit:
tcharding:
ACK 7a3df57659
apoelstra:
ACK 7a3df57659d02ab610e328072435836386ed1c97; successfully ran local tests
Tree-SHA512: 7d07162ba930ca1471684a771cad08ba9153b0b38bf44c98fd41c2d70f05c36b95f022dd82e61e6b50614266f16a5615edd79d6d548b003642509866416021c9
9d1cba4994 units: Introduce fee module (Tobin C. Harding)
cd908fec51 Use explicit calc getters and setters (Tobin C. Harding)
Pull request description:
We have a bunch of functions and impl blocks scattered around the place for calculating fee from fee rate and weight.
In an effort to make the code easier to read/understand and also easier to audit introduce a private `fee` module and move all the code that is related to this calculation into it.
This is in internal change only.
ACKs for top commit:
apoelstra:
ACK 9d1cba4994e9ec94850a0e0e49f85fd0c595541e; successfully ran local tests
Tree-SHA512: a33c1ce4a1b62ff29ee65dd3adf2f19384a77f7e18f1c42019973631726cd710c2c8d9c200afb108d4f3f34fcce5cd5383a7ae512caf76c73604db9cdf9eaeda
e09bdb5f98 Add BIP324 V2 p2p network message support (Nick Johnson)
Pull request description:
Migrating over the BIP324's library's V2 network message encoding and decoding. As discussed over in https://github.com/rust-bitcoin/rust-bitcoin/discussions/2959, this is probably the natural home for it and also cleans up some gross copy/pasta of some of the encoding/decoding chain logic.
This patch adds `V2NetworkMessage` which wraps a `NetworkMessage`, but handles the V2 encoding and decoding. It is a parallel of the existing `RawNetworkMessage` (which mentioned before, may be better described as `V1NetworkMessage` https://github.com/rust-bitcoin/rust-bitcoin/issues/3157). A priority of this patch was to not re-invent any wheels and try to use the existing patterns as much as possible.
ACKs for top commit:
tcharding:
ACK e09bdb5f98
apoelstra:
ACK e09bdb5f98ea516382a04283373ad97a41d57c2b; successfully ran local tests; nice!
Tree-SHA512: a5078d4d3deb04c2e06ea513bbc8c97d0e6d5da5b029847a97b3f90bf55a263858dd16d88299f853aa3c468f7b9bceb3973c5652a49d3e96df3e91181b455f29
29a71de928 Bound Address parsing on NetworkValidationUnchecked (Tobin C. Harding)
cf455d3a06 Fix typo in prifixes (Tobin C. Harding)
Pull request description:
Currently it is not possible for downstream to use a generic on the `Address` type in structs in conjuncture with
derives (`serde::Deserialize` and `Display`) because our impls are only done for `NetworkUnchecked` (as they should be).
However, as observed by dpc, if we add a secondary marker trait and use it to bound the impls, implementing the new marker for `NetworkUnchecked` then downstream can use derives by way of
```
#[derive(Serialize, Deserialize)]
struct Foo<V>
where V: NetworkValidation,
{
#[serde(bound(deserialize = "V: NetworkValidationUnchecked"))]
address: Address<V>,
}
```
This is cool as hell because the `Address` type is currently a royal PITA.
Patch 1 is trivial cleanup.
To get past a build error in `FromStr` I used this little trick
```rust
// We know that `U` is only ever `NetworkUnchecked` but the compiler does not.
Ok(Address(address.0, PhantomData::<U>))
```
Resolve: #3760
and
Close: #3856
ACKs for top commit:
apoelstra:
ACK 29a71de92817bccd49b42b1055cc570832e6b959; successfully ran local tests
Tree-SHA512: 7c158dddb9fdbaaa1e48204bbf915b18ced56f5d82ce82630db6c0b52161bcf43b3ac413fa990a23975743c56917985b2666a74f9067221f003f2dcf080f827e
96f427a8b8 units: Refactor Send/Sync api test (Tobin C. Harding)
Pull request description:
The `api` test for types implementing `Send` and `Sync` is part of both C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the two tests into a single one and document appropriately. This is mirrors how we do it in `io/tests/api.rs`.
ACKs for top commit:
apoelstra:
ACK 96f427a8b84129179e86f3914be8e4712a89f660; successfully ran local tests; lgtm
Tree-SHA512: 7b24780ac2b4f73d0cad952555f005553d9b8c248da6f92c28e7e9510b58eba6c165720ded9bd2f2db19f9a19d72fe7dd333e68312f1291a47e044a94902be0b
So that our `io` crate is not surprising it seems we should generally,
unless there is a good reason not to, follow `std::io`.
Copy the derived trait implementations from `std::io` for `Cursor`,
`Sink`, and `Take`.
Done while investigating C-COMMON-TRAITS
Reorder assertions in `units::amount::tests` to follow got, want order
Makes debugging easier, as there's no need to check the test to verify the order.
316d8bcb01 Change all occurrences of "IO" to "I/O" (Jamil Lambert, PhD)
Pull request description:
Fixes#3871
ACKs for top commit:
tcharding:
ACK 316d8bcb01
apoelstra:
ACK 316d8bcb01504420a14854d2be122d1c8cffb4a9; successfully ran local tests; lgtm
Tree-SHA512: 437a95a1c36bcd4ae27aaacdfc5e0f3463e522a222c4a6ef2c3e234be4a24be2b600687bd58b300bf2b0a0d6596ab008f60903c91646458228eb34cf510908d6
2f91092d77 Derive Clone on consensus errors (Tobin C. Harding)
Pull request description:
All error types in the repo use either [0]:
- `#[derive(Debug, Clone, PartialEq, Eq)]``
- `#[derive(Debug)]`
However the `consensus` error types do not have `Clone` derived.
Derive `Clone` on `consensus::ParseError` and `consensus::Error`.
[0] Excluding `PushBytesError`, fixed in #3879
ACKs for top commit:
apoelstra:
ACK 2f91092d773e1618314b62c9e3eae17c08e1f5ad; successfully ran local tests; lgtm
Tree-SHA512: 2cae6faae97b63de311538c33eb7d1914330672744ca073bc6ea5c11ebd40a40ff3de9a38f1f5b101ff27798416d685a708599c298a6f82d7f5804575c2cb2c1
0870cd1660 Remove Copy from PushBytesError (Tobin C. Harding)
Pull request description:
The `PushBytesError` is the only error type in the codebase to derive `Copy`. Without thinking too hard this is unusual - remove it.
Thinking a bit harder it makes the code less maintainable because we must commit to implementing `Copy`.
ACKs for top commit:
apoelstra:
ACK 0870cd1660edd21739cc94075e4b3a1c7f1a7d15; successfully ran local tests; lgtm
Tree-SHA512: c71db5de634dfe2bd76336e5c31fab496f2a472a8dd164034233544c15bd89c84ff986e476fa9b7b05d01aa5332dd4bc93f63a93bf7a21e9a0ec67fc145739b2
Currently if one calls `set_position` on a cursor setting the position
greater than the total length of the inner buffer future calls to `Read`
will panic. Bad `Cursor` - no biscuit.
Mirror the stdlib `Cursor` and allow setting the position to any value.
Make reads past the end of the buffer return `Ok(0)`.
a7c44cebf9 Use _unchecked to construct amounts (Tobin C. Harding)
09df951760 Use sat variable in tests (Tobin C. Harding)
4a5b2c60c6 Use ssat variable in tests (Tobin C. Harding)
Pull request description:
We have a `_unchecked` constructor now for both `Amount` and `SignedAmount`. Soon we would like to start enforcing the `MAX_MONEY` invariant in both amount types. To make that change easier do a few refactorings:
- Patch 1 and 2 introduce local variables for amount constructors.
- Patch 3 replaces the local variables introduce in (1) and (2) with macros
- Patch 4 uses `_unchecked` constructor for hard coded integers
The strange patch separation is done intentionally so we don't inadvertently reduce test coverage by using the wrong constructor. I made this mistake already in a previous PR, lesson learned.
Note please, the macro introduced in patch 3 is in preparation for enforcing `MAX_MONEY`. The macros allow us to panic (`from_sat().unwrap()`) instead of using the `_unchecked` version.
ACKs for top commit:
apoelstra:
ACK a7c44cebf9975c4eeba56a65c0ea65be90e5c7f3; successfully ran local tests
Tree-SHA512: 55c2428ae231882542a4cfa724675341f7b493d158f4bec26277d3eefb04d9597cc29b05dce859661a96855fa6f4bac250d53c3dfa9f86a9611d43387ee18667
The `api` test for types implementing `Send` and `Sync` is part of both
C-SEND-SYNC and also C-GOOD-ERR (for error types).
Refactor the Send/Sync tests in both `units` and `io` and improve
comments.
9396041524 api: Run just check-api (Tobin C. Harding)
ffd8702cb3 units: Hide the remaining public macros (Tobin C. Harding)
Pull request description:
We do not want to commit to any public macros in `units`. Recently we (I at least) learned that adding `doc(hidden)` signals to users that the macro is unstable and should not be relied upon.
Hide the remaining two macros so we can release 1.0 and not worry about later breaking them.
With this applied there are no exported macros that are not hidden. Verify using `git grep -A 1 macro_export`.
ACKs for top commit:
apoelstra:
ACK 9396041524e291203d5c86665639872f9a6246b5; successfully ran local tests; Yeah, let's do it
Tree-SHA512: a3a59897a2fe16276ab2d364ff247f48772a63a25f91eabc17023a37b9fab3860639dc1e09193c938dd73711ba20c95b8d0ad9db9493d269ee9328b2132d61cb
All error types in the repo use either [0]:
- `#[derive(Debug, Clone, PartialEq, Eq)]``
- `#[derive(Debug)]`
However the `consensus` error types do not have `Clone` derived.
Derive `Clone` on `consensus::ParseError` and `consensus::Error`.
[0] Excluding `PushBytesError`, fixed in #3879
The `PushBytesError` is the only error type in the codebase to derive
`Copy`. Without thinking too hard this is unusual - remove it.
Thinking a bit harder it makes the code less maintainable because
we must commit to implementing `Copy`.
In an effort to improve the documentation on `bitcoin_io` add an example
in `bitcoin` crate that demonstrates a few things:
- Encode/Decode a `rust-bitcoin` type to/from a stdlib type.
- Encode to a custom type by implementing `bitcoin_io` traits.
- Encode to a foreign custom type by using the `bitcoin_io::bridge::FromStd` wrapper.
Later we can link to this example online in the `bitcoin_io` docs.
55a999e0b5 Add serde roundtrip tests to relative locktime types (Tobin C. Harding)
Pull request description:
As we do for absolute locktime types add a couple of `serde` roundtrip tests to the relative locktime types.
ACKs for top commit:
apoelstra:
ACK 55a999e0b5c5867ed66ee64d2b516b80e059e402; successfully ran local tests
Tree-SHA512: 28923a0e2c2053a7346e91a21619dc5d2700ea131aa5ec5a5f6d89d09c70cb45ce9731055bbcc1447d46d5dbe231cb075436dd682229ee8530307e199af54ce2
We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.
In an effort to make the code easier to read/understand and also easier
to audit introduce a private `fee` module and move all the code that is
related to this calculation into it.
This is in internal change only.
We have a bunch of functions and impl blocks scattered around the place
for calculating fee from fee rate and weight.
In preparation for adding a `calc` module use getters and setters in
code that will move to the `calc` module.
(Remember, the `FeeRate` uses an inner sats per kwu value.)
Internal change only.
0e5d6db7df units: Don't ignore serde examples (Tobin C. Harding)
Pull request description:
There is no reason to ignore the rustdoc example in `units::amount::serde`. The `as_btc` function requires the `alloc` features so use `as_sat` in the example instead.
While we are at it clean up the whole block of rustdocs.
ACKs for top commit:
apoelstra:
ACK 0e5d6db7df9a1f9b4e9180d49cf1532551570e33; successfully ran local tests
Tree-SHA512: d108779faa8baf5c8a1cb533e44e8ac209e3e1001eb8ec8ae1f9ba9b0cf5f8c45c38df77e15193cdc990fcd0fbf48bca489aadbd196c6323560040b7fd043332
a18d28761b api: Run just check-api (Tobin C. Harding)
1175faca32 io: Introduce api test file (Tobin C. Harding)
01f3eefc12 io: Move bridge type constructors (Tobin C. Harding)
a8fbd5e302 units: Remove api docs (Tobin C. Harding)
aaa20fc098 units: Move code comment to correct imports (Tobin C. Harding)
Pull request description:
As we did for `units` add an integration test for the API surface of the `io` crate.
The first two patches are cleanup to the `units` test. Patch 3 is trivial refactor.
ACKs for top commit:
apoelstra:
ACK a18d28761b9f6ce3bd06ba72b9afe11773b7443d; successfully ran local tests
Tree-SHA512: e5a39f97f7d6e34cb63613f337101f13fcc5d66f85277b6f0026d339b5677ff60f7e3050ae23807c3d20d0ae4e343a10983f43dab757a66fe4d33afd09a96f62
BIP324 introduced the second version of p2p network messages which
are designed to be used with the new encrypted transport. This patch
adds a V2NetworkMessage type which wraps a NetworkMessage and handles
the V2 encoding and decoding. It is a parallel of the existing
RawNetworkMessage type (which may be better described as
V1NetworkMessage). A priority of this patch was to not re-invent any
wheels and try to use the existing patterns as much as possible.
merge-script
Jamil Lambert, PhD
Innocent Onyemaenu
Tobin C. Harding
Fmt Bot
Jose Storopoli
yhzlsm
Nick Johnson