equihash: Add Rust API for Tromp solver #1083

str4d · 2024-01-04T04:42:20Z

No description provided.

components/equihash/tromp/equi_miner.c

components/equihash/src/lib.rs

codecov · 2024-01-04T19:08:43Z

Codecov Report

Attention: Patch coverage is 92.80000% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 66.82%. Comparing base (f7c29f0) to head (b201c10).
Report is 10 commits behind head on main.

❗ Current head b201c10 differs from pull request most recent head 76131db. Consider uploading reports for the commit 76131db to get more accurate results

Files	Patch %	Lines
components/equihash/src/blake2b.rs	70.83%	7 Missing ⚠️
components/equihash/src/tromp.rs	97.95%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1083      +/-   ##
==========================================
+ Coverage   63.23%   66.82%   +3.58%     
==========================================
  Files         124      134      +10     
  Lines       14507    19622    +5115     
==========================================
+ Hits         9174    13113    +3939     
- Misses       5333     6509    +1176

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

str4d · 2024-04-18T20:07:58Z

Force-pushed to clean up the commit history; no changes to @teor2345's changes, just regrouping them (and squashing some of the fixes into the original commits that didn't make sense to keep in standalone commits).

str4d · 2024-04-18T20:14:06Z

Force-pushed to GitHub-linkify an imported source link in a commit message.

str4d · 2024-04-18T20:14:31Z

And somehow I managed to click two wrong buttons in a row 🤦

str4d · 2024-04-18T20:10:04Z

components/equihash/src/tromp.rs

-        .collect()
+        .collect();
+
+    // Just in case the solver returns solutions that become the same when compressed.


I don't think this should ever be the case. The compressed representation should not be lossy, it just packs the indices together to remove the effective padding bits that exist in the u32 representation.

~~It may not have been observed, or there could be a subtle difference between the C code and the Rust code in compress_array().~~

Update: Yeah, something must be wrong if identical solutions were observed, I'll check for changes to the logic in equi_miner.c, everything else looks equivalent to the zcashd logic.

Update: Duplicate solutions were observed prior to a bug fix, is there any chance that the solver could produce duplicate uncompressed solutions?

str4d · 2024-04-18T20:11:35Z

components/equihash/src/tromp.rs

@@ -70,13 +71,21 @@ unsafe fn worker(eq: *mut CEqui, p: verify::Params, curr_state: &State) -> Vec<V
        let nsols = equi_nsols(eq);
        let sols = equi_sols(eq);
        let solution_len = 1 << p.k;
+        //println!("{nsols} solutions of length {solution_len} at {sols:?}");


I'm inclined to remove this and the other introduced debugging print statements, unless anyone else disagrees.

str4d · 2024-04-18T20:53:41Z

Force-pushed to clean up the feature flagging in the build script, and add C #ifdefs to prevent some warnings.

str4d · 2024-04-18T23:21:59Z

Rebased on current main. I will rebase this PR again once #1357 merges.

Source: zcash/zcash@01d5576 License: MIT

Co-authored-by: teor <[email protected]>

This avoids linker errors by removing cycles.

Co-authored-by: teor <[email protected]>

str4d · 2024-04-19T02:01:34Z

Rebased on main to fix merge conflicts after #1357 was merged.

str4d · 2024-04-19T02:27:16Z

Force-pushed to move the new minimal-form compression methods into the new submodule from #1357.

…rees Also includes some redundant cleanup code for defense in depth.

Source: mikepb/endian.h@0f885cb License: Public Domain (or "BSD OR MIT OR Apache-2.0")

The equivalent change is made to the C worker, which is unused.

str4d · 2024-04-19T02:29:00Z

Force-pushed to adjust the minimal-form compression method APIs and integrate them into the existing tests.

str4d · 2024-04-19T02:33:19Z

components/equihash/src/tromp.rs

