We have a few instances of strange indentation:
- Incorrect number of characters
- Usage of neither "Block" style or "View" style (elect to use "Block")
Do various whitespace refactorings, of note:
- Use space around equals e.g., 'since = "blah"'
- Put return/break/continue on separate line
Whitespace only, no logic changes.
When signing a transaction will result in the sighash single bug being
exploitable we should return the 'one array' (equivalent to 1 as a
uint256) as the signature hash.
Add a unit test to verify we return uint256 1 value when use of
SIGHASH_SINGLE is invalid.
When signing a transaction will result in the sighash single bug being
exploitable we should return the 1 array (equivalent to 1 as a uint256)
as the signature hash.
Currently we are using the correct array value but are re-hashing it,
instead we should directly return it.
7554d76dfe Make Script::witness_version public (Dr Maxim Orlovsky)
Pull request description:
Originally this function was public (at least I was using it in downstream dependency in https://github.com/LNP-BP/descriptor-wallet). Now, in RC1, it became private. It is quite useful to detect witness scriptPubkeys.
ACKs for top commit:
apoelstra:
ACK 7554d76dfe
sanket1729:
utACK 7554d76dfe. I also found needing this rust-miniscript and had to some work-around.
Tree-SHA512: 27ae8fbbb5f19d7b3553fb05f193488c4096aa0e4949a5cdd96b9fda89f1983e45855598c4507131848e0ff2086a5b91b2201e9aed3ed8fcb66034a36715a434
7f33fe6a9b Delete contract hash module (Tobin Harding)
Pull request description:
This module has been deprecated in commit 1ffdce9 in August 2020, it is safe to delete it now.
Fixes: #322
ACKs for top commit:
apoelstra:
ACK 7f33fe6a9b
Kixunil:
ACK 7f33fe6a9b
dr-orlovsky:
ACK 7f33fe6a9b
Tree-SHA512: f218c8b0c09b14cd885cd7cf03c0a4623e5ead785decbc62a2f9610d438d5ea3efd2e2b47172a7608e33714996efa121707583d4257fa683dbfc9717988ceda6
e391ce9939 test: Add a test for incorrect message signature (Andrew Ahlers)
Pull request description:
In response to this comment: https://github.com/rust-bitcoin/rust-bitcoin/pull/819#discussion_r801477961
This should be straightforward. Let me know if there are any style issues. I tried to keep things similar to the existing test while cutting out any extra cruft to keep things small.
ACKs for top commit:
apoelstra:
ACK e391ce9939
Kixunil:
ACK e391ce9939
dr-orlovsky:
ACK e391ce9939
Tree-SHA512: 47296a7e0b2f45d5e50f507727ae4360686730a386f37dedfd1360b8cdf4b9dd3ce3bb5d05ea630177379ce4109059b6924fa362396b984ebab0ed1754318627
ac105903cd Flatten the policy module (Tobin Harding)
Pull request description:
The policy module contains a single `mod.rs` file, this is unnecessary, we can simply use `policy.rs` and flatten the module.
ACKs for top commit:
apoelstra:
utACK ac105903cd
dr-orlovsky:
ACK ac105903cd
Tree-SHA512: b0a9d2a68697a61fd85c1f4471c8df5fdcd7aa7052c33b4db385c311db96d3a6bcc80f17414ecae7f37f15fb0c8dc9f7ceaaf89cc1375f77fb2a5c489b948894
ee3b8c267d Order impl_hashencode lines (Tobin Harding)
Pull request description:
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.
ACKs for top commit:
apoelstra:
ACK ee3b8c267d
dr-orlovsky:
ACK ee3b8c267d
Tree-SHA512: 77f43fb65bdf0020c713b94bd8413c320e3acd6a39f28c1a89d8f0d29893f4559993fa864c490332ead262f03f05519a483d883af6b031889b5634fcf1e6cfe7
f4886afa66 Add full stops to docs (Tobin Harding)
f01f047b21 Remove unnecessary newlines (Tobin Harding)
8a1cc2ca77 Improve docs on ClassifyContext (Tobin Harding)
Pull request description:
Do some clean ups to the `blockdata::opcodes` module. Patch 3 is big but it should be quick to review because I made all the boring 'add full stops' changes in a single commit.
ACKs for top commit:
Kixunil:
ACK f4886afa66
apoelstra:
ACK f4886afa66
dr-orlovsky:
ACK f4886afa66
Tree-SHA512: b30f36bd06a028b6bbc24a64849c0788a9223760907bdcb3765af1742a228f630cc7666ed66fa2afd8fb6c96e3cf416e9bd9d2a3b6c72c6e47a16399a856fca1
0d36455d74 Build the docs with test.sh (Tobin Harding)
8163497ab3 Use correct indentation (Tobin Harding)
3786680cc7 Use correct script name (Tobin Harding)
Pull request description:
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.
- Patch 1: Fixes the incorrect file naming `ci.sh` -> `test.sh` in `CONTRIBUTING.md`.
- Patch 2 - 4: Do trivial cleanup of `test.sh`.
- Patch 5: Does the fix described above.
Resolves: #850
ACKs for top commit:
Kixunil:
ACK 0d36455d74
apoelstra:
ACK 0d36455d74
dr-orlovsky:
ACK 0d36455d74
Tree-SHA512: c33c8df687c2115477eae9888b80d4e744d7b68b598694cf17220dd11098f33ba23c0b33e6f7d291572187942c472d1bc9cbb5217d3d83d41906a97c0b3417e5
146d5e83d1 Improve docs for blockdata::block (Tobin Harding)
f03092c380 Fix erroneous function rustdoc (Tobin Harding)
5464848f45 Refactor check_witness_commitment (Tobin Harding)
Pull request description:
Do some clean ups to the `blockdata::block` module.
- Patch 1: Change predicate names (API breaking, could be seen as unnecessarily changing the API), can remove if NACK'd
- Patch 2: Refactor to assist code clarity
- Patch 3 and 4: are docs improvements, shouldn't be too controversial
ACKs for top commit:
apoelstra:
ACK 146d5e83d1
dr-orlovsky:
ACK 146d5e83d1
Tree-SHA512: 65cc414857c4569a389638b53eb99ed629bf67ae1d8ebdc9023e5974bb26902d4de41ec311bef3b5c895229d7d0df78d469a84c1e94fc0b7be7435338f0d510a
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.
Tobin Harding
sanket1729
Dr Maxim Orlovsky
Andrew Poelstra
Rishabh Singhal