Import bitcoin hashes #1284

tcharding · 2022-09-16T02:35:36Z

#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. #550 (comment)

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.

tcharding · 2022-09-16T02:54:43Z

Fuzzing isn't being run on bitcoin_hashes ATM, here we enable it and it seems its hitting a crash

 Crash: saved as 'hfuzz_workspace/ripemd160/SIGABRT.PC.7ffff7dba00b.STACK.c94ef10c8.CODE.-6.ADDR.0.INSTR.mov____0x108(%rsp),%rax.fuzz'
[2022-09-16T02:43:05+0000][W][5481] arch_checkWait():237 Persistent mode: pid=5483 exited with status: SIGNALED, signal: 6 (Aborted)
Seen a crash. Terminating all fuzzing threads

I do not know what to do about that?

apoelstra · 2022-09-16T18:04:32Z

This is pretty frustrating. The fuzztests are basically completely broken in bitcoin_hashes because the dummy hash logic does not round trip.

cc @TheBlueMatt I am going to change the cfg(fuzzing) logic to require a different, rust-bitcoin-specific config flag that you will need to enable in rust-lightning.

@tcharding I think we should move all the fuzz targets into crate-specific directories because it's confusing to see them all in a flat directory structure like this.

TheBlueMatt · 2022-09-17T06:14:53Z

I mean at a high level that's fine, but I'm a bit confused why they don't "round-trip"? What is round-trip in this context anyway?

apoelstra · 2022-09-17T13:27:30Z

@TheBlueMatt oh, this isn't about round-tripping, it's about getting the same result with the hash function as you do by running through a hash engine.

tcharding · 2022-09-18T23:34:06Z

Changes in force push:

Moved fuzz -> bitcoin/fuzz
Imported bitocin_hashes/fuzz into hashes/fuzz - DID NOT ENABLE IN CI

tcharding · 2022-09-18T23:35:28Z

I'd like to squash the last three patches before merge. Will wait for an ack and a green CI run before doing so.

apoelstra

ACK 874db5d

tcharding · 2022-09-19T23:13:48Z

Changes in force push:

Squashed last three commits into one
Added commit hash from tip of bitcoin_hashes:master to the changelog file
Removed stale exclude = ["embedded"] from workspace manifest
Remove duplicate newline at end of .gitignore file

apoelstra

ACK 4b48da0

tcharding · 2022-09-22T22:26:46Z

Rebase only, no other changes.

apoelstra

ACK 096d7a9

tcharding · 2022-10-18T20:04:44Z

Not sure right now what to do about the new changes merged into bitcoin_hashes since this PR was first done. I think I'll re-do this PR so its up to date with master branch on bitcoin_hashes - I don't really want to be doing this every day though so I'll leave it for a day or two to see if I have a better idea.

tcharding · 2022-10-20T21:53:33Z

I've indicated high priority even though this is a draft, just needs co-ordination between @apoelstra and myself regarding updating this PR and possibly freezing bitcoin_hashes. There are three new commits on the bitcoin_hashes repo that need adding to this PR before it can be reviewed but the concept is reviewable. Thanks.

apoelstra · 2022-10-21T14:03:35Z