+/// `next_nonce` function.
+///
+/// Returns zero or more unique solutions.
+pub fn solve_200_9<const N: usize>(


I don't think there is value in exposing the API for uncompressed solutions; block headers only use compressed solutions, and this crate's other existing API is only for verifying compressed solutions. So I'll probably just make this internal.

What I do want to think about before merging is how to handle configurable parameters. We will need at least two parameter sets (for mainnet and regtest), and depending on how that will be enabled in the C code, we might end up supporting any parameter set. But if we don't then we should probably instead have an enum or a generic parameter for selecting which Equihash parameters to solve with.

I don't think there is value in exposing the API for uncompressed solutions; block headers only use compressed solutions, and this crate's other existing API is only for verifying compressed solutions. So I'll probably just make this internal.

This is currently true for Zebra's use case. I see no harm in exposing the API for uncompressed solutions either, so solutions could be compressed outside this crate more easily (though it seems unlikely that anyone will actually do that).

What I do want to think about before merging is how to handle configurable parameters. We will need at least two parameter sets (for mainnet and regtest), and depending on how that will be enabled in the C code, we might end up supporting any parameter set. But if we don't then we should probably instead have an enum or a generic parameter for selecting which Equihash parameters to solve with.

It may be useful to allow for configurable parameters for custom testnets to allow for picking even easier parameters instead of disabling PoW checks altogether for ephemeral testnets, or harder ones than those used by regtest for testnets that are intended to be used for longer periods.

Let me know what you decide so we can make the parameters configurable in Zebra if they're configurable here.

Update: Configurable parameters don't seem easy to add, let's allow the Regtest parameters later but leave it there.

arya2

This looks like it should work so far, and none of my suggestions are blockers, feel free to resolve any/all of them without making changes.

I'm still reviewing equi_miner.c in detail, but everything else looks equivalent to the logic in zcashd, at least for the Mainnet parameters.

Update: Collision errors are still being logged here (and DuplicateIdxs errors).

arya2 · 2024-07-04T21:53:47Z

components/equihash/tromp/equi.h

@@ -0,0 +1,47 @@
+// Equihash solver
+// Copyright (c) 2016-2016 John Tromp, The Zcash developers


Suggested change

// Copyright (c) 2016-2016 John Tromp, The Zcash developers

// Copyright (c) 2016 John Tromp, The Zcash developers

arya2 · 2024-07-04T22:11:17Z

components/equihash/tromp/portable_endian.h

+// example on how to get the endian conversion functions on different
+// platforms.
+
+// Downloaded from https://raw.githubusercontent.com/mikepb/endian.h/master/endian.h


Suggested change

// Downloaded from https://raw.githubusercontent.com/mikepb/endian.h/master/endian.h

// Downloaded and modified from https://raw.githubusercontent.com/mikepb/endian.h/master/endian.h

arya2 · 2024-07-04T23:16:03Z

components/equihash/src/minimal.rs

+// https://github.com/zcash/zcash/blob/6fdd9f1b81d3b228326c9826fa10696fc516444b/src/crypto/equihash.cpp#L39-L76
+#[cfg(any(feature = "solver", test))]
+fn compress_array(array: &[u8], bit_len: usize, byte_pad: usize) -> Vec<u8> {
+    let index_bytes = (u32::BITS / 8) as usize;


For consistency:

Suggested change

let index_bytes = (u32::BITS / 8) as usize;

let index_bytes = size_of::<u32>();

arya2 · 2024-07-05T00:09:49Z

components/equihash/src/minimal.rs

+                )
+                    .wrapping_shl(8 * (in_width - x - 1) as u32); // Big-endian


This seems like an unnecessary change from the c++ logic, it should panic in minimal_from_indices() before it gets here when using parameters that would be affected by the change.

Suggested change

)

.wrapping_shl(8 * (in_width - x - 1) as u32); // Big-endian

) << (8 * (in_width - x - 1) as u32); // Big-endian

arya2 · 2024-07-05T00:15:45Z

components/equihash/src/minimal.rs

