Merge pull request #739 from daira/fix-sort-nondeterminism

Fix a nondeterminism bug: we were depending on sort order

halo2_proofs/CHANGELOG.md

@@ -6,6 +6,25 @@ and this project adheres to Rust's notion of
 [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+### Breaking circuit changes
+- `halo2_proofs::circuit::floor_planner::V1` was relying internally on the Rust
+  standard library's [`slice::sort_unstable_by_key`]; while it is deterministic,
+  it is not stable across targets or compiler versions. In particular, an edge
+  case within the sorting algorithm differed between 32-bit and 64-bit targets.
+  This meant that some circuits (like the [Orchard circuit]) would be laid out
+  differently, resulting in incompatible verifying keys. This release makes a
+  **breaking change** to the behaviour of `floor_planner::V1` to instead use a
+  stable sort.
+  - To retain compatibility with the Orchard circuit as deployed in [Zcash NU5],
+    a new `floor-planner-v1-legacy-pdqsort` feature flag has been added. When
+    enabled, `floor_planner::V1` instead pins its behaviour to the version of
+    `slice::sort_unstable_by_key` from Rust 1.56.1, always matching how that
+    version behaved on 64-bit targets.
+
+[`slice::sort_unstable_by_key`]: https://doc.rust-lang.org/stable/std/primitive.slice.html#method.sort_unstable_by_key
+[Orchard circuit]: https://github.com/zcash/orchard/blob/0.3.0/src/circuit.rs
+[Zcash NU5]: https://zips.z.cash/zip-0252
+
 ### Added
 - The following structs now derive the `Eq` trait:
   - `halo2_proofs::dev`:

halo2_proofs/Cargo.toml

@@ -57,6 +57,9 @@ maybe-rayon = {version = "0.1.0", default-features = false}
 plotters = { version = "0.3.0", default-features = false, optional = true }
 tabbycat = { version = "0.1", features = ["attributes"], optional = true }
 
+# Legacy circuit compatibility
+halo2_legacy_pdqsort = { version = "0.1.0", optional = true }
+
 [dev-dependencies]
 assert_matches = "1.5"
 criterion = "0.3"
@@ -78,6 +81,7 @@ test-dev-graph = [
 gadget-traces = ["backtrace"]
 sanity-checks = []
 batch = ["rand_core/getrandom"]
+floor-planner-v1-legacy-pdqsort = ["halo2_legacy_pdqsort"]
 
 # In-development features
 # See https://zcash.github.io/halo2/dev/features.html

halo2_proofs/src/circuit/floor_planner/v1/strategy.rs

@@ -199,7 +199,7 @@ pub fn slot_in_biggest_advice_first(
     region_shapes: Vec<RegionShape>,
 ) -> (Vec<RegionStart>, CircuitAllocations) {
     let mut sorted_regions: Vec<_> = region_shapes.into_iter().collect();
-    sorted_regions.sort_unstable_by_key(|shape| {
+    let sort_key = |shape: &RegionShape| {
         // Count the number of advice columns
         let advice_cols = shape
             .columns()
@@ -211,7 +211,24 @@ pub fn slot_in_biggest_advice_first(
             .count();
         // Sort by advice area (since this has the most contention).
         advice_cols * shape.row_count()
-    });
+    };
+
+    // This used to incorrectly use `sort_unstable_by_key` with non-unique keys, which gave
+    // output that differed between 32-bit and 64-bit platforms, and potentially between Rust
+    // versions.
+    // We now use `sort_by_cached_key` with non-unique keys, and rely on `region_shapes`
+    // being sorted by region index (which we also rely on below to return `RegionStart`s
+    // in the correct order).
+    #[cfg(not(feature = "floor-planner-v1-legacy-pdqsort"))]
+    sorted_regions.sort_by_cached_key(sort_key);
+
+    // To preserve compatibility, when the "floor-planner-v1-legacy-pdqsort" feature is enabled,
+    // we use a copy of the pdqsort implementation from the Rust 1.56.1 standard library, fixed
+    // to its behaviour on 64-bit platforms.
+    // https://github.com/rust-lang/rust/blob/1.56.1/library/core/src/slice/mod.rs#L2365-L2402
+    #[cfg(feature = "floor-planner-v1-legacy-pdqsort")]
+    halo2_legacy_pdqsort::sort::quicksort(&mut sorted_regions, |a, b| sort_key(a).lt(&sort_key(b)));
+
     sorted_regions.reverse();
 
     // Lay out the sorted regions.