Recently we made an attempt at making the hex APIs for scripts easier to
use, better documented, and shown via an example.
After that work we decided it would be better if `LowerHex`/`UpperHex`
did not have the prefix. We also wanted to further clarify the inherent
function names to make the all explicit.
See GitHub issue #4316 for the thread of discussion.
Note that this PR does not require changes to the serde regression test
which were non changed in the original work either.
Add the length prefix when formatting hex strings by way of `LowerHex`
and `UpperHex`.
This looses formatting options because I can't remember right now how
not to - again.
Add to the test to cover the `fmt::Debug` impl without checking the
actual text.
Change one of the opcodes so that the display of the special case OP_0
is checked.
The tests for both `Script` and `ScriptBuf` `as_ref` and `as_mut` only
covered the `u8` impl.
Expand the tests to cover the `Self` impl as well.
The allow useless_asref is required to remove a clippy lint. Removing
the "useless" as_ref also removes the test coverage of the as_ref or
as_mut impl.
a5f904559d primitives: Make hex optional (Tobin C. Harding)
Pull request description:
Make the `hex` dependency optional. This means not implementing
`Display` for some types if `hex` is not enabled and only implementing
`Debug`.
Also without `hex` enabled:
- We loose the ability to parse an `OutPoint` from string because we
can no longer parse `Txid`.
- We loose the hex formatting of witness elements.
Note also that `primitives` builds with the `serde` feature even if
`hex?/serde` is excluded from the `serde` feature. I found this
surprising.
Close: #4183
ACKs for top commit:
apoelstra:
ACK a5f904559d3b5d2dfbcbed8b1746305e32103fee; successfully ran local tests
Tree-SHA512: c105e089508036af8251cb923f3eda163b8f2d6151ea043aa1489edb6ce396975d1f598a240f4ca6e48f6ef780774f7d86cb70ec9399f3d2ff87c0ac53ceee91
Make the `hex` dependency optional. This means not implementing
`Display` for some types if `hex` is not enabled and only implementing
`Debug`.
Also without `hex` enabled:
- We loose the ability to parse an `OutPoint` from string because we
can no longer parse `Txid`.
- We loose the hex formatting of witness elements.
Note also that `primitives` builds with the `serde` feature even if
`hex?/serde` is excluded from the `serde` feature. I found this
surprising.
Since the introduction of `Script` `unsafe` started slowly creeping in
as more types with similar semantics were added. The `unsafe` in these
cases is just for trivial conversions between various pointer-like
types. As such, it's possible to move these into a single macro that
takes care of the conversions at one place and avoid repeating the same
`unsafe` code in the codebase. This decreases the cost of audits which
now only need to happen in `internals`, focuses any changes to happen in
that single macro and decreases the chance that we will mess up
similarly to the recent `try_into().expect()` issue (but this time with
UB rather than panic).
The new macro accepts syntax very similar to the already-existing struct
declarations with these differences:
* The struct MUST NOT have `#[repr(transparent)]` - it's added by the
macro
* If the struct uses `PhantomData` it must be the first field and the
real data must be the second field (to allow unsized types).
* The struct must be immediately followed by an impl block containing at
least on conversion function.
* If the struct has generics the impl block has to use the same names of
generics.
* The conversion functions don't have bodies (similarly to required
trait methods) and have a fixed set of allowed signatures.
* Underscore (`_`) must be used in place of the inner type in the
conversion function parameters.
The existing code can simply call the macro with simple changes and get
the same behavior without any direct use of `unsafe`. This change
already calls the macro for all relevant existing types. There are still
some usages left unrelated to the macro, except one additional
conversion in reverse direction on `Script`. It could be moved as well
but since it's on a single place so far it's not really required.
Enable all the pedantic lints and fix warnings.
Notable items:
- `enum_glob_used` import types with a single character alias
- `doc_markdown`: add a whitelist that includes SegWit and OpenSSL
43ae9d7516 primitives: Hide script error internals (Tobin C. Harding)
2d8227f091 Hide relative locktime error internals (Tobin C. Harding)
Pull request description:
Make the struct fields private and add getters.
ACKs for top commit:
apoelstra:
ACK 43ae9d751622c7bef548a469466d74cf01284129; successfully ran local tests; nice! Way easier to understand these types with the new incompatible / expected names
Tree-SHA512: cfe67d60ea61a2a4c27b09071a6b11739ca281bf0b4a655121f90215ce38c3a637acf53a6e01aa2ef26fa80004cd919bf3b3334dbd9566ee2f594cab7750b563
As part of the 1.0 effort and forward maintainability hide the internals
of the two error types in the `script` module. Add getters to get at the
invalid size.
These were re-implementing hashing after the check rather than calling
the `_unchecked` method, so this replaces the manual implementation with
the method.
The Electrum protocol uses hashes of `script_pubkey` that might look
similar to the ones we have in the crate and could be confused. This
change notes that to hopefully avoid the confusion.
Resolves https://github.com/rust-bitcoin/rust-bitcoin/discussions/3997
These methods are either newtype casts that should compile to no-ops or
directly calling into some other function with at most some pointer
adjustments. As such making them `#[inline]` is definitely benefitial.
There are also methods that check length and then call some other
function. These are also worth inlining since the length could be known
at compile time and the check could be eliminated.
We have several trait implementations for `Script` and `ScriptBuf` in a
common module so that it's easy to verify that they are same but `Deref`
and `DerefMut` should *not* be implemented for `Script` so having them
in the common module is not helpful. This moves them to the appropriate
`Owned` module.
The newtype sanity rules (a name I came up with):
* Newtypes should have at most one constructor that directly references
the inner field.
* Newtypes should have at most three accessor methods that directly
reference the ineer field: one for owned access, the second for
borrowed and the third for mutably borrowed.
* All other methods should use the methods above to perform operations
on the newtype and not directly access the fields.
This commit makes `Script` and `ScriptBuf` obey these except for
`reserve` and `reserve_exact` since we don't have `as_mut_vec` method.
As a side effect it also adds `const` to `ScriptBuf::from_bytes`.
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 all the leaf crates and just write the
code.
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`.
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
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.
Currently we feature gate code within the `impl_to_hex_from_lower_hex`
macro on "alloc" but `bitcoin` does not have the "alloc" feature so
this code is never built in. This can be seen by the lack of a
`to_hex` function on `LeafVersion`.
Remove the feature gate from the macro and put it on the individual
call sites as needed.
Tobin C. Harding
Fmt Bot
Jamil Lambert, PhD
merge-script
Martin Habovstiak