We want to get rid of this constant, so we replace it in tests with 0
amount, empty script. Notably, the tests were already using it as a
dummy value - the exact amount was irrelevant, so this change doesn't
break anything.
The `encode_signing_data_to_inner` function previously constructed a
transaction internally, requiring a bunch of allocations, which it'd
then consensus-serialize into a writer (hasher). It also used a dummy
`TxOut::NULL` value which we want to get rid of.
To get rid of both allocations and the NULL value we serialize the
transaction on-the-fly. Because the encoding doesn't involve witnesses
it's not too complicated and the consensus encoding will never change so
there are no desync bugs possible. We may later change this to an
abstract transaction though.
New nightly lint warning "called `Iterator::last` on a
`DoubleEndedIterator`; this will needlessly iterate the entire iterator"
Code that gives the warning is correct, allow the lint to remove the
warning.
Update rustc nightly to 2025-01-16
Back in 2022 we elected to use `mutagen` for mutation testing. Since
then `cargo mutants` has progressed to a point where we would now like
to use it instead.
Remove all the `mutagen` stuff and update the lock files.
Close: #2829
The mining reward for the first epoc is 50 bitcoin. For mainnet this is
old news but for regtest it is still relevant.
Add and use a new const `FIFTY_BTC` to the `Amount` type. To keep the
amount types uniform also add it to the `SignedAmount`.
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
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
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>`.
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
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
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
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`.
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.
f94c7185fd Remove usage of impl_from_infallible in crates (Shing Him Ng)
Pull request description:
Fixes#3843
tcharding Copied your commit message from the other `impl_from_infallible` commit 😄
ACKs for top commit:
apoelstra:
ACK f94c7185fdd62e1ed98ed4016486406146c4d4f3; successfully ran local tests; nice!
tcharding:
ACK f94c7185fd
Tree-SHA512: 8c58c2c87f6892855d74a3306e1027a37394961f0a26b7bd88cc1654a190dda37234e7dde51a419dcd2f1bd1dd1ccceec16bbbc6fbdd5418ad21f10531b402b3
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.
We have a `_unchecked` constructor now for both `Amount` and
`SignedAmount`. In preparation for enforcing the `MAX_MONEY` invariant
use the `_unchecked` constructor throughout the codebase to construct
amounts from hard coded integer values.
2c9fda4135 Add parenthesis to explicitly show precedence (Tobin C. Harding)
Pull request description:
Recent clippy nightly update introduces warnings about precedence. While ours are, IMO, clear the lint docs have some cases that are not so I don't think we should ignore this lint. Specifically I could easily miss this one
1 << 2 + 3 equals 32, while (1 << 2) + 3 equals 7
ref: https://rust-lang.github.io/rust-clippy/master/#/precede
Add parenthesis to explicitly show precedence. Refactor only no logic changes.
ACKs for top commit:
apoelstra:
ACK 2c9fda413572e251fe3fd07f679b6c786da137b3; successfully ran local tests; yeah, these all seem reasonable
Tree-SHA512: 67b8cb56c0b271d41db58f9297e4be43410c04bf7bc2308789624084e5e9fee325a2eb3be7e56bd6b3904cf19c7a8b1b426e7adef7f4af18ea8c9d24b77a9db9
d5de89259b Fix typo in MerkleBlock::from_block_with_predicate comment (Jesus Christ)
Pull request description:
- Corrected a grammatical error by adding "are" to the comment in MerkleBlock::from_block_with_predicate.
ACKs for top commit:
clarkmoody:
ACK d5de89259b
apoelstra:
ACK d5de89259b64ed6d4756973073f06172d1d14090; successfully ran local tests
storopoli:
ACK d5de89259b
Tree-SHA512: 55595e8858e90ee5ab5ef7d592cf2c47dc593f303f3f60040f1612d92abcf4a4d0f9c9ad3bf7eece54f3078a3ab8ff92b36357be58db0919888e239a8a5c1a6b
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the usage of the
`impl_from_infallible` macro in the bitcoin, units, and internals crates
and just write the code.
Recent clippy nightly update introduces warnings about precedence. While
ours are, IMO, clear the lint docs have some cases that are not so I
don't think we should ignore this lint. Specifically I could easily miss
this one
1 << 2 + 3 equals 32, while (1 << 2) + 3 equals 7
ref: https://rust-lang.github.io/rust-clippy/master/#/precede
Add parenthesis to explicitly show precedence. Refactor only no logic
changes.
945fcd0920 fix ParsePublicKeyError using hex::InvalidCharError (Innocent Onyemaenu)
Pull request description:
Replaced the InvalidChar variant u8 with hex::InvalidCharError
Resolves#3835
changed InvalidChar variant of the ParsePublicKeyError from `u8` to `hex::InvalidCharError`
```
pub enum ParsePublicKeyError {
...
/// Hex decoding error.
InvalidChar(hex::InvalidCharError),
...
}
Also,
modified the test cases to accommodate the new variant
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
ACKs for top commit:
apoelstra:
ACK 945fcd09209120ef8869a2e4165e866328cc9bd5; successfully ran local tests; I like it
clarkmoody:
utACK 945fcd0920
Tree-SHA512: c13446c099cb02b4f253f9cc559a860aff3288a2cc5eac96d3cf910bf63e78957741bbdff69b936b16b36e46b366841a5c94876d16cbc0c41aea2a70866a6e45
85e04315d5 Remove test_ prefix from unit tests (Tobin C. Harding)
Pull request description:
There is a loose convention in Rust to not use `test_` prefix. The reason being that `cargo test` outputs 'test <test name>' using the prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some places and not others is confusing to new contributors and is leading me to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
ACKs for top commit:
shinghim:
ACK 85e04315d5
apoelstra:
ACK 85e04315d5eb90075ce55bf18fab8876a4583def; successfully ran local tests
Tree-SHA512: d90ae5ef75cc5e5a8f43f60819544f1a447f13cbe660ba71e84b8f27bfcc04a11d3afde0ed56e4eea5c73ebc3925024b800a1b995f73142cab892f97a414f14a
What:
- Replaced the InvalidChar variant u8 with hex::InvalidCharError
Why:
- hex::InvalidCharError includes both the invalid character and its position.
- This improves debugging and makes error messages more actionable.
There is a loose convention in Rust to not use `test_` prefix. The
reason being that `cargo test` outputs 'test <test name>' using the
prefix makes the output stutter.
This patch smells a bit like code-churn but having the prefix in some
places and not others is confusing to new contributors and is leading me
to explain this many times now. Lets just fix it.
Remove the prefix unless doing so breaks the code.
Rust macros, while at times useful, are a maintenance nightmare. And
we have been bitten by calling macros from other crates multiple times
in the past.
In a push to just use less macros remove the `debug_from_display`
macro and just write the code.
This is an API breaking change to `internals` but an internal change
only to any of the _real_ crates.
72e97c637f Add a hash value to Inventory's Error variant (Nick Johnson)
Pull request description:
I am working on adding BIP324 V2 p2p network message support to the p2p module and ran into a little snag. I can post a draft branch of that work if helpful, but the general strategy is to add a `V2NetworkMessage` which operates in parallel with the existing `RawNetworkMessage` type (which is a bit of misnomer and may be better described in the future as `V1NetworkMessage`, see #3157). This allows both p2p message types to hook into the existing `Encodable`/`Decodeable` chain.
But the `Error` variant of the `Inventory` type message does not have symmetrical encoding and decoding paths. Encoding writes out a zero'd 32 bytes while decoding ignores it completely. And while it is not clear to me if there is [requirement written down](https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors) anywhere, [bitcoin core does appear to always expect a hash](c1252b14d7/src/protocol.h (L499)) even with the `Error` variant where it is meaningless.
I believe rust-bitcoin's handling of this was never a problem before because the top level `RawNetworkPackage` pulls all the required bytes off a reader before decoding them. But this is not as easy to do with the v2 p2p network messages since the length is decoded at the transport level, not the message itself. I believe it is preferable for the Decoders to *not* assume that all bytes have been pulled off already given their input stream interface. Maybe somewhat surprisingly, this is the only issue I have run into so far adding the v2 encoding and decoding paths. As it is now, the code panics with an `Unconsumed` because it hasn't pulled the dummy zero bytes off the reader.
This patch adds the 32 byte array to the Error variant in order to make its Encoding and Decoding paths symmetrical. This also allows a reader to discard the 32 bytes when decoding a message while cleanly keeping things hooked up with the `Decodeable` chain. The hash is still not exposed to the caller.
This is *a way* to solve my issue, but not sure if it will cause more confusion than its worth. I tried a few other strategies, but preferred this one due to how well it hooks into `Decodeable`.
ACKs for top commit:
apoelstra:
ACK 72e97c637fa0916be75aef28ea8169ffbbe2c4f5; successfully ran local tests
tcharding:
ACK 72e97c637f
Tree-SHA512: 20cb9fec0768e0fdf7c7f520a00e1a37f5ef0583e2ebc7d47583249c6829c47826d83694a56338efa5844a9fe264a77f6d04c0c46c85f8732c7585515723d7ce
e13355318e Add From impl (yancy)
364e9ff775 Change method return type (yancy)
fdf3336ed5 Add unchecked variant (yancy)
Pull request description:
Any SignedAmount can now be cast to Amount since the range is the same. Specifically, the range for SignedAmount is (- 21 million, 21 million) while the range for Amount is (0, 21 million). Therefore any value from Amount can be cast to a SignedAmount and it will work. Note it's not the same and still requires checking when going from SignedAmount to Amount since Amount can't handle the negative range.
ACKs for top commit:
tcharding:
ACK e13355318e
Tree-SHA512: c016b51bdd87a12eb09d9c1a82699dad1e866bf96bd3235eeb131f216f02422acb992ddb3a8135af00bbc10e240178fde5e37fb7860d9e6eaf433cf917d4d16a
Any SignedAmount can now be cast to Amount since the range is the same.
Specifically, the range for SignedAmount is (- 21 million, 21 million)
while the range for Amount is (0, 21 million). Therefore any value from
Amount can be cast to a SignedAmount and it will work. Note it's not
the same and still requires checking when going from SignedAmount to
Amount since Amount can't handle the negative range.
As a side effect of changing the return type, TryFrom is no longer valid
and does not compile. Therefore in addition to changing the return
type, TryFrom is also removed.
774f066879 refactor: Change from u64 to Amount (yancy)
Pull request description:
Separate out refactor commit from https://github.com/rust-bitcoin/rust-bitcoin/pull/3794. Can be merged independently.
ACKs for top commit:
tcharding:
ACK 774f066879
apoelstra:
ACK 774f066879c8ad1af81c7e46b404fa63682a0b4c; successfully ran local tests
Tree-SHA512: 9ec5121d823ee3ec506eee5b5187bd496221bd3576afcaa6daf647099720d87b58b69521f29ae9537f123e2958771bc867b123da3f2ba941cba403a6c98e46de
The Amount type provides better type safety and is more appropriate in
this context than u64. Currently the checked arithmetic operations for
Amount and u64 are identical in behavior. Therefore, this refactor does
not result in any behavior change and is purely cosmetic.
While the hash value of the Error variant is meaningless, the variant
still conforms to all other Inventory messages and requires a 32
byte hash to be sent over the wire. This is how bitcoin core operates.
This patch adds the 32 byte array to the Error variant in order to make
its Encoding and Decoding paths symmetrical. This also allows a reader
to discard the 32 bytes when decoding a message. The hash is still not
exposed to the caller.
This was never a problem before because the top level RawNetworkPackage
pulls all the required bytes off a reader before decoding. But this is
not as easy to do with the v2 p2p network messages.
e56f461916 Make capitalization of SegWit uniform in strings (Jamil Lambert, PhD)
3520e832ac Make capitalization of SegWit uniform in rustdocs (Jamil Lambert, PhD)
Pull request description:
Fixes#3770
- Searched and replaced all occurrences of `//` * `segwit` (case insensitive) with `//` * `SegWit`
- Searched and replaced all occurrences of `"` * `segwit` (case insensitive) with `"` * `SegWit`
ACKs for top commit:
apoelstra:
ACK e56f461916b85928139950074d0df52caf4f919b; successfully ran local tests
Tree-SHA512: c56704102d8e86f26378bb302d5fdc7ea21d41fd49f55606e589ec32ff896f1adfb1960d8fb29abccfbf298b90fc357ed609d80fd0163dcb4ff01573646b02c3
f85456a726 Improve rustdocs on addresses module (Tobin C. Harding)
Pull request description:
These docs are stale, update them.
- Mention segwit and legacy
- Improve example code to show feature gating
- Fix headings to be as typical
ACKs for top commit:
storopoli:
ACK f85456a726
apoelstra:
ACK f85456a726a201b26eac58d22eb0eb6a58ad411b; successfully ran local tests
Tree-SHA512: c1cdde744fb960c4119805e6b3fb0cc971eaa94173fb85f8f7a34daab7985a0f14c9be477d7dda430c6873fed7265c598b8dcfbffe0ed6a99ca8590a2b2b724e
3ac9b2ded2 Remove unnecessary type (Tobin C. Harding)
Pull request description:
`hex_psbt` returns `Psbt`, no need for the explicit type.
Internal change only.
ACKs for top commit:
shinghim:
ACK 3ac9b2ded2
apoelstra:
ACK 3ac9b2ded24d5428945538c0a674301f6094df96; successfully ran local tests
Tree-SHA512: 2be1bfa56729bf97e60d721a184d3d21bfb27115fab1dd1b9103428658b5faca012d8f01e0458be7f17dadcd8cc495b327f2b472d825d46aa5cb71d0906ecb21
a5993426fd bitcoin: Improve rustdocs on extern crates (Tobin C. Harding)
Pull request description:
Make slight improvement to the extern crates we re-export from `bitcoin`.
ACKs for top commit:
apoelstra:
ACK a5993426fddea85c29ac6cd5a0e3096988d72b03; successfully ran local tests
Tree-SHA512: bb857316fc2606aa553f15e1c9fcf8dd87374e170121fe30cf9e44ae190f873f94c113af49b096cb49a65fd68d1bf24732e3f1aba04861e2ae934414f30473c6
6e01383f16 api: Run just check-api (Tobin C. Harding)
3855d3cc83 Move script hashes to primitives (Tobin C. Harding)
d1dd63d6d4 Remove wildcard in script re-exports (Tobin C. Harding)
Pull request description:
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the errors because they are in the `mod.rs` file so they should appear in both `primitives::script::FooError` and `bitcoin::script::FooError`.
ACKs for top commit:
apoelstra:
ACK 6e01383f162307a573c3454733ccae0596d3eaf6; successfully ran local tests
Tree-SHA512: 4ef838ee9c4cb3eb308ffa855ea5d6f5ad46f81c17b9413faa493a46a262afc18b47f28a0fdd5fc675eea31b895d0eb0e2505a523e820504ec88d9334d6874b4
0135cddc32 Refactor non_input_size (Tobin C. Harding)
f42f13cd8d Simplify closures in call to fold (Tobin C. Harding)
Pull request description:
Two refactorings to make the code easier to read. No logic changes.
ACKs for top commit:
apoelstra:
ACK 0135cddc32462490528dea549fd8db4c0a30646e; successfully ran local tests
Tree-SHA512: 747cfe1212283a62545cb86a5af0be46592ab3b41f94ab0a2dad8f06b094c04eb9265520ece595d2aab303fddf55a652e7ef977911439602550c264bedd4385b
3c8c956511 Remove Weight::from_wu_usize function (Tobin C. Harding)
Pull request description:
This constructor is an anomaly in this repo. Also it is ugly to have the type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like casts that don't obviously loose data. We have a solution for that already in `internals::ToU64`. This trait cannot be used in const contexts though so we do introduce a single cast in this patch.
ACKs for top commit:
apoelstra:
ACK 3c8c95651169fb9329ceb380162721c2d2f9b3aa; successfully ran local tests
Tree-SHA512: 168196edb7c151378d425b96ea3c9cdb0a4f6a879543e89facd02ed1fdf9bc69bde8ef862ffa0959b7c5ca21d6f4fe5ae38a933c379e7e88a946ca7cb68d61ec
99e2a9f530 Remove unnecessary rustdocs (Tobin C. Harding)
Pull request description:
The `deprecated` attribute already fully explains the status of a deprecated function and the replacement - no need to repeat this.
ACKs for top commit:
apoelstra:
ACK 99e2a9f530eced69e9df1441e15d24e626974385; successfully ran local tests
Tree-SHA512: 133d41573a3ba97abf85100649c1e8dfe131521a627e0b7f1ab803845caf1c183b99488cb5d9f7e858cab1b35f8907a79b320ec01901b15fdd63d08f0bc1f87e
4ef06b4f2b Remove ToU64 from rustdocs (Tobin C. Harding)
Pull request description:
The `ToU64` trait is not meant to be used publicly, remove the mention of it from rustdocs.
ACKs for top commit:
shinghim:
ACK 4ef06b4f2b
apoelstra:
ACK 4ef06b4f2b9945b46c190abd0cb3a6dfe234c7b5; successfully ran local tests
Tree-SHA512: e9acef9194cee380aa2aa44af9285d4f6e025bdf09f76e5066916c51215db15f1f920fd8ecf3cf079f0a2dddab0332b3a7df104f5bd61d12fc337b4ed163310c
This constructor is an anomaly in this repo. Also it is ugly to have the
type parameter hard coded in the function name.
The problem the constructor is trying to solve is that we don't like
casts that don't obviously loose data. We have a solution for that
already in `internals::ToU64`. This trait cannot be used in const
contexts though so we do introduce a single cast in this patch.
Woops, this should have been done before v0.101.0 was released.
Move the `ScriptHash` and `WScriptHash` types to `primitives`.
Requires moving constants and error types as well. We re-export the
errors because they are in the `mod.rs` file so they should appear in
both `primitives::script::FooError` and `bitcoin::script::FooError`.
These lints are valuable, lets get at em.
Changes are API breaking but because the changes make functions consume
self for types that are `Copy` downstream should not notice the breaks.
428e9787d1 Explicitly define Ord for NodeInfo (Shing Him Ng)
Pull request description:
Fixes#3654 by explicitly defining Ord so that we avoid potentially catastrophically changing addresses out from under users
ACKs for top commit:
sanket1729:
utACK 428e9787d1
apoelstra:
ACK 428e9787d181a462d06a18b7a45701790cbc0929; successfully ran local tests
Tree-SHA512: e900e07b2c53f643e3239bf7d26567b56275899b408a94e45f1cddd81217141c304c87159ce413a7a4660f1c09a0db2bbc1146948a2d9e7600abf14cd73ac691
To prevent rounding errors converting to and from f64 change
`Amount::MAX` to `MAX_MONEY` which is below the limit in f64 that has
issues.
Add checks to `from_str_in`, `checked_add` and `checked_mul` that the
result is below MAX, where previously a u64 overflow was relied on.
Change tests to account for new lower MAX that is within the range of
SignedAmount and does not overflow so easily
Remove overflow tests
`Amount::MAX` is now below `u64::MAX` and within the range of values for
`SignedAmount`. These tests therefore do not overflow.
In effective_value there is no error with `Amount::MAX` and the correct
value is returned.
In psbt the removed test is effectively the same as the previous test.
Modify `Amount` tests to work with new `MAX`
Tests need to be changed that checked values above the new `MAX` or
`Amount::MAX` was out of range for `SignedAmount` which it isn't anymore
b5a0731a23 refactor: remove unnecessary if block (Chris Hyunhum Cho)
Pull request description:
This `if...else` block can be replaced with one `if` block, by changing `==` to `!=`.
ACKs for top commit:
jamillambert:
ACK b5a0731a23
apoelstra:
ACK b5a0731a23f945f7e4cc7d8f3c0f1d3f83875272; successfully ran local tests
tcharding:
ACK b5a0731a23
Tree-SHA512: eee0d63532953d27fe081c36180ae9adb63c0ab7f1fb8324d9fc5e4a408f421d8df894efcccfc4bd3142625d6be6ea1258c9478faa025ce147ebe1b4e9377738
22769268f3 units: Close the hex parse errors (Tobin C. Harding)
Pull request description:
As part of the 1.0 effort close the errors in the `units::parse` module.
ACKs for top commit:
apoelstra:
ACK 22769268f34b45c0bd86c548eb13cfe1e290f0d7; successfully ran local tests; thanks!
sanket1729:
ACK 22769268f3
Tree-SHA512: 598ba236f8c08c3f750112e0d8b8e0aa0568be2700a328e4a2d8999ca59eada8a16a6a1d9e9121e7f42ce9bbe3da3f87221ba67c36056996a687e489f4c6007c
77085a1fa1 units: Remove serde from amounts (Tobin C. Harding)
Pull request description:
The `Amount` and `SignedAmount` were not supposed to implement `serde` traits by design because doing so implicitly uses sats. We provide two modules `as_sat` and `as_btc` to allow users to explicitly serialize in their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives were added for `serde` and we did not notice it during review.
ACKs for top commit:
apoelstra:
ACK 77085a1fa1f39e7358202e49189282e48a965747; successfully ran local tests
Tree-SHA512: ff4c1f34191e12d955e66737493af4b2dad761fa6f95eb334b86715906e70d2180e7f15d8b4af302d53fb0b1c8b5fb50b4924e9fcdee8824840ba555e73c1c17
60f43a893d Remove duplicate test case (Tobin C. Harding)
Pull request description:
In commit:
`a10d5e15b3 Extract the Script assembly creator from fmt::Debug`
A test case was refactored and where there used to be two test case, one for `Debug` and one for `Display`, two identical test cases were left.
Remove duplicate test case.
ACKs for top commit:
apoelstra:
ACK 60f43a893d67c221f61e289cab6394418411cf55; successfully ran local tests
Tree-SHA512: 8d21f07b33c9f88ac820422b9f5471bf53e36050a31854a7152ab14c6e25654455f4eb366ea2b497a80f741af36bf068b1df8a69321a0299c9bae60f001b354e
f961f3c0ec Add test (yancy)
3352032892 Fix internal weight calculation (yancy)
Pull request description:
A fix for 261c8d8ae6. Maybe it's better to roll back that commit instead. Clearly there needs to be a test or two added here as well since this change causes not test failures.
ACKs for top commit:
apoelstra:
ACK f961f3c0eca4e9b175761756f48699012b63d70a; successfully ran local tests
sanket1729:
utACK f961f3c0ec
tcharding:
ACK f961f3c0ec
Tree-SHA512: e6bb6d492b6a85f328bdfae6d0e5df9ea48c9c06bd0c310816826c893163bd0a6071af893fa7012fbd4eea0394bc8997277151486807fb8bd06ac78ce8cfc5fd
4d0d78a3af Fix typo in from_next_work_required documentation (Shing Him Ng)
5e47c4123d Update CompactTarget::from_next_work_required to take timespan as i64 (Shing Him Ng)
Pull request description:
[In Core](f7144b24be/src/pow.cpp (L56)), the timespan is defined as a signed int64. This PR will update `from_next_work_required` to take in an `i64` instead of a `u64`
Fixes#3652
ACKs for top commit:
tcharding:
ACK 4d0d78a3af
apoelstra:
ACK 4d0d78a3af2ca300c6b40348724e806bd51ef27a; successfully ran local tests
Tree-SHA512: eea33ec18b081a97ae2a22c681982ad8693200ff8de3a3fc8f46b84b5e72f41589b79140ddd79ba3fce628f1539baf8387422bcc2c6b409fcadc4c2043aa2901
The `Amount` and `SignedAmount` were not supposed to implement `serde`
traits by design because doing so implicitly uses sats. We provide two
modules `as_sat` and `as_btc` to allow users to explicitly serialize in
their preferred format.
In commit: `d57ec019d5 Use Amount type for TxOut value field` derives
were added for `serde` and we did not notice it during review.
e0442782c8 fix(script): account for data pushing opcodes in `is_standard_op_return` (Rob N)
Pull request description:
While I believe the original commit used 80 bytes for the entire script as the limit, Bitcoin Core as of [this commit](1101837461/src/policy/policy.h (L68-L72)) will relay OP_RETURN outputs as long as the data itself is not above 80 bytes, meaning a script of maximum size 83 bytes should be standard.
ACKs for top commit:
tcharding:
ACK e0442782c8
apoelstra:
ACK e0442782c8c04bdc48c2743e70f46e41c507e5f4; successfully ran local tests
Tree-SHA512: edd627806c9b5c85050594d45fde49ce3945b9358ae67b4bd0e22f7feb855fe5f4d90f1770870716decbc70aa1a3e0758902c74e3c010977731b823fd72ec23b
In commit:
`a10d5e15b3 Extract the Script assembly creator from fmt::Debug`
A test case was refactored and where there used to be two test case, one
for `Debug` and one for `Display`, two identical test cases were left.
Remove duplicate test case.
9aebb96fb9 Fix psbt fuzz crash (Sanket Kanjalkar)
Pull request description:
Fixes: https://github.com/rust-bitcoin/rust-bitcoin/issues/3628
This occurs when combining two PSBTs with different xpub key sources. Added a length check before indexing into slices to prevent out-of-bounds access.
For some reason, the precommit hook complained about non-ascii files. I don't think any of the names here are non-ascii
ACKs for top commit:
apoelstra:
ACK 9aebb96fb99e8e9e019663659c6eff851a62f2ce; successfully ran local tests; thanks!
tcharding:
ACK 9aebb96fb9
Tree-SHA512: b61274c594bc1f2ea4d04c8a7ace673a7632bb9ea31f59b56779a008c35e61281ea4f6b869990d886779e3e556932a3b2e8b015733ef18e236f12ca77e211c26
While I believe the original commit used 80 bytes for the entire script as the limit,
Bitcoin Core as of [this commit](7a172c76d2/src/policy/policy.h)
will relay OP_RETURN outputs as long as the data itself is not above 80 bytes, meaning a script of maximum size 83 bytes should be standard.
Bitcoin Core's consensus rules allow this timespan interval to be
negative. This commit also updates Params::pow_target_timespan to be a
u32. This field is almost compared to u64 and i64s, so changing this to
a u32 will allow users to use `.into()` for converstions
This function calls helper function predict_weight_internal and that
function was refactored. The refactor changed the interface to accept
the input_weight instead of partial input_weight, however this function
was not updated with the refactor and was still passing a partial input
weight. This commit is a followup that fixes the internal calculation
error by pass input_weight instead of partial_weight to the helper
function predict_weight_internal.
261c8d8ae6 Add total_weight to Input Weight Prediction (yancy)
cc4c36e8ac Deprecate weight (yancy)
Pull request description:
Closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3645
I'm not sure if `total_weight` is the most accurate. Please advise if there's a different name that is better.
ACKs for top commit:
apoelstra:
ACK 261c8d8ae659d98fd3f63228f16d8f2eb4010b7e; successfully ran local tests
Tree-SHA512: f7fbcc587d3b72edf4add8a7bfecf99ee8133f679bd905c2ab6eeb959db455905c7e32ae91c95571599f09df605454f1e9953f1a47d1f1ee6d66270784c77793
d9ccb60a2f Fix typo in hash tag type (Tobin C. Harding)
Pull request description:
In commit: `39f7dcb Reduce API surface of tagged wrapped hash types` I introduced a typo in the tagged hash tag type. Fortunately it has no effect because the tag type controls how an engine is created (when the tag is input into the engine) and has no effect on the call to `from_engine`. However the typo makes the code confusing.
This bug is currently unreleased.
Fix: #3649
ACKs for top commit:
apoelstra:
ACK d9ccb60a2f9e89c8bb1979f032e8a222ab9b025a; successfully ran local tests
Tree-SHA512: 54cc78fb74ea119beaf4500d49a4d3954ffc3c52494b3ebc1539a3eec35078cb9388e72b42c59f371b53cced89b7b6303a0e3d3b02e59d72da182dd7bbfe2a55
`rust-bitcoin` is a lot further from 1.0 than `bech32` is. We previously
removed the re-export of `bech32` because we thought it might hold us up
releasing `rust-bitcoin 1.0` but in hindsite this was incorrect.
Having a public re-export of dependencies helps users keep their deps in
sync with those in `rust-bitcoin` and is thus useful.
Close: #3650
In commit: `39f7dcb Reduce API surface of tagged wrapped hash types` I
introduced a typo in the tagged hash tag type. Fortunately it has no
effect because the tag type controls how an engine is created (when the
tag is input into the engine) and has no effect on the call to
`from_engine`. However the typo makes the code confusing.
This bug is currently unreleased.
Fix: #3649
073ff81536 address: Simplify `Address::assume_checked` impl (Max Fang)
e4cf8ebc20 address: Add `Address::into_unchecked` (Max Fang)
Pull request description:
## Commits
### address: Add `Address::into_unchecked`
Adds an ergonomic way to convert any `Address` (network can be checked
or unchecked) into an `Address<NetworkUnchecked>` without cloning, which
I've found useful in several contexts.
### address: Simplify `Address::assume_checked` impl
Removes an unnecessary `match` which I noticed while implementing `Address::into_unchecked`.
## Small note on use of `Self`
The style guide in `CONTRIBUTING.md` notes to return `Self` when possible, but that doesn't apply here as the `Address` being returned is different from the `Address` type referred to by `Self`, due to the changed `NetworkValidation` type.
ACKs for top commit:
tcharding:
ACK 073ff81536
apoelstra:
ACK 073ff81536e7a24883d6470ecf3054f4b7263186; successfully ran local tests; nice!
Tree-SHA512: dd6749cffad75d88568106169032d8af023ed56df4b1b38fa613ae7b140d1eb22933e7b0ffc1b17681a5db4b11b28d9db4efb62868755ca3c0f7cac20de0be4b
a8379bf005 Mark `checked_` functions const in bitcoin. (Jamil Lambert, PhD)
fe10ff2eb7 Mark functions `const` in `units` (Jamil Lambert, PhD)
Pull request description:
Following on #3608, #3627 and #1174 the rest of the `checked_` functions in all `rust-bitcoin` have been marked as const. Except for tests and traits.
ACKs for top commit:
apoelstra:
ACK a8379bf0053e66cf5984ce449d19e54e529b6b70; successfully ran local tests; thanks! I find this much easier to read
tcharding:
ACK a8379bf005
Tree-SHA512: b8d97b7a3d9fc33b57349f418ccc5aac2f3e8c4145a73bf0e24e85547217d41214696e9a80b1fb5a1bae7e766aad1c2b5df6044564772fe76cec9b3628bcd8e5
Adds an ergonomic way to convert any `Address` (network can be checked
or unchecked) into an `Address<NetworkUnchecked>` without cloning, which
I've found useful in several contexts.
ad82ed7179 Fix spelling typo (yancy)
8fe5ffde4c Rename tests that have _test suffix (yancy)
Pull request description:
Convention is to not include test as a suffix
Used `git grep -n _test` and renamed all the tests that made sense to rename. There are a number of fn `do_test(data: &[u8]) {` that I felt it would be better not to touch. Everything else that was a testname I renamed if it had a _test suffix.
ACKs for top commit:
apoelstra:
ACK ad82ed71796c79399cd8a81814b1af61c4ca3582; successfully ran local tests
tcharding:
ACK ad82ed7179
Tree-SHA512: 9c4ee71974e39814a8a63d269e3dcf6e761312dd0903ac1e6268dc421b9ef89a63f27cade3d5a51436660bb01457ac1a2f23a628a4d11622cd4a33fa6c483934
ec06028f63 hashes: Make hex dependency optional (Tobin C. Harding)
9dce0b4b8c Remove hex string trait bounds from GeneralHash (Tobin C. Harding)
766f498b33 Pull serde stuff out of impl_bytelike_traits macro (Tobin C. Harding)
Pull request description:
This is done in 3 parts:
1. Pull the `serde` stuff out of `impl_bytelike_traits` to fix the bug described here: https://github.com/rust-bitcoin/rust-bitcoin/issues/2654#issuecomment-2470716693
2. Prepare the `hashes` trait by removing string/hex trait bounds from `GeneralHash` and also pull the hex/string stuff out of `impl_bytelike_traits`
3. Make hex optional, including adding custom debug logic when `hex` feature is not enabled
Patch 3 is tested in `hashes/embedded`, by the new `debug` unit test, and there is a `Midstate` unit test as well that covers the `Debug` impl.
Close: #2654 - BOOM!
ACKs for top commit:
apoelstra:
ACK ec06028f63ba591a14c3a15cdfd410bb5ff1c09b; successfully ran local tests; nice!
Tree-SHA512: 85eb10d36a4581af6cd700f7ff876585bcc114c60e9864906e65659f3b3ee550fe6d9f40ca4230d870a9e23f0720723e11443ec329f16e40259a259b9be57466
On the way re-design the API by doing:
- Introduce `Checked` and `Unchecked` tags
- Rename the `txdata` field to `transactions`
- Make the `Block` fields private
- Add getters for `header` and `transactions` fields
- Move the various `compute_` methods to be free standing functions
- Make the `check_` functions private
- Introduce extension traits
In preparation for adding a block extension trait move the only private
method off the `Block` type and make it free standing.
While we are at it just take a slice of transactions since we don't need
the block itself - rename appropriately.
Internal change only.
For the `hashes` crate we would like to make `hex` an optional
dependency. In preparation for doing so do the following:
- Remove the trait bounds from `GeneralHash`
- Split the hex/string stuff out of `impl_bytelike_traits` into a
separate macro.
The `impl_bytelike_traits` macro is public and it is used in the
`hash_newtype` macro, also public.
Currently if a user calls the `hash_newtype` macro in a crate that
depends on `hashes` without the `serde` feature enabled and with no
`serde` dependency everything works. However if the user then adds a
dependency that happens to enable the `serde` feature in `hashes` their
build will blow up because `serde` code will start getting called from
the original crate's call to `hash_newtype`.
Pull the serde stuff out of `hash_newtype` and provide a macro to
implement it `impl_serde_for_newtype`.
Add a unit test that verifies we correctly remove the annex when getting
the tapscript from the witness stack.
Unit test written by Casey, pulled out of #3599.
Co-developed-by: Casey Rodarmor <casey@rodarmor.com>
In #2646 we introduced a bug in the taproot witness stack getter
functions, of which we have three:
- `tapscript`
- `taproot_control_block`
- `taproot_annex`
Each returns `Some` if a possible bytes slice is found (with no other
guarantees).
Use `taproot_annex` combined with getters from `primitives` to implement
the other two getters. This simplifies the code and fixes the bug.
Add an additional getter to `primitives` `Witness::third_from_last`.
Fix: #3598
These re-exports are hard to get right. We (I) recently made an attempt
to make our stack of crates easier to navigate using the idea that
exporting a type from the crate it is defined in adds some additional
information without any loss of clarity. Note however that the module
must be re-exported from the "highest" place possible because we at
times add additional functionality as we move up the stack e.g.,
`bitcoin::merkle_tree` has logic in it that `primitives::merkle_tree`
does not. In order to future proof the codebase default to always using
the highest module up the stack even when that module adds no additional
code e.g., re-export `blockdata::fee_rate` as opposed to
`units::fee_rate` in the event that we later add logic to
`bitcoin::blockdata::fee_rate`.
This patch adds an additional re-export: `sequence` (previously missing).
8098d5ee05 Reword `Address` constructor docs (Jamil Lambert, PhD)
Pull request description:
Change the wording of the Address constructor function docs to be a standard format. Following up on [#3584 comment](https://github.com/rust-bitcoin/rust-bitcoin/pull/3584#issuecomment-2457197066)
ACKs for top commit:
apoelstra:
ACK 8098d5ee055d672fc8f6e4eef3971b66478a61b0; successfully ran local tests
tcharding:
ACK 8098d5ee05
Tree-SHA512: 1027f334d69182465b2ecafd6fcf9f5deb2b299a64d12dea7f09f25379bd98b1e11be52a04fea83c2194164fd428931f6416eee52603f4bc043717b4e4d1b193
3523b8e117 Rename generic parameter and variable (yancy)
Pull request description:
Use W for Write
closes https://github.com/rust-bitcoin/rust-bitcoin/issues/3590
ACKs for top commit:
tcharding:
ACK 3523b8e117
apoelstra:
ACK 3523b8e1171663fc93ab25765271a1070a2d649f; successfully ran local tests
Tree-SHA512: 8dd9f38ee07e7652ebd291eb121a2630ebb09ea9ea70873f87e57241f51e5114106517511d1fe07774ec4f42920e9d0619031a1545787631a30422de12421a67
c61df05916 Fix bip34 number parsing for Block (junderw)
Pull request description:
Fixes#3583
ACKs for top commit:
apoelstra:
ACK c61df059160d3aeef73046de28cb5d5afcb962d2; successfully ran local tests
tcharding:
ACK c61df05916
Tree-SHA512: 3fade122f98c7c9e3b2888c33479d3a9426fb35f212ccee6b89b7cd5c92d2b58c93e165d5c59eda183c0943809d7e633d929000be92b197988a3ceb0ea37514f
1649b68589 Standardize wording to `constructs a new` (Jamil Lambert, PhD)
27f94d5540 Replace `creates` with `constructs` (Jamil Lambert, PhD)
Pull request description:
As discussed in issue #3575 there are various ways of saying a new object is created.
These have all be standardized to the agreed version.
Close#3575
ACKs for top commit:
apoelstra:
ACK 1649b68589834dfe9d5b63812da3e9f0e5930107; successfully ran local tests
tcharding:
ACK 1649b68589
Tree-SHA512: 0ed9b56819c95f1fc14da1e0fdbbe03c4af2d97a95ea6b56125f72913e8d832db5d2882d713ae139d00614e651f3834a4d72528bdf776231cceb6772bf2f9963
There is a range of different wordings used in the docs of constructor
type functions.
Change all to start with `Constructs a new` or `Constructs an empty`.
In functions that act like constructors there is a mixture of the usage
of `creates` and `constructs`.
Replace all occurrences of `creates` with `constructs` in the first line
of docs of constructor like functions.
Is there any advantage trying to lay out the re-exports to give users an
idea of the crate structure?
We have the explicit aim that users who depend on `bitcoin` do not ever
need to reach directly into `primitives` (or `units`) however it is kind
of nice to know where things come from, saves jumping to multiple files
looking for them (for those of us that jump to files manually).
I do not know how all the re-exports interact with other folks IDEs, I
personally open files manually and just remember where stuff is.
85942c355d Re-export block::Header as BlockHeader (Tobin C. Harding)
Pull request description:
For users who want to just grab stuff from the crate root it makes total sense for there to be a `BlockHeader`.
Another nice thing, in the HMTL docs it makes BlockHeader be in the struct list right along with `Block`, `BlockHash`, and `BlockHeight`.
Close: #3548
ACKs for top commit:
apoelstra:
ACK 85942c355dfdcf9ddf9a46202f4db56794dcc85d; successfully ran local tests
jamillambert:
ACK 85942c355d
Tree-SHA512: 6fbaf7936062323d31940179ffcbc62951b21f53955a42dca2e28476bda0cebeb041db0e1b4f1ceeac32e43d94fe5476ef055595ae88dc6cfc266d32082bdf4d
4431df18fe script: remove `unsafe` marker from slice-to-script conversions (Andrew Poelstra)
Pull request description:
The length of the slices is not a safety invariant.
Fixes#3531
ACKs for top commit:
tcharding:
ACK 4431df18fe
Tree-SHA512: 85f745058bc5bf68d312f75c53cef570ead986247ca3cb3fc54b5381dd0214e7df55c1fcb99f06e693b0b9478ec6a9785186cac56886f8007ab7b827d903bd01
The length of the slices is not a safety invariant.
Fairly large diff because I had to remove a bunch of `unsafe` blocks
around calls to this function.
Fixes#3531
We have a couple of problems:
1. There are two macros currently for fmt stuff that do similar things,
`arr_newtype_fmt_impl` and `hex_fmt_impl` - the difference is not
immediately obvious, its the way that the byte array is iterated.
2. Our hash types are missing `AsRef<[u8; len]>` and `Borrow<[u8; len]>`.
Introduce a new macro and remove a bunch of other macros. Include
extensive docs but hide the macro from public docs because its not
really for consumers of the library.
The macro requires `$crate::hex` to point to `hex-conservative`.
Note the macro is pretty generic (as in general purpose), `hashes` might
not be the right home for it. Potentially a better place would be in
`hex` itself?
For users who want to just grab stuff from the crate root it makes total
sense for there to be a `BlockHeader`.
Another nice thing, in the HMTL docs it makes BlockHeader be in the
struct list right along with `Block`, `BlockHash`, and `BlockHeight`.
This has been fixed and we use nightly to lint so we have access to the
merged fix.
Removing the attribute uncovers a bunch of real lint warnings, fix
them while we are at it.
a51768af3f key: Deprecate to_bytes (Tobin C. Harding)
3af3239ad0 script: Re-order functions (Tobin C. Harding)
db40297f87 script: deprecate to_bytes (Tobin C. Harding)
c5cd0db493 Revert the change to to_bytes (Tobin C. Harding)
dc2ca785d2 Add to_vec and deprecate to_bytes for array types (Tobin C. Harding)
a6b7ab32a8 Move impl_array_newtype to internal_macros (Tobin C. Harding)
Pull request description:
Use `to_vec` and deprecate `to_bytes`, the opposite of what we did in #2585.
For functions that return a `Vec` by first allocating use function name `to_vec`. This explicitly excludes:
- Functions that return an array (`CompressedPublicKey::to_bytes`)
- Functions that consume self and return a `Vec` without allocating (`ScriptBuf::into_bytes`)
See #3025 for discussion and consensus.
Close: #3025
ACKs for top commit:
apoelstra:
ACK a51768af3f3d4c8e138e1ded250800810bedc903; successfully ran local tests
Tree-SHA512: ee932c13ad2e09c2b76a7833b23c859df175aa307f56e673921f3ae8b5d865518c6f999749e3b627594457b3ca33301b777177ada3520cf006acc0f14e5dacf8
0bf1910980 Remove wildcard from script self re-exports (Tobin C. Harding)
397a4b9382 Remove wildcard in push_bytes module (Tobin C. Harding)
Pull request description:
We thought #3436 was done (well I thought it was), turns out I was wrong.
ACKs for top commit:
jamillambert:
ACK 0bf1910980
apoelstra:
ACK 0bf1910980a13005496244ec4d4adf0553afbc73; successfully ran local tests
Tree-SHA512: 131a4aa4a907099790d14cfc2d19305943a2270cf6852c7dd92e35ea69188c9a40797fb22fd2ed8b2fefc2d6380b884401b5e32f521711f0f7b2da610d0e332f
The private_key_debug_is_obfuscated test is removed because it belongs
in the secp256k1 library, not in the bitcoin library. Keeping it here
is redundant and creates unnecessary maintenance.
As we have been doing in other places deprecate `to_bytes` in favour of
`to_vec`. Note this is only for functions that return a `Vec`, the `key`
module has `CompressedKey::to_bytes` still as it returns an array.
Deprecate the `Script::to_bytes` function in favour of `to_vec` as we
are doing elsewhere.
Note that `ScriptBuf` has `into_bytes` because it does not copy.
Potentially this should be deprecated in favour of `into_vec`?
Note that in regards to the `to_` prefix this naming as valid according
to convention because the `Script` type is borrowed and `to_vec` copies
the underlying bytes.
During this release cycle we deprecated `to_vec` in favour of
`to_bytes`, we have since reversed our position on the name.
Remove the deprecation of `to_bytes` from the three types that had it
and use `to_vec`.
For the array wrapper types (eg `ShortId`) created with the
`impl_array_newtype` macro deprecated the `to_bytes` function and add a
`to_vec` function.
Discussed and decided upon in issue: #3025
dfb76c1e15 Use doc(notable_trait) (Tobin C. Harding)
Pull request description:
There is an unstable feature that puts up a little 'i' in a circle next to any function that returns a type that implements an notable trait. For us this means we can make the extension traits more discoverable.
ref: https://doc.rust-lang.org/unstable-book/language-features/doc-notable-trait.htmlClose: #3232
ACKs for top commit:
apoelstra:
ACK dfb76c1e15933fe0058c1bb69c4b1b9acddceee8; successfully ran local tests
Tree-SHA512: 0fbc7a2a3c8c499a9276d1e86b9966a7ae6bd8a354aff5fd40aa11d07945db589b2a9c2cdfa43ddadfafcee706ae7f68cedc269f74622643307cc43cd07d554f
In the `script` module remove the wildcards and re-export stuff from
`self` explicitly in both `primitives` and `bitcoin`.
Internal change only, everything is re-exported.
The current feature gating is wrong, this bug is unreleased because it
was introduced #2585.
The `impl_array_newtype` macro is only used in the `bitcoin` crate, it
does not need to be in `internals`. Also, other crates have an `alloc`
feature which `bitcoin` does not have so if we ever need it in other
places we'll need a duplicate with the correct feature gating anyways.
Move the macro to `bitcoin::internal_macros` and remove the incorrect
`alloc` feature gating.
WARNING: This is not like all the other extension traits.
Because of the use of generics on various `Transaction` methods it is
not easily possible to use the `define_extension_trait` macro.
Manually create the extension traits (public and private) for the
`Transaction` type. This is quite ugly but c'est la vie
(Includes two in the `transaction` module and one in the
`consensus_validation` module.)
Currently `script_pubkey_lens` returns a generic `Iterator` using `impl`
syntax. This syntax is not supported in traits and we want to move the
function to the soon-to-be-added `TransactionExt` trait.
Add a struct to hold the iterator returned by `Map`, this is ugly but
its the least ugly thing I could come up with.
Split the `Transaction` impl block into three parts:
- The bits going to `primitives`
- The bits staying in a public extension trait
- The bits staying in a private extension trait
Internal change only.
It is acceptable to use a wildcard import in bench code for the same
reasons it is acceptable in the `tests` module.
In preparation for introducing extension traits in the `transaction`
module use wildcard import in the module's bench code.
c89b816437 psbt: Fix bug in Subtype consensus_encode (Tobin C. Harding)
Pull request description:
In #2906 we switched from using a `u8` for type keys to using a `u64` and encoding as a compact int (inline with the spec). Note that a `u8` encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by `PoprietaryKey::consensus_encode` uses a hard code 1 for the length of the encoding (because of single byte) instead of the variable length for the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
Fix: #3501
ACKs for top commit:
jamillambert:
ACK c89b816437
apoelstra:
ACK c89b8164377123eb20476636f2f5271c6a687406; successfully ran local tests
Tree-SHA512: 1b61b6a9ece197d74038ceedb447fd3ca21db8e2a6a96c9281a99ac232c18c3ca55da8e3f46930401714d3575e9a406a36e4f44929ca963208a5df4be6b46cfb
025a8773bf Use fully qualified path in macro (Tobin C. Harding)
Pull request description:
Using fully qualified paths in macros reduces maintenance burden. We have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
ACKs for top commit:
apoelstra:
ACK 025a8773bf63aacdaca011ef000f41a85a961567; successfully ran local tests; will one-ACK merge
Tree-SHA512: eb5923a48ae4d82499679a58375ef7d2e8ba85c91671e350f7be19f0372750a269f44dd2f05f4a70ed0c7f277b160400eb41ff1d42b90e6057f1344be7e11a89
66da2266e2 Explicitly re-export stuff from crates down the stack (Tobin C. Harding)
Pull request description:
Up until recently we were using wildcard re-exports for types moved to `units` and `primitives`. We have decided against doing so in favour of explicit re-exports.
Audit `units` and `primitives` using `git grep 'pub enum'` (and `struct`) and explicitly re-export all types.
Remove all wildcards except for the re-exports from `opcodes`, there are too many opcodes, explicitly re-exporting them does not aid clarity.
ACKs for top commit:
apoelstra:
ACK 66da2266e26dfe53947c4606e9d18620931e93cf; successfully ran local tests
Tree-SHA512: 74717f8b127e975e3d131aab884bdfe78e699d88b7ee1db7731ad117437d37684285264001cf6b2182eb1e565171167695e00c4b6aef28a3e26b69d9cebfbb74
9a7b1c232b Wrap the bech32 decoding error (Tobin C. Harding)
Pull request description:
In #2381 we attempted to fully encapsulate the `bech32` crate to help with stabalizing `rust-bitcoin` however we failed to notice the `address:ParseError` has a variant that includes `bech32`. Public enums have public variant internals in Rust. Also the `From<bech32::segtiw::DecodeError` makes `bech32` public.
Closes: #3043
ACKs for top commit:
apoelstra:
ACK 9a7b1c232b494dccdce091a46d916cc411a612a1; successfully ran local tests; will one-ACK merge since this is a gazillion years old and obviously right
Tree-SHA512: b5053aa43107aa47da1fe7e7db0f882cfb231b9769a7b67d8c930532c471df191f588bf98f2b00cc76d5a2e9c74e035ee96128da115363ac3952f96a766494ea
Using fully qualified paths in macros reduces maintenance burden. We
have one macro where we use relative path to access the `psbt` module.
Refactor only, no external change.
In #2906 we switched from using a `u8` for type keys to using a `u64`
and encoding as a compact int (inline with the spec). Note that a `u8`
encodes to the same bytes as a `u64` when the value is < 252.
In that patch, I introduced a bug because the length returned by
`PoprietaryKey::consensus_encode` uses a hard code 1 for the length of
the encoding (because of single byte) instead of the variable length for
the new compact encoding.
Bug showed up in fuzzing, and was isolated by Jamil - mad props.
We do not want `bech32` to appear in the public API of the `address`
module in case `bech32` does not stabalize before the soon-to-be-created
`address` crates does.
We already had a go at removing it but forgot one error variant - wrap
the variant in a new type with a private inner bech32 error field.
Up until recently we were using wildcard re-exports for types moved to
`units` and `primitives`. We have decided against doing so in favour of
explicit re-exports.
Audit `units` and `primitives` using `git grep 'pub enum'` (and
`struct`) and explicitly re-export all types.
Remove all wildcards except for the re-exports from `opcodes`, there are
too many opcodes, explicitly re-exporting them does not aid clarity.
5633b10f5c Manually implement compute_txid and compute_wtxid (Tobin C. Harding)
Pull request description:
We would like to move the `Transaction` type over to `primitives` including the `compute_txid` and `compute_wtxid` functions however currently the implementations, as expected, use `Encodable`.
Manually implement `Encodable` by hashing all the fields in the correct order.
Note we have unit tests already that check the output string of the txid returned so these act as regression tests for this patch.
ACKs for top commit:
apoelstra:
ACK 5633b10f5c826e0b2ac47dd85f697f12710898d7; successfully ran local tests; nice
Tree-SHA512: 66a955d3d896801cfefe0388aade3a31f22fac5b6da7b996be61f374b93772487c0c203320aaf5165fcef26874564bce375ecb364175b0a01c3008b7ea8db981
We would like to move the `Transaction` type over to `primitives`
including the `compute_txid` and `compute_wtxid` functions however
currently the implementations, as expected, use `Encodable`.
Manually implement `Encodable` by hashing all the fields in the correct
order.
Note we have unit tests already that check the output string of the txid
returned so these act as regression tests for this patch.
In preparation for moving the `TxIn` over to `primitives` make the
private `TxIn::BASE_WEIGHT` associated const into a file-scoped constant
because the other alternative is to make it public.
9c2ac46902 Split up ParseError (Jamil Lambert, PhD)
3d994f7bdb Decode an address string based on prefix (Jamil Lambert, PhD)
Pull request description:
When a decoding error occurs for a bech32 address string the error is discarded and the same address string is attempted to be decoded as base58. This then incorrectly returns a base58 error.
Check the string prefix and decode as bech32 or base58 and return the relevant error. If the prefix is unknown return an `UnknownHrpError`.
Close#3044
ACKs for top commit:
tcharding:
ACK 9c2ac46902
apoelstra:
ACK 9c2ac46902ae2e6f2513ee125ea5c89953ac89a2; successfully ran local tests
Tree-SHA512: 40c94328828af86723e84d4196e8949430fb9a15efd8865c18cb5048fe59b8a2514d97f4809d828353b78c010544a8a6d4589a8c9c7fbd75d9d0ecceb3151e8f
bbffa3db43 Remove the IO error from DecodeError (Tobin C. Harding)
713196be0d Return DeserError from encode::deserialize (Tobin C. Harding)
33566ac58c Split encode::Error into two parts (Tobin C. Harding)
b04142c745 Add encode::Error::MissingData variant (Tobin C. Harding)
5a42ef2850 Do not manually map IO error (Tobin C. Harding)
efd7f9f06c Add error constructor parse_failed_error (Tobin C. Harding)
ebfef3f114 Return generic error as Some (Tobin C. Harding)
a6254212dc Move consensus error code to submodule (Tobin C. Harding)
Pull request description:
The `consensus::deserialize` and `consensus::deserilaize_partial` functions should not return an I/O error. Doing so causes various other error types to include an `io::Error` and the `io::Error` is an annoying type to work with.
This PR is a bunch of steps, and it took me a good while with quite a bit of backtracking to get here. As such you may want to review the final state before looking at each patch.
The `consensus` errors can be further cleaned up but I'd prefer not to spend more time on this unless it has some chance of merging.
ACKs for top commit:
apoelstra:
ACK bbffa3db43802b30d23259c0372f16a877a0ef8b; successfully ran local tests
Tree-SHA512: 522fdd29638a214cb7fcee29dd3b9f5c846f041fba087a56a91b83e6d85f033cbed95f659dc4321cd4596943ff233bdd184cdfbfcc787fe89172bb93aa4ab186
Currently `combine_node_hashes` is an associated function, it is also
private. It is called from within other methods of the `TapNodeHash`.
In preparation for moving the `TapNodeHash` to `primitives` while
leaving all the methods in `bitcoin` in an extension trait; move the
associated function out of `TapNodeHash` and make it a stand alone
private function.
e7d326f071 Seal extension traits (Tobin C. Harding)
Pull request description:
The extension traits are temporary just while we try to stabalize `primitives`, they are not intended to be implemented by downstream.
Seal the extension traits so that downstream crates cannot implement them.
Fix: #3231
ACKs for top commit:
apoelstra:
ACK e7d326f071a368389f087ddb10ee9bbf3552c33a; successfully ran local tests; thanks! I know this is tedious and annoying
Tree-SHA512: 365979aeabb7941b9c8fa526f71aaadae3ab1cdd6a39e992c5eea2c1057b4b7c2b3a846ffd96a7eab47b9ad4e3e4de4fb141c24c62747e5cee45c74f52f9a172
The `DecodeError` (badly named) consensus decodes an object from an
iterator that implements `Read`. The `Read` impl never returns a real IO
error, we use the `io::Error` to temporarily wrap the error returned by
the inner iterator and unwrap it in `IterReader::decode`. As such there
is no reason for the `DecodeError` to hold an `encode::Error`, it can
hold an `encode::ParseError`.
The value of this change is easily seen in the removal of calls to
`unreachable`.
The `consensus::encode::Error` contains an IO error but reading from a
buffer only ever errors for EOF. We converted all instances of EOF to
`MissingData` already so now we can split the IO error apart from the
actual encoding errors variants.
The `io::Error` is troublesome because it contains a bunch of stuff that
never happens when reading from a buffer. However the EOF variant can
occur if the buffer is too short. As an initial step towards reducing
usage of the `io::Error` add a `MissingData` variant to the
`encode::Error` and when converting from an IO error map to
`MissingData` if EOF is encountered.
The `encode::Error::ParseFailed` variant holds an inner string, this is
suboptimal.
In an effort to patch the `encode::Error` while mimizing the diffs
required add a helper function that creates the variant. The benefit is
that later patches that effect this variant will only need to update the
constructor function and not every call site.
Internal change only.
The `consensus` module has a bunch of error types, move them all to a
separate module. Add re-exports so the types are still available at the
same place they were. Make the `error` module private and re-export all
errors from the `consensus` module root.
Peter Todd
Martin Habovstiak
Tobin C. Harding
Jamil Lambert, PhD
merge-script
Innocent Onyemaenu
Fmt Bot
Jose Storopoli
Nick Johnson
Shing Him Ng
Jesus Christ
yancy
calciumbe
Chris Hyunhum Cho
Rob N
Sanket Kanjalkar
Max Fang
LLFourn
junderw
Diogo Canut
Leo Nash