+#[cfg(any(feature = "solver", test))]
+pub(crate) fn minimal_from_indices(p: Params, indices: &[u32]) -> Vec<u8> {
+    let c_bit_len = p.collision_bit_length();
+    let index_bytes = (u32::BITS / 8) as usize;


For consistency:

Suggested change

let index_bytes = (u32::BITS / 8) as usize;

let index_bytes = size_of::<u32>();

arya2 · 2024-07-05T02:32:21Z

components/equihash/src/tromp.rs

+    _f: [u8; 0],
+    _m: PhantomData<(*mut u8, PhantomPinned)>,


Could we add a short explanation here?

arya2 · 2024-07-05T02:33:51Z

components/equihash/tromp/equi_miner.c

+      eq->blake2b_update(state, (uchar *)&leb, sizeof(u32));
+      eq->blake2b_finalize(state, hash, HASHOUT);
+      eq->blake2b_free(state);
+      // Avoid use-after-free and double-free


It may be a good idea to add this warning to the blake2b_free() function docs too.

str4d commented Jan 4, 2024

View reviewed changes

components/equihash/tromp/equi_miner.c Show resolved Hide resolved

str4d commented Jan 4, 2024

View reviewed changes

components/equihash/src/lib.rs Outdated Show resolved Hide resolved

str4d force-pushed the equihash-solver-tromp branch from cb60b5b to f3ea80e Compare January 4, 2024 18:47

teor2345 mentioned this pull request Jan 8, 2024

feat(mine): Add an internal Zcash miner to Zebra ZcashFoundation/zebra#8136

Merged

11 tasks

bishopcheckmate mentioned this pull request Jan 16, 2024

Add Zebra crate versions to dev-dependencies, to enable crater tests ZcashFoundation/zebra#6924

Closed

3 tasks

This was referenced Jan 22, 2024

Remove or comment out zebra internal miner ZcashFoundation/zebra#8180

Closed

Restore internal miner ZcashFoundation/zebra#8183

Open

str4d mentioned this pull request Jan 26, 2024

equihash: Fix some Rust API issues #1088

Merged

daira assigned str4d Mar 14, 2024

str4d force-pushed the equihash-solver-tromp branch from 8e66e0d to d33ec1d Compare April 18, 2024 20:06

str4d force-pushed the equihash-solver-tromp branch from d33ec1d to 9dbdf18 Compare April 18, 2024 20:13

str4d closed this Apr 18, 2024

str4d deleted the equihash-solver-tromp branch April 18, 2024 20:14

str4d restored the equihash-solver-tromp branch April 18, 2024 20:14

str4d reopened this Apr 18, 2024

str4d commented Apr 18, 2024

View reviewed changes

str4d force-pushed the equihash-solver-tromp branch from 9dbdf18 to b201c10 Compare April 18, 2024 20:52

str4d mentioned this pull request Apr 18, 2024

Refactor equihash code #1357

Merged

str4d force-pushed the equihash-solver-tromp branch from b201c10 to be16706 Compare April 18, 2024 23:21

str4d and others added 5 commits April 19, 2024 02:00

equihash: Import Tromp solver

45652a2

Source: zcash/zcash@01d5576 License: MIT

equihash: Import blake2b_simd C bindings from zcashd

7ab6c47

Source: zcash/zcash@01d5576 License: MIT

equihash: Modify Tromp solver to compile as C

3aaeb8b

Co-authored-by: teor <[email protected]>

equihash: Pass blake2b_simd bindings to Tromp solver as callbacks

45e7238

This avoids linker errors by removing cycles.

equihash: Add Rust API for Tromp solver

d07505d

Co-authored-by: teor <[email protected]>

str4d force-pushed the equihash-solver-tromp branch from be16706 to ed22417 Compare April 19, 2024 02:00

str4d force-pushed the equihash-solver-tromp branch from ed22417 to 6571aa9 Compare April 19, 2024 02:26

teor2345 added 11 commits April 19, 2024 02:27

equihash: Add Rust APIs for compressed solutions

1b20c15

equihash: Verify compressed solutions in tests

fe3b269

equihash: Move allocation out of the loop

463e7d9

equihash: Set C pointers to NULL after freeing them to avoid double-f…

3c78bf6

…rees Also includes some redundant cleanup code for defense in depth.

equihash: Place solver behind a feature flag

5f77bd7

equihash: Ensure returned compressed solutions are unique

d7ccd07

equihash: Add a portable endian.h for htole32() on macOS and Windows

989f40e

Source: mikepb/endian.h@0f885cb License: Public Domain (or "BSD OR MIT OR Apache-2.0")

equihash: Remove unused thread support to enable Windows compilation

b737d0f

equihash: Don't import a header that's missing in Windows CI

2bd7bc8

equihash: Clear slots when setting the hash state

9391e65

The equivalent change is made to the C worker, which is unused.

Add commented-out prints of solution candidates for debugging

76131db

str4d force-pushed the equihash-solver-tromp branch from 6571aa9 to 76131db Compare April 19, 2024 02:28

str4d commented Apr 19, 2024

View reviewed changes

str4d marked this pull request as ready for review April 19, 2024 02:34

arya2 self-requested a review June 20, 2024 15:05

arya2 reviewed Jul 5, 2024

View reviewed changes

arya2 mentioned this pull request Oct 2, 2024

add(mining): Restore internal miner ZcashFoundation/zebra#8906

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

equihash: Add Rust API for Tromp solver #1083

equihash: Add Rust API for Tromp solver #1083

str4d commented Jan 4, 2024

codecov bot commented Jan 4, 2024 •

edited

Loading

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

str4d Apr 18, 2024

arya2 May 4, 2024 •

edited

Loading

str4d Apr 18, 2024

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

str4d commented Apr 19, 2024

str4d commented Apr 19, 2024

str4d commented Apr 19, 2024

str4d Apr 19, 2024

arya2 May 4, 2024 •

edited

Loading

arya2 left a comment •

edited

Loading

arya2 Jul 4, 2024

arya2 Jul 4, 2024

arya2 Jul 4, 2024

arya2 Jul 5, 2024 •

edited

Loading

arya2 Jul 5, 2024

arya2 Jul 5, 2024

arya2 Jul 5, 2024

		@@ -0,0 +1,47 @@
		// Equihash solver
		// Copyright (c) 2016-2016 John Tromp, The Zcash developers

	// Copyright (c) 2016-2016 John Tromp, The Zcash developers
	// Copyright (c) 2016 John Tromp, The Zcash developers

	// Downloaded from https://raw.githubusercontent.com/mikepb/endian.h/master/endian.h
	// Downloaded and modified from https://raw.githubusercontent.com/mikepb/endian.h/master/endian.h

	let index_bytes = (u32::BITS / 8) as usize;
	let index_bytes = size_of::<u32>();

		)
		.wrapping_shl(8 * (in_width - x - 1) as u32); // Big-endian

	)
	.wrapping_shl(8 * (in_width - x - 1) as u32); // Big-endian
	) << (8 * (in_width - x - 1) as u32); // Big-endian

equihash: Add Rust API for Tromp solver #1083

Are you sure you want to change the base?

equihash: Add Rust API for Tromp solver #1083

Conversation

str4d commented Jan 4, 2024

codecov bot commented Jan 4, 2024 • edited Loading

Codecov Report

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

Choose a reason for hiding this comment

arya2 May 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Apr 18, 2024

str4d commented Apr 18, 2024

str4d commented Apr 19, 2024

str4d commented Apr 19, 2024

str4d commented Apr 19, 2024

Choose a reason for hiding this comment

arya2 May 4, 2024 • edited Loading

Choose a reason for hiding this comment

arya2 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 4, 2024 •

edited

Loading

arya2 May 4, 2024 •

edited

Loading

arya2 May 4, 2024 •

edited

Loading

arya2 left a comment •

edited

Loading

arya2 Jul 5, 2024 •

edited

Loading