e503f14331 Improve docs: blockdata::transaction (Tobin Harding)
f02b3a8472 Add code comment for emtpy input (Tobin Harding)
6a0ec1ac47 Remove redundant _eq (Tobin Harding)
3bcc146a44 Improve docs: encode_signing_data_to/signature_hash (Tobin Harding)
Pull request description:
Do some cleanups to the docs in `blockdata::transaction`. Patch 1 needs the most careful review please. The rest should not be too controversial.
ACKs for top commit:
apoelstra:
ACK e503f14331
dr-orlovsky:
ACK e503f14331
Tree-SHA512: 3953226e1b7f0db0371b1902888407a48531688bf8ed08539a0090f369b491b130d70b2fae859878ef178a397cefe0ee2a15f3358afc990a2776194cc2b3882b
4dcbef6ddd Improve docs: script module (Tobin Harding)
Pull request description:
Improve the docs in the `blockdata::script` module by doing:
- Use full sentences (use capitals and full stops)
- Improve grammar/wording if necessary
- Remove incorrect/unneeded comments
- Fix layout of rustdoc i.e., use brief and description sections
- Use 100 line character width if it makes the comment look better
- Use third person instead of imperative tense
## Note to reviewers
Sorry to be a bore and request review on all these docs fixes, this one is all in a single patch which makes it a bit harder to review. It is very similar in content to all the others that are open right now so I'm going to be a bit rude and leave it like this. Please say if this is even slightly putting too much demand on you review time.
ACKs for top commit:
apoelstra:
ACK 4dcbef6ddd
dr-orlovsky:
ACK 4dcbef6ddd
Tree-SHA512: 49fa1d88c4b97decbc563747ba166fe95698da6a634801ccf5f99fd67a4a907067dbf0a4d64e7773d5d5b04aef404167b6cc911382363247d15a61cef5d8965c
d68531d815 Update secp256k1 dependency (Tobin Harding)
Pull request description:
Update our `rust-secp256k1` dependency to the latest released version.
Requires doing:
- Add a new variant to `Error` for the case where parity of the internal key is an invalid value (not 0 or 1).
- Use non-deprecated const
Please check the error change carefully, this error does relate _only_ to the parity of an internal key, right?
ACKs for top commit:
apoelstra:
ACK d68531d815
dr-orlovsky:
ACK d68531d815
Tree-SHA512: 2552b07c0ccc065ced412caadaa0e9d8d77b5f2ce3698b7f53367a9f183557172526c154594c1c706e229da1bab67d11d88255cfd1fe3aac3e16888fe2948aae
Update our `rust-secp256k1` dependency to the latest version.
Requires doing:
- Add a new variant to `Error` for the case where parity of the internal
key is an invalid value (not 0 or 1).
- Use non-deprecated const
aaf587d320 Use correct opcode count (Tobin Harding)
Pull request description:
Code comment contains an off-by-one error, update it to the correct value '61'.
Fixes: #866
ACKs for top commit:
apoelstra:
utACK aaf587d320
Kixunil:
ACK aaf587d320
Tree-SHA512: 0306f1bbad904c1bfb26ce69758114dd94ee748c8733094fe94b1e1072be84a823a906ecc2046c30aa23c04e762199418bfeab3b63f3dc0c25e2c582813edbb4
Improve the docs in the `blockdata::script` module by doing:
- Use full sentences (use capitals and full stops)
- Improve grammar/wording if necessary
- Remove incorrect/unneeded comments
- Fix layout of rustdoc i.e., use brief and description sections
- Use 100 line character width if it makes the comment look better
- Use third person instead of imperative tense
Improve the rustdocs for the `blockdata::transaction` module:
- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording/grammar
- Use backticks in links
- Use 100 character column width if it improves readability
Nothing too controversial here :)
The line of code `let mut have_witness = self.input.is_empty();` is
puzzling if one does not know _why_ we serialize in BIP141 style when
there are no inputs.
Add a code comment to save devs spending time trying to work out _why_
this is correct.
The two methods `encode_signing_data_to` and `signature_hash` use the
same docs (one is a public helper for the other). The docs have gotten a
bit stale (refer to deprecated types).
Instead of duplicating all the text, add a statement pointing readers
from the docs of `signature_hash` to the docs on
`encode_signing_data_to`.
Improve the rustdocs for the `blockdata::block` module:
- Use full sentences (capitalisation and full stop)
- Use third person tense instead of imperative
- Improve wording if needed
Currently function contains nested `if` clauses that arguably obfuscate
the code. We can make the code easier to read by pulling out the error
paths and returning them higher up in the function.
Refactor only, no logic changes.
7638d59fa6 Improve rusntdocs for *_hash_ty methods (Tobin Harding)
Pull request description:
Recently we added two new methods with rustdocs. The rustdocs have a few minor things that can be improved.
- Use 'Returns' section for both success and error path returns
- Use markdown heading for section
- [Subjectively] improve wording of docs
@rish-singhal this PR is minor rustdocs stuff that I have recently been learning myself. I hope this makes your future patches easier to do in regards to the docs. FTR there are `# Returns` sections and `# Examples` sections that one can use, no set rules as far as I can tell. I chose `# Returns` in this case because it seemed to work. Please feel free to NACK the wording changes if you do not agree.
ACKs for top commit:
apoelstra:
ACK 7638d59fa6
Kixunil:
ACK 7638d59fa6
Tree-SHA512: ac1629647a39789e9a162b54440798507c74501ef0a5014bc6dfe2cf5779588a304c059433b8387bbe06f413f0712d1204e4b7d912c79e31f9485f6ddcf9ceba
Put the calls to `impl_hashencode` in the same order, and with the same
whitespace, as the calls to `hash_newtype`. This makes groking the file
easier because its quick to glance down the types and see which ones
implement hashencode (consensus_encode/decode) and which ones do not.
We currently build the docs as a separate CI job, we can however just do
it as part of the `Tests` job using the nightly toolchain.
Conditionally build the docs based on a `DO_DOCS` env var.
Note, uses `--cfg docsrs` so can only be built run with nightly toolchain.
91c5d7192f Change the parameter for control block verification (sanket1729)
Pull request description:
- Changes the API from TweakedPublicKey to XonlyPublicKey. I believe we
introduced TweakedPublicKey to guard against creating address API. This
is confusing because when we want to verify control block we have to
call dangerous_assume_tweak.
- This is in true in most cases that the key would be tweaked, but we only
want to guard in while creating a new address. If we want to verify
blocks, we should deal with native X-only-keys regardless of how they
were created
- Also removes the & from a 32 Copy byte as discussed elsewhere.
ACKs for top commit:
Kixunil:
ACK 91c5d7192f
apoelstra:
ACK 91c5d7192f
Tree-SHA512: d7da403435afbd1c1650b6e62055b1b0e6811d6ec30fff198315523035a56b493d510e8a560b08552684417886687c8a8daa57b5eef4f3699dfff7e2ee6a7447
Changes the API from TweakedPublicKey to XonlyPublicKey. I believe we
introduced TweakedPublicKey to guard against creating address API. This
is confusing because when we want to verify control block we have to
call dangerous_assume_tweak.
This is in true in most cases that the key would be tweaked, but we only
want to guard in while creating a new address. If we want to verify
blocks, we should deal with native X-only-keys regardless of how they
were created
5e2449922d Separate merge logic out of Map trait (Tobin Harding)
Pull request description:
Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for merging together two PSBTs. Doing so broke the
`psbt` module.
Add a public trait `Merge` and implement it for
`PartiallySignedTransaction` using the code currently in the `merge`
method of the now private `Map` trait.
Motivated by https://github.com/rust-bitcoin/rust-bitcoin/pull/841
ACKs for top commit:
JeremyRubin:
> ACK 5e24499
apoelstra:
ACK 5e2449922d
sanket1729:
ACK 5e2449922d. Also verified that the vectors are same of that of BIP174
Tree-SHA512: 79eefe93e870b61231b388aa28a95ee5c8ac06b68910f4ff324569512a79eafe5b86239fd45f54ca7a868cf59dc6301e45d1f046c039a64b2493a8ffcea659fd
fb04cabe1d Add a method to psbt to compute find sighash type (Rishabh Singhal)
Pull request description:
Fixes#838: Add a utility method to psbt to compute find sighash
type of a given input.
For now, I have changed my previous implementation as discussed in #838 to functional style code as suggested by @Kixunil.
ACKs for top commit:
apoelstra:
ACK fb04cabe1d
Kixunil:
ACK fb04cabe1d
Tree-SHA512: 86184649e7a309348cb217347b82bf39c9997ae259fe7881322038a88bd04deab927bede1dd71d17496bac420353a3fd07e7d191ff4671a07754c02a38dd1319
8016a858f9 Tempararily pin fuzzer on rust 1.58 (sanket1729)
Pull request description:
I think this might take a while to resolve and we should move ahead with
1.58. Looks like the fresh release of 1.59 added LLVM 13.0 that broke
some things.
ACKs for top commit:
Kixunil:
ACK 8016a858f9
apoelstra:
ACK 8016a858f9
Tree-SHA512: 171cf52cdb5144e5e4a5bbbe179ad93d441c9e1c98394dce786e5e87e2020d88b81b133730a2e9a040e3520205e1456713d0a0b6473928a26254c67369b2841f
I think this might take a while to resolve and we should move ahead with
1.58. Looks like the fresh release of 1.59 added LLVM 13.0 that broke
some things.
c0d36efb8b Don't allow uncompressed public keys without prefix 0x04 (Noah Lanson)
Pull request description:
Was following #520 and through it was a quick fix that I could do:
#### Changes:
- If an uncompressed public key doesn't have prefix 0x04 in `PublicKey::from_slice()`, an error is returned.
<br>
I was wondering if `PublicKey::from_str()` should also enforce the same rules, however I have not incuded this in the PR.
Please let me know if any changes need to be made.
Thanks
ACKs for top commit:
Kixunil:
ACK c0d36efb8b
apoelstra:
ACK c0d36efb8b
sanket1729:
utACK c0d36efb8b. Not thrilled about the error message expecting len 66, when it can be both 66/130. But can live with it
Tree-SHA512: cfbcd569691c9a7f69ee775ec530605f42e988470a2ff9c28b4c881cec6b259053bb2288818e00b6f6b20316b1fb30fecc0b9a240ebbe7618f202ef6b5efeb9b
Recently we (*cough* Tobin) made the `Map` trait private and neglected
to add a public API for combining together two PSBTs. Doing so broke the
`psbt` module.
Pull the merge logic out of the `Map` trait and put it in methods on
each individual type (`Input`, `Output`, `PartiallySignedTransaction`).
Doing so allows for simplification of return types since combining
inputs/outputs never errors.
Use the term 'combine' instead of 'merge' since that is the term used in
BIP 174.
006193f5b6 feat: Support running CI locally with `act` (Andrew Ahlers)
Pull request description:
Disable problematic jobs that involve Github Actions caching or `cross` whenever the environment is set to ACT. This allows running the CI pipeline locally and hopefully speeds up PR cycle times by reducing unexpected CI pipeline results.
## Motivation
The CI pipeline does not run until maintainer approval. This allows an easy path for contributors to test out the CI pipeline locally and avoid unexpected results. I personally kept hitting issues here due to MSRV always throwing me off
## Potential issues
- `act` does not support Github Actions caching feature which is used in the `fuzz` workflow so I simply disabled it if `act` is detected.
- the `cross` workflow is similarly disabled. I kept hitting `sh: 1: cargo: not found` (see: https://github.com/cross-rs/cross/issues/260). I tried a few different workaround but had no success.
I'm hoping this is acceptable as it still improves the local testing situation and covers the `Tests` workflow
ACKs for top commit:
apoelstra:
ACK 006193f5b6
Kixunil:
ACK 006193f5b6
Tree-SHA512: 079d50276161ef2c7d61e41d8f47126b8def41ea61f4560be03950e6e4e07ae8bb67ab22ac8f0e46e5e5e3406b64c1de4bb2c62526c36b82db962842ffc377ca
Disable problematic jobs that involve Github Actions caching or `cross`
whenever the environment is set to ACT. This allows running the CI
pipeline locally and hopefully speeds up PR cycle times by reducing
unexpected CI pipeline results.
10fedfb3b4 Change Prevouts::All(&[TxOut]) to Prevouts::All(&[Borrow<T>]) (sanket1729)
Pull request description:
I believe this avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods.
See #834
ACKs for top commit:
apoelstra:
ACK 10fedfb3b4
Kixunil:
ACK 10fedfb3b4
Tree-SHA512: 20f69c626b38d6b3c03c8cb370cfad097bbf0bfefff9bb2379c8af3bc94e25d8cc45fc5d69488aeefad58a95470e8f30eb7b400349992a9ebd0d3a13870cba43
4e19973d4e Add a breaking test (sanket1729)
69c6eb6173 Bug: Change type of pbst partial sig from secp key to bitcoin key (sanket1729)
Pull request description:
This changes the type of secp signature from secp256k1::PublicKey to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
ACKs for top commit:
Kixunil:
ACK 4e19973d4e
apoelstra:
ACK 4e19973d4e
Tree-SHA512: 2a326bafc050cd101e75899c32224a9ac2fcb2ec0b9f7f173404a46f2b3a92ecb78d0002db252a5af06705566bdc10102d20f4718eaeeebd3730fdb5ee89ff5a
This avoids some allocation of creating a vec of TxOut to
create a slice incase the data is already available in psbt/other
methods. Facilitates creation of Prevouts from &[TxOut] as well as
&[&TxOut]
This changes the type of secp signature from secp256k1::Signature to
bitcoin::PublicKey. Psbt allows storing signatures for both compressed
as well as uncompressed keys. This bug was introduced in #591 while
trying to change the type of BIP32 keys from bitcoin::PublicKey to
secp256k1::PublicKey.
79cee4cd31 fix: Error on unsuported addresses in `is_signed_by_address` (Andrew Ahlers)
Pull request description:
This is a direct response to this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/684#issuecomment-1012136845
Currently unsupported addresses simply return `false` which is highly misleading as it is entirely possible that the keys related to a given address were used to sign the message.
If this is successful I can follow up with
> a method to check if a PublicKey is associated with an address
ACKs for top commit:
apoelstra:
ACK 79cee4cd31
Kixunil:
ACK 79cee4cd31
Tree-SHA512: 0c2f15696c63272e8ad1ecc0959c828f2d6c4aa16a7d099235c3f6bd287a0c20e034752331644c4bcc3af61ba4d739ad6795cbcea61c9e615c1eb7b43ebf0eeb
df7bb03a67 Simplify read_scriptbool (Tobin Harding)
4b6e86658d Refactor is_provably_unspendable (Tobin Harding)
e54a2d653b Put && operator at front of line (Tobin Harding)
f5512c4931 Refactor is_p2pkh (Tobin Harding)
373ea89a9a Simplify read_scriptbool (Tobin Harding)
654b2772b8 Add passing unit tests for read_scriptbool (Tobin Harding)
Pull request description:
In an effort to make the code clearer and more explicit, do various refactorings around logical operators. Each done as a separate patch to ease review and limit scope of discussion.
Based on review of https://github.com/rust-bitcoin/rust-bitcoin/pull/806
ACKs for top commit:
Kixunil:
ACK df7bb03a67
apoelstra:
ACK df7bb03a67
Tree-SHA512: 06460979d492eb38cefc147397338b7fd95320c66ce8e8b4f8e2b454bb35721ce308413690a0618bd19d695df56175646d4d0c619388c0268f7fd35d5a7b6a3d
Simplify `read_scriptbool` by doing:
- Use `split_last` to get at the last element
- Mask the last byte against ^0x80 instead of using two equality
statements
22aeaef52b Use write_all instead of write (Riccardo Casatta)
Pull request description:
write() could write only a part of the given buffer, the caller should
check the numbers of byte written (which is what write_all does)
ACKs for top commit:
apoelstra:
ACK 22aeaef52b
Kixunil:
ACK 22aeaef52b
dr-orlovsky:
utACK 22aeaef52b
Tree-SHA512: e4bf3c757e1d369f9bb737e970b93ec29a487419eb478b41c36da033eafea3f0a96faa1e5c6f9397febba309ce4330b29a9e5369bb547645e5f72ba935d2cafe
Dr. Maxim Orlovsky
Tobin Harding
Andrew Poelstra
Andrew Ahlers
sanket1729
Rishabh Singhal
Noah Lanson
Riccardo Casatta