Recently we (tcharding) do some mechanical improvements to the rustdocs
in the `blockdata` module without considering the content. On review a
bunch of improvements were suggested.
Improve the content of various rustdoc comments in the `blockdata`
module.
Suggested content came from reviewers, all mistakes are my own :)
Reduce the number of lines of code by using a longer column width, 100
as is more-or-less standard in this repo.
This patch only changes column width (line length), no other changes.
Manual parsing Json is tedious and error-prone. It contained a bunch of
`unwrap`s and was hard to read.
This replaces manual Json parsing with serde_derive implementation.
Closes#1231
1a2cf2681d Implement consensus encoding adapter for serde (Martin Habovstiak)
a6ecc58a5e Add `put_bytes_min` and `space_remaining` methods (Martin Habovstiak)
Pull request description:
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
Based on #1252
Required by #1234
ACKs for top commit:
tcharding:
ACK 1a2cf2681d
apoelstra:
ACK 1a2cf2681d
Tree-SHA512: 96e10cf6ea0e7dfecfb58ee97453e0e7c8a2cfbb8af1e73a23c3afb67b985b394976361ac237528991fbb7344cc9f24644869199008245a91838309aff34bb97
df90c50242 Add log2 to Work (Jiri Jakes)
Pull request description:
Adds method `Work::log2()` providing value equivalent to Bitcoin Core's `log2_work` in its logs. Fixes#1326.
Questions:
- The original issue (#1326) also suggests to add log2 to Target but it does not seem to be meaningful, does it? Bitcoin Core, to which the issue refers, also displays only log2 of work.
- Although work should not be 0, the type allows it. In this case, log2 would return -inf. I think we could leave it like that but if there are suggestions to deal with it in a different way, please let me know.
ACKs for top commit:
Kixunil:
ACK df90c50242
tcharding:
ACK df90c50242
apoelstra:
ACK df90c50242
Tree-SHA512: 24e83c849ea3e16cad6f1636920883967cdac90b1b98ab19e86d1a5d0ac4585079740b54c1315afecabc1943d29a8b94316e3586bd2d10f1b61cc5cf7ad5b273
e9dffb1b7b Change `max_money` to a constant. (Martin Habovstiak)
Pull request description:
The value is statically known which is better expressed as a constant. Also allows usage in const context.
ACKs for top commit:
apoelstra:
ACK e9dffb1b7b
tcharding:
ACK e9dffb1b7b
ariard:
ACK e9dffb1
sanket1729:
ACK e9dffb1b7b
Tree-SHA512: b9b80d573531fe75dce22e185a1c84b2885160334418d1cfbd7279684fd4229c3c6c4041d3a3badb3652c5723e90ff52d3c761cbc3bff7b73978776694a67422
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
Pnicking on oversized slice is useful to catch errors in code that's
supposed to know the exact sizes but this is undesirable in code that
doesn't. These two methods help with handling the case when `buf.len()`
is not known upfront.
5fc40baa73 Fix new clippy warnings (sanket1729)
3a923980e1 Hotfix: Fix broken serde (sanket1729)
Pull request description:
My local scripts did not test serde feature on the merge commit. While merging #734, I accidentally broke the serde feature on the latest master. This PR fixes it.
Sorry about that, I will update my local script to be the same as of CI check.
ACKs for top commit:
tcharding:
ACK 5fc40baa73
Kixunil:
ACK 5fc40baa73
Tree-SHA512: 99d348262239c53ab86ecf66bc64461958f181df0c102beddae486c813ab6d0c7a0b5af505fa5c7e60d5b59554e0bcf8547a71bf713020ddee1cc3d4b3a631f4
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.
dce88b09ff taproot: Run the formatter (Tobin C. Harding)
db5c8fe61c Move the taproot module to crate root (Tobin C. Harding)
Pull request description:
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.
CC sanket1729, I did this after reviewing the `taproot` module during work on https://github.com/rust-bitcoin/rust-bitcoin/pull/1260 and your review comment.
ACKs for top commit:
Kixunil:
ACK dce88b09ff
apoelstra:
ACK dce88b09ff
Tree-SHA512: 5498401a6ef92239568b8c338d5a531f5509043b82a52d2e221a37f6432cf38ca1145ce47adb526e9ed4c9d2fe9833011eec24e27353e64f68e2eded795ae9e4
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()
Clippy emits various warnings of form:
warning: this expression creates a reference which is immediately
dereferenced by the compiler
As suggested, remove the unnecessary explicit reference.
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
d4bfc3d7b1 github: add Kani to Github CI (Andrew Poelstra)
c11645b7e5 amount: use kani to check some arithmetic properties (Andrew Poelstra)
Pull request description:
A first exploration into using Kani. See #1370.
ACKs for top commit:
Kixunil:
ACK d4bfc3d7b1
tcharding:
ACK d4bfc3d7b1
Tree-SHA512: 431ba1c1c42521bc36ab5260ea338c5c8154edd7f037928ba36a6c901551d05e420e8a886f772ea205009f718a16a5793a2e408818de88602968b52492a910cd
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.
57165d3f7f Fix typo in the SHA512 documentation (Calvin Kim)
Pull request description:
Saw the typo and since this typo also exists in crates.io, I thought it'd be good to quickly change it.
Though maybe this should be replaced altogether with the `hash_type` macro since every other hash function implementation is using that.
ACKs for top commit:
Kixunil:
ACK 57165d3f7f
apoelstra:
ACK 57165d3f7f
Tree-SHA512: dba54a71073bf58cafe4a248ec25c5f6a691f565d6bda749d5b50786d353a30cd59f09e10f600cb95b8f635ab1ce38aef8decb178b586c80df1b6cc9a7943a49
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
Tobin C. Harding
Martin Habovstiak
Andrew Poelstra
Jiri Jakes
sanket1729
connormullett
Calvin Kim
yancy