13d94cbc47 Remove no_run (Tobin C. Harding)
Pull request description:
`no_run` is not needed since we already mark this up as `bash` which rustc doesn't run when running examples.
While the keyword `bash` is not currently supported it may well be in the future and since only the `rust` keyword causes code to run any other string is effectively a wildcard, `no_run` is therefore meaningful only as a convention. Lets keep `bash` in case support is added later on.
cc sr-gi
ACKs for top commit:
sr-gi:
ACK 13d94cbc47
Kixunil:
ACK 13d94cbc47
sanket1729:
ACK 13d94cbc47
apoelstra:
ACK 13d94cbc47
Tree-SHA512: 90cbe8c5ea30577fc148d8e8b388008ea7f6c4975ea8b9e249e6058de901afca084d59e6a89eb9bda0a8e3112fb89336b622acd23046299c5e5ad7723ae26148
In some protocols it is preferred to serialize consensus-encodable types
using consensus encoding. E.g. serialize `Transaction` as hex-encoded
string in Json in Bitcoin Core RPC protocol. This change provides
adapter to make this easier.
The adapter allows providing custom byte-to-string encoder for more
exotic cases and provides a hex implementation which should be useful in
majority of the cases.
Should help with #765
My local scripts did not test serde feature on merge commit. While
merging 734, I accidently broke the serde feature on latest master. This
PR fixes it.
962abcc963 Add serde regression tests (Tobin Harding)
Pull request description:
Attempts to add regression tests for _all_ types defined in `rust-bitcoin` that implement `Serialize`/`Deserialize`.
- Add a `tests` directory and implement regression tests in there
- Use files for input hex and output bincode to reduce source file clutter
- Copy test block and `include_bytes!` usage from RCasatta's [PR](https://github.com/rust-bitcoin/rust-bitcoin/pull/750)
- Uses Kixunil's macro suggested below
- Adds a single regression test to `util/taproot.rs` for private types
## Note to reviewers
- Uses JSON for opcodes in a separate file (`tests/regression_opcodes.rs`), for all other tests uses bincode.
- Bypasses the order issue for maps by only serializing maps with a single element - is this correct?
Fixes#723
ACKs for top commit:
apoelstra:
ACK 962abcc963
sanket1729:
ACK 962abcc963. This has been open for a long time. Merging this in the interest of progress.
Tree-SHA512: e34e48e1c56fab5898bc74e7fb867435ed387d828dd3daf0c7d6df8f305e1da6883e91487115ac428618eb7d95bd16aa2cd209ca219684959bc95587ef0b4083
4c8570b512 base58: Run formatter (Tobin C. Harding)
2780e6cdaa Move base58 module to crate root (Tobin C. Harding)
Pull request description:
Move `base58` module to the carte root, direct `rustfmt` to not format the digits array. Run formatter as a separate patch.
ACKs for top commit:
apoelstra:
ACK 4c8570b512
sanket1729:
ACK 4c8570b512
Tree-SHA512: 3ab7afe089b9866a59f3f67c74c3e6657ca51d2d0ea1aaee0218a400b90826d36eaca7d491006a9448359ae3de5755d20b2d2df71e0fa3d6497fbaca270dcf42
`no_run` is not needed since we already mark this up as `bash` which
rustc doesn't run when running examples.
While the keyword `bash` is not currently supported it may well be in
the future and since only the `rust` keyword causes code to run any
other string is effectively a wildcard, `no_run` is therefore meaningful
only as a convention. Lets keep `bash` in case support is added later on.
In preparation for removing the `util` module move the `base58` module
to the crate root. This is likely not the final resting place for this
module but it is a step in the right direction.
Includes addition of rustfmt attribute to skip formatting the digits
array. No other changes to the `base58` module.
For some applications, such as block explorers, it's useful to be able
to obtain the public key used in case of P2PK. This is considered
general enough for addition into bitcoin library,
so this change adds it.
The public key is returned only when it's valid and the script is P2PK.
To avoid duplicating the logic checking whether a script is P2PK the
logic is moved to a new p2pk_pubkey_bytes() method which returns raw
bytes and is then called from both is_p2pk() and p2pk_public_key()
We are trying to flatten the `util` module. The `taproot` module can
live in the crate root. If/when we create a `crypto` module/crate we may
wish to pull some stuff out of this module but for now moving it gets us
closer to removing `util` without making the directory structure any
worse.
Includes adding rustfmt attributes to skip formatting of macros.
2df51dae15 Create crypto module (Tobin C. Harding)
Pull request description:
Done as part of [util flattening](https://github.com/rust-bitcoin/rust-bitcoin/issues/639).
Create a `crypto` module and move into it (out of `util`):
- ecdsa
- schnorr
- key
After review, this PR now includes some type re-names
- EcdsaSig -> ecdsa::Signature
- SchnorrSig -> schnorr::Signature
- EcdsaSigError -> ecdsa::Error
- SchnorrSigError -> schnorr::Error
- InvalidSchnorrSigSize -> InvalidSignatureSize (this is an error enum variant)
ACKs for top commit:
apoelstra:
ACK 2df51dae15
sanket1729:
ACK 2df51dae15
Tree-SHA512: 7cf63d51ed5fdc737cd59767d9bb96b1e3501634e3aee855493f6a51ad5c5397ce4b25c77f9929abd70d6ceb351fc6520e835da108f4c9a46df5b9c2b52ca6b3
f7a6d17143 Add rand feature flag to the example documentation (yancy)
Pull request description:
It's confusing trying to follow the documentation [here](https://docs.rs/bitcoin/latest/bitcoin/util/address/index.html) unless you know to enable the `rand` feature flag. This PR updates the docs so people know to enable the flag.
```
22:35 < control> hello, how can i generate a simple wallet address using rust bitcoin? cant find working example
22:45 < andytoshi> control: do you have a private key?
22:45 < andytoshi> what kind of address do you want to generate?
22:46 < andytoshi> my guess would be that you want to use bdk rather than rust-bitcoin directl
22:48 < control> P2PKH address and generate private key. just as simple as bit library in python does
22:52 < control> im trying to run this example
22:52 < control> https://docs.rs/bitcoin/latest/bitcoin/util/address/index.html
22:52 < control> but it says ^^^^ could not find `rand` in `secp256k1`
```
ACKs for top commit:
apoelstra:
ACK f7a6d17143
Kixunil:
ACK f7a6d17143
Tree-SHA512: 6d94f8ffa7797d1e7720a840d4f8bb0ac274507118597dff60631ad3d28dbe57e2341b87d6101e2a9f5fbcc2b1f0beb6d06f9dde48a480cb10a9a45c887f83a4
Kani can't really handle string processing, and appears to be unable
to check integer multiplication (for now), but we do several checks
for addition and subtraction, and conversion between signedness,
that Kani can easily prove.
Done as part of flattening util.
Currently in `util` module we have a bunch of modules that provide
cryptography related functionality.
Create a `crypto` module and move into it the following:
- ecdsa
- schnorr
- key
To improve uniformity and ergonomics, do the following re-names while we
are at it:
- EcdsaSig -> ecdsa::Signature
- SchnorrSig -> schnorr::Signature
- EcdsaSigError -> ecdsa::Error
- SchnorrSigError -> schnorr::Error
- InvalidSchnorrSigSize -> InvalidSignatureSize (this is an error enum variant)
519db4d951 add Network::to_core_arg() method (connormullett)
Pull request description:
closes: #1207
Adds converting `Network` to its `bitcoind` equivalent.
The arguments for -chain can be found in the documentation and is one of the following:
main, test, signet, regtest
ACKs for top commit:
apoelstra:
ACK 519db4d951
Kixunil:
ACK 519db4d951
Tree-SHA512: 5fd805d654f7c30f87ff877fe90e19490d0deb73b46ce87cc6b43d30595eb9d2de3f646f58a5d72180c3e8cc6a9b614bfe6753ecd6c21b8d193a8d862e3f887f
108a1f73ca Fail CI if docs build throws warnings (Tobin C. Harding)
b014f0fdcb Fix rustdocs build warnings (Tobin C. Harding)
Pull request description:
Currently we do not fail the CI script if the docs build throws warnings, since we are a group of super anal, easily triggered, code cleanliness obsessed devs this causes a mild rash to develop on the lower back [0]. We can easily fix this by checking for build warnings in CI.
[0] - Amusingly my rash has been playing up since Friday but I thought I'd fixed the warnings in an open PR someplace so I was ignoring it, seeing Kixunil's [issue](https://github.com/rust-bitcoin/rust-bitcoin/issues/1403) this morning prompted me to fix it :)
Fix#1403
ACKs for top commit:
Kixunil:
ACK 108a1f73ca
apoelstra:
ACK 108a1f73ca
Tree-SHA512: 0f86c318b2ec8bf7aa6a0d0f355f8fe8e3eb8ad5eb74d95f8dab882d6729c386c3e0ef4cc2378645e15460ff2b9b47d66e3603958f8b188f5e2b07272739d755
Convert all rustdocs build warnings to errors using `-D warnings` and
exit the script with 1 to signal the error.
While we are at it use `--all-features` instead of explicit feature list
when building docs. Without this the docs build fails with:
error: too many file operands
64495cc5fe Drop Network arg from max_money() (Antoine Riard)
Pull request description:
Amount of coins available stay in the same across Bitcoin network: signet, testnet, mainet. From my understanding this is a leftover from some potential multi-chain support.
For more context: https://github.com/lightningdevkit/rust-lightning/pull/1839#discussion_r1019753069
If there is already an existent PR, it can be closed, however didn't find one.
ACKs for top commit:
apoelstra:
ACK 64495cc5fe
tcharding:
ACK 64495cc5fe
Tree-SHA512: 929011ee73c5eda903fb0140438ed5e88c8f5b7378036a87a6a660a8b9138bf204bf59a0ba822c0cd98e37e97d2d0dbbf8c9893a834da9acdf817ba43a5ed5b6
e04a7a926d network: Run cargo fmt (Tobin C. Harding)
dc33c7999f Enable formatting of the network module (Tobin C. Harding)
81f69e846b Exclude from formatting stuff in the network module (Tobin C. Harding)
408d7737fb Run cargo fmt (Tobin C. Harding)
1ecf09359b Add local variable to reduce line length (Tobin C. Harding)
b2e74bc050 Exclude long function call (Tobin C. Harding)
ff5a80dbd3 Exclude formatting of function fmt_satoshi_in (Tobin C. Harding)
308a12b7cf Exclude array from formatting (Tobin C. Harding)
fb7ff46ccc Improve crate root re-exports (Tobin C. Harding)
Pull request description:
The `network` module is not currently included in formatting. Also, at this point in time not much is happening in the `network` module so formatting it should not cause too many merge conflicts with other in-progress PRs.
The first 3 patches are formatting preparation of the repo, the next 2 are formatting preparation of the `network` module. The last patch is the result of running `cargo +nightly fmt`. Can one reviewer please verify the last patch consists only of `rustfmt` changes by running the command on their branch. Thanks
cc luckysori :)
ACKs for top commit:
apoelstra:
ACK e04a7a926d
Tree-SHA512: 49b2873f0bfdd448df97073b462fa860c85b7f3c3094fdb0e16fd86aa467156ea8b2ceec61e2ee99848802cd171fd8abbc17e45c66672889a37c413fa4bea636
9674bf29fe hashes: Fix clippy warnings (Tobin C. Harding)
b9643bf3e9 Import bitcoin_hashes crate into hashes (Tobin C. Harding)
580feab3f9 internals: Add CHANGELOG file (Tobin C. Harding)
bae64e156e Move CHANGELOG to bitcoin crate (Tobin C. Harding)
9a2c856be6 Update gitignore file (Tobin C. Harding)
Pull request description:
#1337 was split out of this in an attempt to help review, I think we can merge this one though now it has some traction.
We would like to bring the `bitcoin_hashes` crate into the `rust-bitcoin` repository. https://github.com/rust-bitcoin/rust-bitcoin/issues/550#issuecomment-1248071843
Import `bitcoin_hashes` crate into `hashes`.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 2a78c250f78d391599040223870a4d1d6f6f5482
Please note the commit history of `bitcoin_hashes` is only preserved on the soon-to-be-archived `bitcoin_hashes` repository.
ACKs for top commit:
sanket1729:
ACK 9674bf29fe. Did my best to review the CI code, it looks good, and seems like we are testing everything when we add "hashes" to the root level `./contrib/tests.sh`. I would like other reviewers to confirm the same.
apoelstra:
ACK 9674bf29fe
Tree-SHA512: db3ce6adeb38430c5a9f372da16be9fb048c2e5cff646b701139fb4b5fa76e10261432284ede7fd139b75cd69bb124d55973ceb7eccaa996226a7be4cad9b68a
Amount of coins available stay in the same across Bitcoin network:
signet, testnet, mainet. From my understanding this is a leftover
from some potential multi-chain support.
We just removed the `bitcoin/src/network/` exclude from `rustfmt` config
file. Run the command `cargo +nightly fmt`.
No other changes than those introduced by `rustfmt`.
When we moved to edition 2018 the use of `extern` became unnecessary and
we moved to using `pub use` for re-exports. It was observed however that
`pub extern crate` is more readable.
Improve crate root re-exports by doing:
- Use `pub extern crate foo` to re-export foo.
- Fix docs attribute for optional dependency `bitcoinconsensus`.
- Re-order to how rustfmt would put them.
30888f74c5 Move psbt module to crate root module (Tobin C. Harding)
8a75ff450f Move read_to_end out of util module (Tobin C. Harding)
445b07c94c Move util::Error to error module (Tobin C. Harding)
Pull request description:
In an effort to flatten `util` move things out that can/should be put in submodules of the crate root module. For each, configure `rustfmt` to ignore the module. This pushes the `rustfmt` review nightmare down the road.
ACKs for top commit:
apoelstra:
ACK 30888f74c5
Kixunil:
ACK 30888f74c5
Tree-SHA512: 0d93d60bec822d1dc82d4d67c25854364b0863488e4b35c9a0828a843fc3792286c18abde40a8e9d6ec535cfc7f0f0d6495d35961ce43af3f2605c92aaa0815d
f55c4099d5 Format the merkle_tree module (Tobin C. Harding)
Pull request description:
Run `cargo +nightly fmt` and commit the changes to the `merkle_tree` module. No manual changes, only those introduced by `rustfmt`.
ACKs for top commit:
tcharding:
ACK f55c4099d5
apoelstra:
ACK f55c4099d5
Tree-SHA512: 2f47fc426988b4755408fea5f8c4c47ebe23b53850737a1640fa9477cfd83f0ada046aec734986a92a9effbaaee0f34764ef87d82deff1a594f7b73bf2e93e93
We now have an `error` module but the `util::Error`, which is a general
error, is not in it.
Make `Error` more ergonomic to use by doing:
- Move the `util::Error` to `crate::error::Error`
- Re-export it from the crate root since it is our most general error
- Re-export and deprecated it from `util`
613107298d Move merkleblock into merkle_tree (Tobin C. Harding)
c89d9c48ac Move merkle_tree.rs to merkle_tree/mod.rs (Tobin C. Harding)
Pull request description:
Re-done after review comments below. This is now PR 1 in the `merkle_tree::block` series :)
Move the `merkleblock` module into the `merkle_tree` module in a submodule called `block`. In order to do the minimum amount of changes in this patch DO NOT rename types to improved naming and reduce stutter.
Note:
- block module is private
- the three types are re-exported from `merkle_block`
- the `MerkleBlock` re-export from the crate root is left in place.
This patch purposefully does the minimum amount of changes because there a whole bunch of improvements to the old "merkleblock" module that are coming next in a separate PR.
ACKs for top commit:
Kixunil:
ACK 613107298d
apoelstra:
ACK 613107298d
Tree-SHA512: 7299f605a0408372301642ac6826f7532de187b43a6d934715fc0806379b04cfd1550610428b720cb89095659c25e0f4fc8d6c842a93eafc19c091bbfcd5f35e
Move the `merkleblock` module into the `merkle_tree` module in a
submodule called `block`. In order to do the minimum amount of changes
in this patch DO NOT rename types to improved naming and reduce stutter.
Note:
- block module is private
- the three types are re-exported from `merkle_block`
This patch purposefully does the minimum amount of changes because there
a whole bunch of improvements to the old "merkleblock" module that are
coming next.
In preparation for moving `MerkleBlock` into the `merkle_tree` module;
create a new directory for the module and move `merkle_tree.rs` to
`merkle_tree/mod.rs`.
d78a996bf6 Add `Witness::from_slice()` and depreciate `Witness:from_vec()` (Noah Lanson)
d5bdf5d225 Add non-generic `Witness::push_slice()` method (Noah Lanson)
Pull request description:
Cleanup PR to improve the `Witness` API by:
- Adding `Witness::from_slice()` and depreciating `Witness::from_vec()` methods (#1371).
- Making `Witness::push()` not generic and take in `&[u8]` instead of `AsRef<[u8]>` (#1372).
Note: `Witness::from_vec()` has been marked for depreciation from `0.30.0`. Let me know if this should be different.
ACKs for top commit:
tcharding:
ACK d78a996bf6
apoelstra:
ACK d78a996bf6
Tree-SHA512: 3a0b11b1ea77966a773cf7c9e9853822192897eac495fc0a23068bad3b0c46714fc839b20ceeb6e076aa10ea8ff0c023dfc418feff2f892cf11e8c057e5b0c7d
4e9ff972ad Improve checksum documentation (Tobin C. Harding)
0f01cb9f51 Use rustdoc summary (Tobin C. Harding)
6151d4c841 base58: Rename public functions (Tobin C. Harding)
a94af5c052 base58: Re-order code (Tobin C. Harding)
d362e6286a base58: Improve rustdocs (Tobin C. Harding)
a43234e7ab base58: Make SmallVec methods private (Tobin C. Harding)
27f2cba623 base58: Use alternate form to print hex (Tobin C. Harding)
f659a7aca3 base58: Remove key related errors (Tobin C. Harding)
Pull request description:
Do some clean up work to the `base58` module in preparation for splitting it out into its own crate.
- Patches 1-6: Basic clean up.
- Patch 7: Re-names the public API functions.
- Patch 8: Fixes rustdoc comment as suggested during review.
- Patch 9: Improves documentation on checksum, also as suggested during review.
ACKs for top commit:
apoelstra:
ACK 4e9ff972ad
sanket1729:
ACK 4e9ff972ad. Left some naming nits.
Tree-SHA512: 0fb1e5a964bd197fcb3ef5e9ecd6f8c6b35439af46528e8dbe654d9d10f7c8ed3ca1461593caf6efd0be1cd3a1c24fed1a176931114846a394b396bed6a2411d
49d7b0bfe1 Remove deprecated re-exports (Tobin C. Harding)
Pull request description:
Recently we added a bunch of deprecated re-exports while moving things out of the util module. Turns out while the code reads like it works, `deprecated` actually only works for functions, not types or modules etc.
Remove the non-functional deprecated lines and elect to _not_ re-export things we moved. Release 0.30 is going to break a lot of code but there is no real nice way to resolve that. We will need good release notes and a public apology probably :)
Fix import statements that still rely on `util::bip32` - these should have been fixed when we moved the `bip32` module.
ACKs for top commit:
Kixunil:
ACK 49d7b0bfe1
apoelstra:
ACK 49d7b0bfe1
Tree-SHA512: 2b6a6d2d001f6124585f692315c48654b4fd0f5047b9fcef92b25829a27c8a02b3d187c8d363e9304b3998b652ead1ad368a7bf68ea23c984d1973074df2af21
00c7b6e06f Witness: Fix nits from PR 1323 (junderw)
Pull request description:
Ref: #1323
This is just to quickly fix some of the smaller nits. Larger changes (deprecations, adding / refactoring of methods) should be in a separate PR.
ACKs for top commit:
Kixunil:
ACK 00c7b6e06f
tcharding:
ACK 00c7b6e06f
sanket1729:
ACK 00c7b6e06f
Tree-SHA512: 5f661187a7003060669d15d873e323c017c905a00b62eb56ca3afc2fc27084b245ad62dfcf6d2fd14eac361430be954e7636f6b9ff668aefaad0424789a2f826
Recently we added a bunch of deprecated re-exports while moving things
out of the util module. Turns out while the code reads like it works,
`deprecated` actually only works for functions, not types or modules
etc.
Remove the non-functional deprecated lines and elect to _not_ re-export
things we moved. Release 0.30 is going to break a lot of code but there
is no real nice way to resolve that. We will need good release notes and
a public apology probably :)
Fix import statements that still rely on `util::bip32` - these should
have been fixed when we moved the `bip32` module.
We would like to bring the `bitcoin_hashes` crate into the
`rust-bitcoin` repository.
Import `bitcoin_hashes` into `rust-bitocin/hashes`, doing so looses all
the commit history from the original crate but if we archive the
original repository then the history will be preserved. We maintain the
same version number obviously and in the changelog we note the change of
repository.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 54c16249e06cc6b7870c7fc07d90f489d82647c7
Includes making `embedded` and `fuzzing` per-crate i.e., move them into
`bitcoin` as hashes includes these also.
NOTE: Does _not_ enable fuzzing for `hashes` in CI.
Notes on CI:
Attempts to merge in the github actions from the hashes crate however reduces
coverage by not running hashes tests for beta toolchain. Some additional
work could be done to improve the CI to increase efficiency without
reducing coverage. Leaving for another day.
3c0d5aed73 Add get_tapscript to Witness (junderw)
4226d60205 Add Index<usize> and nth(index) to Witness (junderw)
Pull request description:
Ref: https://github.com/rust-bitcoin/rust-bitcoin/pull/672#issuecomment-980636502
[Add Index<usize> and nth(index) to Witness](4226d60205)
[4226d60](4226d60205)
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
---
[Add get_tapscript to Witness](a7501d9599)
[a7501d9](a7501d9599)
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
---
Edit: This is the previous PR body:
> In a taproot script payment with annex, quick access to the 3rd to last element (which is the actual script in this case) is convenient.
>
> This feels like kicking the can down the road again, but I think it's a nice to have method.
>
> RCasatta dr-orlovsky were discussing this issue. I would like to ask if they have any thoughts on the addition of this.
ACKs for top commit:
tcharding:
ACK 3c0d5aed73
apoelstra:
ACK 3c0d5aed73
Kixunil:
ACK 3c0d5aed73
Tree-SHA512: 0038eed6ad56786b8dd6d98db0d1846753b8b25de0bc1089cdc75d5850d0ccc66dde9a10be7fe09589ad7db118fd50ee9f7993695968df5c389457ccfcdaa761
Recently we added a workspace but left the CHANGELOG file at the
repository root, this is incorrect because the CHANGELOG is a per crate
thing since it is updated along with crate release.
248f9a3b4b Use capital letters for Bitcoin Core (Tobin C. Harding)
832169eb8d Add to/from_consensus methods to Version type (Tobin C. Harding)
24984f095f Make block::Version inner value private (Tobin C. Harding)
7e146ede96 Make types in block module more terse (Tobin C. Harding)
Pull request description:
After initial attempt and review this PR has been re-written.
- Patch 1: Make types in `block` more terse, this is preparatory clean up based on suggestion below.
- Patch 2: Make inner value of `Version` private to hide the i32/u32 discrepancy
This is a follow up to #1240
ACKs for top commit:
Kixunil:
ACK 248f9a3b4b
apoelstra:
ACK 248f9a3b4b
Tree-SHA512: ee031035288a2bcc246a9837a6028c254c51daf78a5cc2441b467ab7f183f1700a63911a2e78b84a20674ce0a83851a7c3bb7e46644a56fdd255685b9a0bf7f2
2157e69857 Document the `all` module (Tobin C. Harding)
Pull request description:
Improve documentation on the `all` module by doing:
- Document guarantee that `all` will only ever contain opcode constants
- Fix stale/incorrect code comment
Done as follow up to #1295
ACKs for top commit:
apoelstra:
ACK 2157e69857
Kixunil:
ACK 2157e69857
Tree-SHA512: 4df091bbdce7b9ba73caabd74b80f9e8c0a30fa2f9a20ed9b75542e71a204e5cd82698a74bebbd6f0beab55ecd807154d1b7d27a787cc9dede7abbd20a0a4ad5
The `Version` type uses a signed 32 bit integer inner type but we bit
twiddle as if it was a `u32`. We recently made the inner type private to
hide the data type because of this oddness.
Add methods `from_consensus` and `to_consensus` to facilitate any
possible thing users may want to do with a consensus version value.
The Bitcoin block version is a signed integer for historical reasons,
but we bit twiddle it like an unsigned integer and during consensus
encode/decode we cast the signed value to an unsigned value.
In order to hide this confusion, make the inner value private and add a
couple of constants for v1 and v2 block versions.
Currently the types in the block module have longer names than
necessary, "header" and "version" identifiers contain the word "block",
this is unnecessary because we can write `block::Header` instead of
`BlockHeader` when context is required. This allows us to use the naked
type `Header` inside the `block` module with no loss of clarity.
We are stuck with `BlockHash` because the type is defined along with all
the other hash types in `hash_types`, leave it as is for now but
re-export it from the `block` module to assist in putting types that are
used together in scope in the same place, making import statements more
ergonomic.
The `all` module enables usage of a wildcard import statement without
muddying the scope with any other types defined in `opcodes`, in other
words if one wants to use the `All` type `opcodes::All` is the most
clear way to use it, however usage of naked `OP_FOO` types is perfectly
clear.
Add documentation stating that we guarantee to never put anything else
in the `all` module so folks are confident using a wildcard import will
not bring any rubbish into scope.
Expected usage in downstream applications that need types in `opcodes`
as well as the opcodes:
```
use bitcoin::opcodes::all::*;
use bitcoin::opcodes;
```
Also, we do no implement `Ord` or `PartialOrd`, document this including
HTML tags hiding an example bug from Bitcoin Core that shows why not.
9a1623c1dc Re-export hashbrown when enabled (Tobin C. Harding)
Pull request description:
`hashbrown` used to be exported until commit 23ee0930c7 which removed the `pub extern crate` declaration.
Found thanks to afilini (#1342)!
ACKs for top commit:
apoelstra:
ACK 9a1623c1dc
Kixunil:
ACK 9a1623c1dc
Tree-SHA512: 0363781dc06211eec59246ce54582220674d5ab2bd6e62ad15eeb97b0df6435cdf23df8306131c176b6003dde31d1e376f52981f2e69e9d2529876d3ada727e0
b6f9e47dba Fix `no_std` when `bitcoinconsensus` is enabled (Martin Habovstiak)
Pull request description:
`default-features = false` was missing previously but blindly adding it would lead to subtle risk of breaking when a crate not needing `std` depends on `bitcoinconsensus` and simultaneously another crate not needing `bitcoinconsensus` depends on `std` and another crate depends on them both.
This change fixes it by introducing `bitcoinconsensus-std` feature flag and provides a fallback if the flag is off. Unfortunately the fallback has to use a bit of reasonable `unsafe` due to limitations of upcasting.
The only safe alternatives are not do it and provide worse experience for crates that are affected by the problem above or break the API, which couldn't be backported and would be more annoying to use.
Closes#1343
This is considered PoC PR as I realized the possibility of the hack (and necessity of `unsafe`) at the last moment. Things like tests and modifying CONTRIBUTING to change the stance on `unsafe` will be added if `unsafe` is ACKed.
ACKs for top commit:
tcharding:
tACK b6f9e47dba
apoelstra:
ACK b6f9e47dba
Tree-SHA512: 3a2845f4701c94ff6214749fa490aecf3fd96089df31b15f9d3e0afe3c74329ff2b9054d51244358a79f928aa9d4cf4001fc3ec40a9b0e189323544c4480c709
b84e1d46c0 Move amount module out of util (Tobin C. Harding)
Pull request description:
Done as part of flattening the `util` module. Simply move the `amount` module out of the `util` module and to the crate root. Justified by the fact that the `Amount` type is more-or-less a "primitive" bitcoin type.
ACKs for top commit:
apoelstra:
ACK b84e1d46c0
Kixunil:
ACK b84e1d46c0
sanket1729:
ACK b84e1d46c0
Tree-SHA512: 9ec707f49b7ab29f573be22b366d2ea9c1a8e4b27e80350d521b9c6607fca4142f079648cb739ba8590edd97c21a00029c3647c4c8cebe47cc2dfee1b10b8b39
29df410ea3 Document state after call to calculate_root_inline (Tobin C. Harding)
2dbc7fdf21 Rename merkle_root functions (Tobin C. Harding)
22dd904735 Rename util::hash module (Tobin C. Harding)
Pull request description:
Done as part of flattening `util`.
The `util::hash` module only provides two functions, both to calculate the merkle root of a list of hashes.
1. Rename `util::hash` -> `crate::merkle_root`
2. Change function names to `calculate[_inline]` so usage becomes `merkle_root::calculate`
Done as two separate patches so we can bikeshed the names, can squash if needed.
ACKs for top commit:
Kixunil:
ACK 29df410ea3
apoelstra:
ACK 29df410ea3
Tree-SHA512: 17ace90c7700b5d7adf8b95731c9a348b5c92863806cc88bc40730547f457e44160efb19985e025970b59fea86d68f0bf4be0af17717a65ae44f11c8d10ec4c6
1050fe9cae Remove unnecessary borrow (Tobin C. Harding)
3966709336 Use is_none() (Tobin C. Harding)
d192052519 Remove unnecessary dereference (Tobin C. Harding)
624cda07b3 Remove unnecessary casts (Tobin C. Harding)
Pull request description:
Clippy has been updated and new warnings are being triggered in our codebase. This PR does all warnings using nightly since they all looked like reasonable things to fix.
Needed for CI to pass in other open PRs.
ACKs for top commit:
Kixunil:
ACK 1050fe9cae
sanket1729:
ACK 1050fe9cae.
Tree-SHA512: 7dcfb6a72a0aae51b49b417bb94cbe1becb1095d1bf0011921b1834a10f792cfcdeee37993ab9b103bd2dfcc9cd3c26cd7f1bb80b06b0d1aa4aaa454bfb0b3f0
This new method will check the last witness element to see if it starts with 0x50, and
depending on the result it will return the second to last or third to last witness
element according to BIP341.
In its current state, Witness can not know what type of script it is fulfilling,
so it is up to the caller to verify if the previous output is a taproot output or not.
Arbitrary indexing into Witness fixes the API of last and second_to_last to be more flexible.
This patch started off as an addition of third_to_last, but ended up evolving
into arbitrary indexing to allow for future use cases.
A list of the indices of the start byte for each witness element is stored as an ordered
contiguous group of u32s represented as 4 bytes each in the Vec<u8> contents.
The bytes are stored using to_ne_bytes for performance reasons. A helper function is added
to the tests to allow for easier contruction of the contents Vec in test vectors. u32 was
chosen because 22 bits are needed to store 4,000,000 which is the maximum weight limit for
a block. This might need to be reworked in the event of consensus limits increasing, but
u32 can hold 1000x the current limit, so it should be fine for the forseeable future.
The push and consensus_deserialize functions utilize rotate_left and rotate_right to move
the indices to the end of the new allocation. Depending on the size of the data, this
might be more of a performance hit than just allocating a new temporary Vec to store the
indices and append them after parsing is completed. However, for a majority of cases
rotating the indices should be faster. Suggestions to use VecDeque instead of Vec for
contents necessitate other considerations, since it is not a public facing change,
those optimizations can be dealt with in future patches.
The Index<usize> trait is implemented using the new nth method with expect.
The Iter struct is reworked to make use of the new data representation. This new data
structure makes it trivial to implement DoubleEndedIterator and other such traits, but
I have decided to leave this as out of scope for this patch.
This transaction broke past versions of `rust-bitcoin` and LND so this
adds a test to avoid reintroducing the problem in the future.
See also https://github.com/romanz/electrs/issues/783
In order that we can safely change/maintain de/serialization code we
need to have regression tests with hard coded serializations for each
type that implements serde.
It is enough to test a single serde data format, use JSON for `opcodes`
and bincode for other types.
Do regression testing in a newly added `tests` module.
The `base58` module is for encoding and decoding, it makes sense for the
public functions to be called `encode` and `decode`. We also have some
functions that operate on data with a checksum, for these it makes sense
to tack `check` onto the _end_ of the function name.
With this applied the public API is:
- decode
- decode_check
- encode
- encode_check
- encode_check_to_fmt
Code is arguably easier to read if the most important stuff comes first.
In the old days, when writing C, we had to put definitions before they
were used but in Rust this is not the case
Re-order the `base58` file so that the public API functions are up the top
then other helper functions are defined _after_ they are called.
Refactor only, no logic changes.
Currently we are manually adding `0x` in calls to `write!`, this is
unnecessary since the alternate form already adds the `0x`.
Was verified with
```
#[test]
fn bad_checksum_error_hex_format() {
let want = "invalid base58 character 0xab";
let got = format!("{}", Error::BadByte(0xAB));
assert_eq!(got, want)
}
```
Use alternate form to print hex.
The key related errors are incorrect because they are circular, we have
a base58 error variant in `key::Error` and two key error variants in
`base58::Error`.
Remove the key errors from the `base58::Error` type.
The function call `calculate_root_inline` calculates the merkle root
using the input array as a scratch buffer, i.e., we trash the data
during recursive calls to `merkle_root_r`.
Add explicit documentation to the function so its super clear not to use
the hashes again after calling this function.
Recently we renamed the `hash` module to `merkle_root`, this makes the
public functions provided stutter if used with one layer of path as is
Rust convention:
`merkle_root::bitcoin_merkle_root`
We can improve on this by renaming the functions to 'calculate', then we
get
- `merkle_root::calculate()`
- `merkle_root::calculate_inline()`
The `util::hash` module provides two functions for computing a merkle
root from a list/iterator of hashes.
Rename the module to `merkle_root` and move it to the crate root,
deprecate the original functions.
Done as part of flattening the `util` module.
d2ed0fe022 Add `impl IntoIterator for &'_ Witness` (Martin Habovstiak)
Pull request description:
It is considered idiomatic for types that have `iter()` method to also implement `IntoIterator` for their references. `Witness` was missing this so it is added here.
ACKs for top commit:
apoelstra:
ACK d2ed0fe022
tcharding:
ACK d2ed0fe022
Tree-SHA512: fc891109696de4f349324d6ddc160249ef22510622d9ce72a65b18f085d86b0de0f3ecb4f7060e1eaf716a908029865cd21cda5a6598fc4c16d0540152d9a4c9
2674327c93 Remove the endian module (Tobin C. Harding)
Pull request description:
Now we have MSRV of 1.41.1 we can use the `from_le_bytes` and `to_be_bytes` methods implemented on standard integer types, these became available in Rust 1.32.
Remove the `endian` module replacing its logic with calls to methods on the respective stdlib integer types.
ACKs for top commit:
Kixunil:
ACK 2674327c93
apoelstra:
ACK 2674327c93
Tree-SHA512: 7cdaf278c9d162cb0080bb6b9ea80ab55f881bfcd389f8b968f8cfaeebb0d27d3b5b46e9677a376bc6b7d4068cf094f50560ed4ae7bc817c50da688f70a7af25
It is considered idiomatic for types that have `iter()` method to also
implement `IntoIterator` for their references. `Witness` was missing
this so it is added here.
Now we have MSRV of 1.41.1 we can use the `from_le_bytes` and
`to_be_bytes` methods, these became available in Rust 1.32.
Remove the `endian` module replacing its logic with calls to methods on
the respective stdlib integer types.
Andrew Poelstra
Martin Habovstiak
sanket1729
Tobin C. Harding
connormullett
yancy
Antoine Riard
Noah Lanson
Casey Rodarmor
junderw
DanGould