See the previous commit message for justification; for sensible
arithmetic on block timestamps we need the ability to do MTP
calculations on arbitrary MTPs and arbitrary intervals between them.
However, the absolute::Mtp and relative::MtpInterval types are severely
limited in both range and precision.
Also adds a bunch of arithmetic ops to match the existing ops for
BlockHeight and BlockInterval. These panic on overflow, just like the
underlying std arithmetic, which I think is reasonable behavior for
types which are documented as being thin wrappers around u32.
We may want to add checked_add, checked_sub and maybe checked_sum
methods, but that's out of scope for this PR.
For our relative locktime API, we are going to want to take differences
of arbitrary MTPs in order to check whether they meet some relative
timelock threshold.
However, the `locktime::absolute::Mtp` type can only represent MTPs that
exceed 500 million. In practice this is a non-issue; by consensus MTPs
must be monotonic and every real chain (even test chains) have initial
real MTPs well above 500 million, which as a UNIX timestamp corresponds
to November 5, 1985.
But in theory this is a big problem: if we were to treat relative MTPs
as "differences of absolute-timelock MTPs" then we will be unable to
construct relative timelocks on chains with weird timestamps (and on
legitimate chains, we'd have .unwrap()s everywhere that would be hard to
justify). But we need to treat them as a "difference of MTPs" in *some*
sense, because otherwise they'd be very hard to construct.
There is a lot of duplicated code between BlockHeight and BlockInterval.
It obfuscates the differences between them: which timelock types they
can be converted to/from and what their arithmetic properties are.
a5d96ceb39 hashes: remove private `internal_new` method (Andrew Poelstra)
0aeff359f5 hashes: reformat (Andrew Poelstra)
aadd7df266 hashes: move `from_engine` fn impl out of macro body (Andrew Poelstra)
Pull request description:
This is a followup to https://github.com/rust-bitcoin/rust-bitcoin/pull/4010 which does some simple cleanups to the hash macros and some function signatures.
ACKs for top commit:
tcharding:
ACK a5d96ceb39
Tree-SHA512: e8c3d8770fe3a49da4eb2d548af86cbe6e0e17efcac419815f4953b7dafffbd3e5c0be65574e08c86d09fe594d95512dfd7837534be429b1490fea973ec4d5e6
53d32c9e4f bitcoin: remove torv2 support (Bruno Garcia)
Pull request description:
This PR removes support to TorV2 since it's deprecated and no longer useful to have it.
ACKs for top commit:
apoelstra:
ACK 53d32c9e4f0ef0f3b2c7d4dcba42e3ac5344f78a; successfully ran local tests
tcharding:
ACK 53d32c9e4f
Tree-SHA512: 69a2ba399d5eac7f132519ab83362fbd8739d9e975795e441cefa75896ddbf4041db2125ffde51316f9ad69aa0b62c8b226ccff042b0dae6d3c615826bc339f4
44d01d0ce4 Fix broken changelog links (GarmashAlex)
Pull request description:
Fixed malformed markdown links in bitcoin/CHANGELOG.md that were causing errors. Specifically, removed double parentheses from links to primitives and units CHANGELOG.md files, ensuring proper rendering and accessibility of the documentation.
ACKs for top commit:
apoelstra:
ACK 44d01d0ce468549c64f8362c552c434eb1c3503b; successfully ran local tests
Tree-SHA512: e2d2598070c3df8e4dfa59c019d8ab9a1f86731a70624ceef80b68849454bd3828c558ef8eb30bb9d56c648fcacccea459d2a7a536021d65c2fbcbe5595916ce
055ab7e684 Remove impl Display from mutants exclusion list (Jamil Lambert, PhD)
Pull request description:
The output of Display should not change in stable crates for types that have well defined formatting and ones that implement FromStr. Crates that are included in the mutation testing have been updated to have regression tests for all relevant Display implementations in #4420 and #4422.
Remove the exclusion so that the Display implementations are included in mutation testing.
Together with #4420 and #4422Closes#4415
ACKs for top commit:
tcharding:
ACK 055ab7e684
apoelstra:
ACK 055ab7e684e53783b56139d14779fb39268e8f3c; successfully ran local tests
brunoerg:
code review ACK 055ab7e684
Tree-SHA512: 703aeee57ec9459b8cef38adbbf04d7bcb2bc425db22512e6936a5a9f6a51c740f62b03ab070aed11bd17740ae6b9f510ec743fd683f98cb03e0a1d086a4784e
This method is used to implement `from_byte_array`. But there is no need
for the method to exist. We can just inline it in `from_byte_array` and
reduce the amount of indirection in our macros.
Also make the same change in sha256t.
This macro is pretty weird -- it requires that a freestanding
`from_engine` method exists, which it uses to implement a `from_engine`
method within an impl block, by just calling through to the freestanding
method.
To reduce indirection, at a very small cost in increased repeated code
(we now need to add a `impl Hash {` and `}` and doccomment around each
freestanding function, we just remove this from the macro entirely.
This will allow us to implement `from_engine` for `sha256t::Hash` in a
different way than for the non-generic hash types.
To minimize the diff, we do not re-indent the freestanding
`from_engine`. We will do that in the next commit. However, the diff is
still a bit noisy because I had to replace `fn from_engine` with `pub fn
from_engine` in every case. I took the opportunity to also change the
return type from `Hash` to `Self` to make it clearer that these are
constructors.
Fixed malformed markdown links in bitcoin/CHANGELOG.md that were causing errors.
Specifically, removed double parentheses from links to primitives and units
CHANGELOG.md files, ensuring proper rendering and accessibility of the documentation.
826acb8273 units: rename relative::HeightInterval constructors (Andrew Poelstra)
39b4f7670d units: rename relative::Height to HeightInterval (Andrew Poelstra)
d3619cc1bc units: deprecate relative::MtpInterval::to_consensus_u32 (Andrew Poelstra)
1a6b8b4c7a units: rename relative::Time to MtpInterval (Andrew Poelstra)
39b057fade units: rename absolute::Height consensus functions (Andrew Poelstra)
5a8f33f380 units: rename absolute::Mtp consensus functions (Andrew Poelstra)
8ffcd2cf30 units: rename absolute::Time to absolute::Mtp (Andrew Poelstra)
Pull request description:
This PR does a whole bunch of renames to the `units` locktime modules. These modules contain the underlying units for locktime types and hopefully aren't used directly too often, which should minimize the disruption from these renames.
This fixes a number of issues:
* Confusion between blocktimes and MTPs (which in fairness, *are* blocktimes, but they're not the one that users are likely to reach for)
* Constructor and conversion names that imply there is a "consensus encoding" for these bare units, which corresponded to the consensus encoding of the corresponding locktimes/sequence numbers
* `from_*` methods without `to_*` methods and vice-versa (overlaps with the above)
* the horribly named `value` method on relative heights and times (but not absolute ones)
This PR does **not** remove the `MtpAndHeight` type, nor does it add constructors for `Mtp` from lists of blocktimes. This is because the PR was too big already and I felt I should split it up.
Alternate to #4427
ACKs for top commit:
tcharding:
ACK 826acb8273
Tree-SHA512: 6e0491e17927625cde85c2cf92ff152a10613e632474122a626ee31b662d21c09fcb9fa3014c44708c97536535a33845cbbcd81e73dcdf98e9ee9fd6143c698f
3bb6c73f2d Add methods to retrieve inner types (Shing Him Ng)
Pull request description:
Resolves#4345
ACKs for top commit:
tcharding:
ACK 3bb6c73f2d
apoelstra:
ACK 3bb6c73f2d3edd1165b7b7f3a833fa471786e166; successfully ran local tests; should backport to 0.32.x
Tree-SHA512: c89017bbc2126ec62c756c4ee9b49dcc8b94a3063a8155aadcf7c69a6f0bc9337baedffe7f52a4ab6f0b738302bea683391d394483c4c7eefbb622b97d34d26c
Rename `value` to `to_height` to be symmetric with `from_height`;
deprecate `to_consensus_u32` which had no symmetric `from_consensus_u32`
and was only used to implement the corresponding methods in primitives
and bitcoin.
This is disruptive, but makes the type name consistent with
`MtpInterval` and also greatly improves clarity, helping to distinguish
between absolute and relative locktimes and reminding the author (and
reviewer) of locktime code that this needs to be a diff.
This method is weird. It's basically just used internally to implement
the locktime methods in `primitives` and `bitcoin`. It has no symmetric
from_consensus_u32.
Conversely the constructors from 512-second intervals have no symmetric
to_* method -- the inverse of these functions is called `value`, which
is a meaningless and undiscoverable name.
As with absolute::Mtp, there is no "consensus encoding" of a block
height, except that obtained by converting it to a locktime. For
symmetry with `Mtp`, rename the methods.
16f6fc6b5b Improve docs on absolute::LockTime::is_implied_by (Tobin C. Harding)
Pull request description:
If one wishes to verify a script that contains CLTV is valid in a transaction then one must compare the argument to CLTV (the locktime) to the transaction locktime. And to be valid the CLTV locktime must be less than or equal to the transaction locktime. This usage kind of lends itself to the term 'implied by' and we have a function already `is_implied_by` that does exactly this.
Improve the docs by adding a section mentioning this usecase.
ACKs for top commit:
apoelstra:
ACK 16f6fc6b5b469741c076bc3a4d92f7d4e619ffc7; successfully ran local tests
Tree-SHA512: f7de177927150ae27b3db187465e0f699da9e8e00a490b0354176b2dedc34d731fda40bf4d55324aa28b0d55f2f036653ab14d99b82d56705c929986db8fb30e
f646afdb81 Fix typo in githooks warning message (strmfos)
Pull request description:
Replaced “re-reun” with “re-run” in the post-merge hook notice to provide clear instructions for reinstalling project githooks.
ACKs for top commit:
apoelstra:
ACK f646afdb8189f738d7c31a0eda7c2abc537c1239; successfully ran local tests
Tree-SHA512: 7943dd801b1548846d54f08260090a4f5b2264d7d7527483af207bd03a0a90079642d0775fd8409ed0f7b89df6d994e2028a877105da6b594135cbdf37ff6356
There is no "consensus encoding" for a MTP. The intention for these
methods was that a user could interpret the MTP as a locktime and then
consensus-encode that locktime. However, it was instead interpreted as
the MTP representing a *blocktime* as it is consensus-encoded in a block
header.
Evidence of this misinterpretation is in several doccomments, which
casually refer to the Mtp (which used to be just called Time) as a
"block time", which is simply incorrect.
This is not a generic UNIX timestamp, but rather a MTP restricted to
have values between 500 million and u32::MAX. Most importantly, it is
*not* a blocktime, which is what is implied by its name and
constructors.
7bcdd5ff66 Add regression tests for Display impl (Jamil Lambert, PhD)
9c90bf49c9 Add lock by time and denomination display regression tests (Jamil Lambert, PhD)
eb7dee831e Rename locktime tests (Jamil Lambert, PhD)
Pull request description:
The output of Display should not change in stable crates for types that have well defined formatting and ones that implement FromStr. Error types do not need to be tested. Discussed in #4415.
Patch 1 is a simple rename to allow for clearer naming when both height and time locktimes are tested.
Patch 2 adds the missing time locktime and denomination `Display` and round trip tests to the existing macro
Patch 3 adds all the other missing `Display` tests in `units`
ACKs for top commit:
tcharding:
ACK 7bcdd5ff66
apoelstra:
ACK 7bcdd5ff6655576dac79e14b9c1a50c74db310a2; successfully ran local tests
Tree-SHA512: af8810af1282cfa94ddf8d2badfa0984c190d1d20812c45b725d4b5467e2ac971eadbd0d821c24e81be8708a167782f9c1274b1ad228d0863d6560c2174537c0
82da8a599e Add regression tests for Display impl (Jamil Lambert, PhD)
Pull request description:
The output of `Display` should not change in stable crates for types that have well defined formatting and ones that implement `FromStr`. Error types do not need to be tested. Discussed in #4415.
Add missing tests for all implementations in `primitives` and round trips for types that implement `FromStr`.
ACKs for top commit:
tcharding:
ACK 82da8a599e
apoelstra:
ACK 82da8a599e4a046b8da7763e8aaeff28becc49a6; successfully ran local tests
Tree-SHA512: ca70ee81e8a76bee27bc0314f2bc718893190c7cc37e5a357244f0925bc3fb675cb2e096aaf764a35216ea7c544f491bd071034f23f97cfa55e3bf50f47b2d84
For TweakedKeypair, `to_inner` is also renamed to `to_keypair` to maintain
consistency. Similarly, `to_inner` is renamed to `to_x_only_pubkey` for
TweakedPublicKey
If one wishes to verify a script that contains CLTV is valid in a
transaction then one must compare the argument to CLTV (the locktime) to
the transaction locktime. And to be valid the CLTV locktime must be less
than or equal to the transaction locktime. This usage kind of lends
itself to the term 'implied by' and we have a function already
`is_implied_by` that does exactly this.
Improve the docs by adding a section mentioning this usecase.
The output of `Display` should not change in stable crates for types
that have well defined formatting and ones that implement `FromStr`.
Error types do not need to be tested.
Add missing tests for all implementations in `units`.
The output of Display should not change in stable crates for types that
have well defined formatting and ones that implement FromStr.
Crates that are included in the mutation testing have been updated to
test all relevant Display implementations.
Remove the exclusion so that the Display implementations are included in
mutation testing.
The existing display regression tests only tested height locktimes.
Denomination display regression and round trip were not tested.
Add tests for time locktimes and denomination.
Existing display tests only test lock by block height. New tests are
needed for lock by block time.
Change existing test names to make it clear which type of locktime is
being tested.
The output of `Display` should not change in stable crates for types
that have well defined formatting and ones that implement `FromStr`.
Error types do not need to be tested.
Add missing tests for all implementations in `primitives` and round
trips for types that implement `FromStr`.
3bcb5b0f8d Add exclusions for match arm/guard mutants (Jamil Lambert, PhD)
Pull request description:
cargo-mutants 25.0.1 introduced a new mutation pattern that deletes an arm of a match statement or replaces a match guard with true and false. These are applied in sections that have been excluded from mutation testing and cause false positives.
The match patterns are also now stricter causing previously excluded `impl` to be included.
Exclude the new match arm and guard mutation patterns.
Add `impl fmt::` versions for Debug and Display.
Closes#4402
ACKs for top commit:
apoelstra:
ACK 3bcb5b0f8d7e1750675b2aba09f487fb4daab97b; successfully ran local tests
tcharding:
ACK 3bcb5b0f8d
Tree-SHA512: d7a45bdd5bc844c273956922b5483c70f24ca8ea20da275fd6b1ff13e71ec86fcc487932365beb3084cc3894bbe1317ddb3df975f90584b639187ba278b0721e
b9a12043b0 bip32: return error when trying to derive a path too deep (Andrew Poelstra)
73781e047b bip32: rename Error to ParseError (Andrew Poelstra)
a66ad97fb6 bip32: split InvalidChildNumber and InvalidChildNumberFormat out of error (Andrew Poelstra)
a891fb9b74 bip32: remove unused error variants (Andrew Poelstra)
f0a237c001 bip32: split out DerivationError from the main error enum (Andrew Poelstra)
32d96f6c33 bip32: make Xpriv::new_master be infallible (Andrew Poelstra)
0e5e021b69 bip32: change several cryptographically unreachable paths to expects (Andrew Poelstra)
Pull request description:
This PR makes a first pass at splitting the `bip32::Error` type into multiple distinct types -- one for derivation (which can fail if you try to derive a hardened child of an xpub, or if you try to derive too many layers), one for parsing child numbers or derivation paths, and one for parsing xkeys. Along the way it cleans up a ton of weird things and typos, e.g. the psbt `GetKeyError` having an unused `Bip32` variant whose display text references "bip 23".
Because all the error types get renamed, every part of this PR is an API break, but only the last commit is a "real" API break which uses the new `DerivationError::MaximumDepthExceeded` error variant to return an error when trying to derive a path of length 256 or longer. This means that `Xpriv::derive_xpriv` again returns an error result.
I will make a simpler version of this last commit suitable for backporting to 0.32.x. (In 0.32.x `Xpriv::derive_priv` returns an error, so we can change it to error out on max-depth-exceeded without breaking the API. Sadly most users are likely to be unwrapping the error because in 0.32.x currently the error path is cryptographically unreachable...but at least this way the panic will be in their control rather than ours.)
Fixes https://github.com/rust-bitcoin/rust-bitcoin/issues/4308
ACKs for top commit:
tcharding:
ACK b9a12043b0
Tree-SHA512: 688826126ff24066c6de9de3caa73db68c516ba8893d64d9226a498774a2fb9db7d7fd797375c6b3f820588c178632e1e6e8561022dfa7042a560582bf1509b4
The bip32::Error enum is now exclusively used for errors related to
parsing and decoding. It is still a little messy (mainly: it contains a
base58 variant which is used when parsing a string but not when decoding
from bytes) but much cleaner than it was.
f31b7329c6 changed wording and punctuation (crStiv)
Pull request description:
nothing fancy, just changed wording and improved punctuation
ACKs for top commit:
apoelstra:
ACK f31b7329c6b84c342dea867c43ae822674a3cf8a; successfully ran local tests
tcharding:
ACK f31b7329c6
Tree-SHA512: 78daae62a7235dcf238687548f2b2778120aede3f5237e83a9015b87a112946b7ff4177cfbbab6206ea09f41cc9d08b3684f169beeba314f6afc1c917920f788
cargo-mutants 25.0.1 introduced a new mutation pattern that deletes an
arm of a match statement or replaces a match guard with true and false.
These are applied in sections that have been excluded from mutation
testing and cause false positives.
The match patterns are also now stricter causing previously excluded
`impl` to be included.
Exclude the new match arm and guard mutation patterns.
Add `impl fmt::` versions for Debug and Display.
5d5a19793a Run the formatter (Tobin C. Harding)
2b72f1f30b Make Lower/Upper hex print scripts with len prefix (Tobin C. Harding)
c27b95fb0d Make script to/from hex use consensus encoding (Tobin C. Harding)
f4bc58dc48 bitcoin: Add a script encoding example (Tobin C. Harding)
Pull request description:
Done while investigating #3910 - this PR is quite invasive.
- Patch 1 adds an example that shows the current behaviour of various API calls for converting scripts to/from hex.
- Patch 2 adds a pair of functions that do not use the length prefix and makes script to/from_hex use the prefix.
- Patch 3 makes `LowerHex` and `UpperHex` use the len prefix also
- Patch 4 runs the formatter
Explicitly keeps serde untouched - i.e., without the len prefix
ACKs for top commit:
apoelstra:
ACK 5d5a19793aae73ff09d7064455a1a995eb796c28; successfully ran local tests
Tree-SHA512: dbbec372ea7b01818fce68ffb807a4531f068e10147e8bedf37b77f66a068a628e380549c379c061b868973e97806ada59729248b03bbd1cf6809f6098170cb6
8b47068a2e feat(locktime): implement MtpAndHeight structure and validation logic (aagbotemi)
Pull request description:
This PR fixes#4299
- Computed MtpAndHeight structure
- Checked if relative time and height is satisfied by MtpAndHeight
- Compared the Ordering of MtpAndHeight with time and height
- Checked MtpAndHeight satisfaction and comparison in Locktime
- Added unit tests for all the implementation
I've reviewed and adhered to the contribution guidelines
ACKs for top commit:
apoelstra:
ACK 8b47068a2efada30aec21c61ae4be0da4d8e8fc8; successfully ran local tests
Kixunil:
ACK 8b47068a2e
tcharding:
ACK 8b47068a2e
Tree-SHA512: b00d1384d5deaa038b486ca9d77ad33cfa6cd8c987e08407863f2be8d540014bdcc971cd9d46acb51a2d105341accc04ba151e5cccb276e8352a5d45b33097eb
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.
I'm not sure why we do not use consensus encoding currently for encoding
and decoding scripts to/from hex strings. Many tests include hard coded
hex which do not include the length prefix.
- Add a pair of encoding functions to encode/decode to/from hex without
the length prefix.
- Make `to_hex` and `from_hex` expect the length prefix i.e., use
consensus encoding.
This makes the API easier to use because the various encoding APIs can
be use together now eg `consensus::encode_hex` and `ScriptBuf::from_hex`.
Our encoding/decoding support for scripts is currently a bit convoluted.
Add an example that shows how all the APIs go together.
Note this highlights a potential problem. `to_hex_string` is in
`bitcoin` and therefore has access to consensus encoding but does not
include the length prefix. This means that the output string from
`to_hex_string` cannot be parsed with
`consensus::encode::deserialize_hex` - which is surprising and has lead
to difficulty by at least one user trying to debug.
Andrew Poelstra
Bruno Garcia
GarmashAlex
merge-script
Fmt Bot
strmfos
Shing Him Ng
Jamil Lambert, PhD
crStiv