I merged the last bitcoin_hashes PR that you asked me. There are now two open PRs there that we can re-open here (one is by you, the other is Jeremy's schemars PR that has no clear path to being merged).

I guess on bitcoin_hashes we should open one last PR (that we won't bring over here) that basically updates the README to say that the repo has been frozen while it's being migrated to rust-bitcoin, and to watch this PR. Then once this is merged we can add another which says that the repo is deprecated in favor of this one.

concept ACK.

apoelstra

ACK dbc7c6d

Kixunil · 2022-10-24T22:47:03Z

Oh, I just realized: won't this slow down the development by running all tests for every change?

tcharding · 2022-10-24T23:56:09Z

CI is already slow as hell IMO, we should definitely look into some ways to speed it up. I don't think this PR makes it any worse than writing more tests makes it worse, or am I missing something?

Kixunil · 2022-10-25T08:53:43Z

It makes it slow in the same way writing tests that are only relevant sometimes make things slow.

If we're going to have a single repository maybe we should somehow detect which crates changed and only run tests for those and maybe also crates that depend on them (but we would have to use path dependencies and update downstream crates which is probably actually a good thing).

Oh and of course this is complicated so I wouldn't block PRs on that.

apoelstra · 2022-10-25T18:53:11Z

Agreed on all counts @Kixunil.

It shouldn't be too hard to detect which crates have changed and to only run relevant tests. Though we should at least compile-test everything to check that we haven't broken interdependencies.

tcharding · 2022-10-25T20:34:19Z

Aight, this seems like a thing to go near the top of my todo list as we progress with crate smashing.

tcharding · 2022-10-25T20:35:59Z

With "improve CI to only run tests for code that effected by this PR" on my todo list does this PR get an ACK from you @Kixunil?

Recently we introduced a workspace but did not update the git ignore file. Ignore lock files, build artifacts, test artifacts, and fuzz artifacts.

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.

The changelog file is a per crate, per release, thing. Add one to the `internals` crate.

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.

Recently clippy was updated and now new warnings are generated for the `hashes` crate. Clippy emits 3 warnings of form: warning: this expression borrows a value the compiler would automatically borrow As suggested, remove the explicit borrow.

tcharding · 2022-11-07T22:04:43Z

Changes in force push:

Re-worked the CI script as I had not done it correctly initially, could be improved still but I think it maintains reasonable coverage now. It may be inefficient, of not DO_FEATURE_MATRIX does not exist in bitcoin/contrib/test.sh so all features are tested all the time - may be a waste of resources.
Added final patch to fix clippy warnings in hashes introduced recently by the new Clippy version.
Fixed mistake where I added back in a deleted fuzz_target (uint256)

sanket1729

ACK 9674bf2. 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 9674bf2

apoelstra · 2022-11-13T00:41:58Z

@Kixunil could we have an ACK from you?

I'd like to get this merged fairly quickly since it'll require I change my local build scripts (and also we can merge this now without rebasing, unlike several other big open PRs)

apoelstra · 2022-11-14T14:17:25Z

Also, as @tcharding notes in rust-bitcoin/rust-secp256k1#509 we really ought to change the _ in the name ... this may be an opportunity to do so. But we can do that in a followup PR.

Kixunil · 2022-11-14T14:20:57Z

This a bit big that's why I didn't find the time yet. Will try later today.

apoelstra · 2022-11-14T14:28:38Z

Thank you! The majority of it is a literal copy/paste from the bitcoin_hashes repo which you can recreate and check that way.

tcharding · 2022-11-18T00:43:51Z

There are actually no merge conflicts with fuzz/fuzz_targets/deserialize_psbt.rs, not force pushing so as to minimize re-acks. Since we don't use the "merge" button this should be ok to merge without rebase, right? You use github-merge.py don't you @apoelstra? I took a quick re-read of it.

apoelstra · 2022-11-18T18:20:46Z

Yep, looks like it -- I'm just running my tests on the merge commit and then we should be good to go!

apoelstra · 2022-11-18T18:33:07Z

WARNING: merge differs from github!
Type 'ignore' to continue. ignore

Difference with github ignored.
[pull/1284/local-merge 8da07951e] Merge rust-bitcoin/rust-bitcoin#1284: Import bitcoin hashes
 Date: Fri Nov 18 18:20:26 2022 +0000
rust-bitcoin/rust-bitcoin#1284 Import bitcoin hashes into master
* 9674bf29fe996a1f4c73c8a78193674214f9733c hashes: Fix clippy warnings (Tobin C. Harding) (pr/1284/head, pull/1284/head)
* b9643bf3e9b21e4e2207ea210d1d57ffa014d271 Import bitcoin_hashes crate into hashes (Tobin C. Harding)
* 580feab3f9d8fac1a7dc315f3a7dc371bbb9a8d5 internals: Add CHANGELOG file (Tobin C. Harding)
* bae64e156ee21d7dd2fae12b77674d330419ed0d Move CHANGELOG to bitcoin crate (Tobin C. Harding)
* 9a2c856be6c90d14ab79cfa6f864e6eb75549f1a Update gitignore file (Tobin C. Harding)
ACKs:
* ACK 9674bf29fe996a1f4c73c8a78193674214f9733c. 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.  (sanket1729)
* ACK 9674bf29fe996a1f4c73c8a78193674214f9733c (apoelstra)
Type 's' to sign off on the above merge, or 'x' to reject and exit. s

Neat.

tcharding · 2022-11-18T19:14:07Z

Win!

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from 20522f6 to 172ba24 Compare September 16, 2022 02:40

tcharding force-pushed the 09-16-import-bitcoin-hashes branch 2 times, most recently from d09620d to 6029100 Compare September 16, 2022 03:04

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from 6029100 to 874db5d Compare September 18, 2022 23:32

apoelstra previously approved these changes Sep 19, 2022

View reviewed changes

tcharding dismissed apoelstra’s stale review via 4b48da0 September 19, 2022 23:12

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from 874db5d to 4b48da0 Compare September 19, 2022 23:12

apoelstra previously approved these changes Sep 20, 2022

View reviewed changes

apoelstra mentioned this pull request Sep 20, 2022

Proposal: split bitcoin into multi-crate workspace #550

Open

tcharding dismissed apoelstra’s stale review via 2188b40 September 22, 2022 22:13

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from 4b48da0 to 2188b40 Compare September 22, 2022 22:13

apoelstra mentioned this pull request Sep 24, 2022

Fix clippy warnings #1299

Merged

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from 2188b40 to 096d7a9 Compare September 27, 2022 18:23

apoelstra previously approved these changes Sep 28, 2022

View reviewed changes

tcharding mentioned this pull request Oct 20, 2022

Update error implementations for the new MSRV rust-bitcoin/bitcoin_hashes#155

Merged

tcharding marked this pull request as draft October 20, 2022 21:44

tcharding added the P-high High priority label Oct 20, 2022

tcharding mentioned this pull request Oct 21, 2022

Clean up after subcrate creation #1337

Closed

tcharding dismissed apoelstra’s stale review via 8e1fb5e October 21, 2022 20:55

apoelstra previously approved these changes Oct 22, 2022

View reviewed changes

tcharding added the release notes mention label Nov 7, 2022

tcharding added 5 commits November 8, 2022 08:40

Update gitignore file

9a2c856

Recently we introduced a workspace but did not update the git ignore file. Ignore lock files, build artifacts, test artifacts, and fuzz artifacts.

Move CHANGELOG to bitcoin crate

bae64e1

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.

internals: Add CHANGELOG file

580feab

The changelog file is a per crate, per release, thing. Add one to the `internals` crate.

hashes: Fix clippy warnings

9674bf2

Recently clippy was updated and now new warnings are generated for the `hashes` crate. Clippy emits 3 warnings of form: warning: this expression borrows a value the compiler would automatically borrow As suggested, remove the explicit borrow.

tcharding dismissed apoelstra’s stale review via 9674bf2 November 7, 2022 22:02

tcharding force-pushed the 09-16-import-bitcoin-hashes branch from dbc7c6d to 9674bf2 Compare November 7, 2022 22:02

This was referenced Nov 8, 2022

Remove Index trait bound from Hash rust-bitcoin/bitcoin_hashes#168

Closed

Remove schemars pinning rust-bitcoin/bitcoin_hashes#182

Closed

sanket1729 approved these changes Nov 12, 2022

View reviewed changes

apoelstra approved these changes Nov 13, 2022

View reviewed changes

apoelstra merged commit 349a8a5 into rust-bitcoin:master Nov 18, 2022

tcharding deleted the 09-16-import-bitcoin-hashes branch January 12, 2023 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import bitcoin hashes #1284

Import bitcoin hashes #1284

tcharding commented Sep 16, 2022 •

edited

Loading

tcharding commented Sep 16, 2022

apoelstra commented Sep 16, 2022

TheBlueMatt commented Sep 17, 2022

apoelstra commented Sep 17, 2022

tcharding commented Sep 18, 2022

tcharding commented Sep 18, 2022

apoelstra left a comment

tcharding commented Sep 19, 2022

apoelstra left a comment

tcharding commented Sep 22, 2022

apoelstra left a comment

tcharding commented Oct 18, 2022

tcharding commented Oct 20, 2022

apoelstra commented Oct 21, 2022

apoelstra left a comment

Kixunil commented Oct 24, 2022

tcharding commented Oct 24, 2022

Kixunil commented Oct 25, 2022 •

edited

Loading

apoelstra commented Oct 25, 2022

tcharding commented Oct 25, 2022

tcharding commented Oct 25, 2022

tcharding commented Nov 7, 2022 •

edited

Loading

sanket1729 left a comment

apoelstra left a comment

apoelstra commented Nov 13, 2022

apoelstra commented Nov 14, 2022

Kixunil commented Nov 14, 2022

apoelstra commented Nov 14, 2022

tcharding commented Nov 18, 2022

apoelstra commented Nov 18, 2022

apoelstra commented Nov 18, 2022

tcharding commented Nov 18, 2022

Import bitcoin hashes #1284

Import bitcoin hashes #1284

Conversation

tcharding commented Sep 16, 2022 • edited Loading

tcharding commented Sep 16, 2022

apoelstra commented Sep 16, 2022

TheBlueMatt commented Sep 17, 2022

apoelstra commented Sep 17, 2022

tcharding commented Sep 18, 2022

tcharding commented Sep 18, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Sep 19, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Sep 22, 2022

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Oct 18, 2022

tcharding commented Oct 20, 2022

apoelstra commented Oct 21, 2022

apoelstra left a comment

Choose a reason for hiding this comment

Kixunil commented Oct 24, 2022

tcharding commented Oct 24, 2022

Kixunil commented Oct 25, 2022 • edited Loading

apoelstra commented Oct 25, 2022

tcharding commented Oct 25, 2022

tcharding commented Oct 25, 2022

tcharding commented Nov 7, 2022 • edited Loading

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Nov 13, 2022

apoelstra commented Nov 14, 2022

Kixunil commented Nov 14, 2022

apoelstra commented Nov 14, 2022

tcharding commented Nov 18, 2022

apoelstra commented Nov 18, 2022

apoelstra commented Nov 18, 2022

tcharding commented Nov 18, 2022

tcharding commented Sep 16, 2022 •

edited

Loading

Kixunil commented Oct 25, 2022 •

edited

Loading

tcharding commented Nov 7, 2022 •

edited

Loading