feat: enforce canonicity on `HashSet/BTreeSet/HashMap/BTreeMap` #162

dj8yfo · 2023-06-26T11:55:14Z

Resolves #63

frol

There are only tests here, but no actual implementation that was in #160. Is it on purpose?

borsh/tests/common_macro.rs

borsh/tests/test_btree_map.rs

.github/test.sh

dj8yfo · 2023-06-26T16:21:46Z

There are only tests here, but no actual implementation that was in #160. Is it on purpose?

Nope, it's just draft. Pushed it to periodically see feedback from ci.

borsh/src/de/mod.rs

frol · 2023-06-27T13:21:37Z

borsh/tests/common_macro.rs

+            #[cfg(not(feature = "de_strict_order"))]
+            {
+                let result = result.unwrap();
+                assert_eq!(result.len(), arr_key.len());
+                for key in &arr_key {
+                    assert!(result.contains(key));
+
+                }
+
+            }
+
+            #[cfg(feature = "de_strict_order")]
+            {
+                assert!(result.is_err());
+
+                assert_eq!(result.unwrap_err().to_string(), ERROR_WRONG_ORDER_OF_KEYS);
+            }


You implemented it well, but semantically this feels wrong to me. This test ensures that if canonicity is broken, we still read the data just fine.

Let me think out loud here:

Currently, we ask people to opt-in for strong consistency, and if any crate in the dependencies tree opts-in (when borsh is used in several crates using different features), the strong consistency checks will be enabled everywhere, and potentially erroring on incorrectly constructed payloads and affecting performance (due to validation on deserialization).

Alternatively, we may enable the checks by default and create a feature to opt-out ("non-strict-mode"), and the risk will be the opposite that someone enabling the feature in one crate will disable canonicity checks (on deserialization), and potentially speed things up.

I cannot say I like either of the approaches more than the other, but this test case looks like we endorse broken canonicity. Also, I feel that to be on the safe side of things, I would enable this feature by default (when making a breaking change release), but at the same time, it would introduce a performance hit onto the users who always construct and read the data with borsh-rs (e.g. near-sdk-rs smart contracts, nearcore internals), and serialization is canonical there.

@dj8yfo What do you think?

I think the test just creates a piece of runnable documentation of actual behavior and doesn't attempt to endorse or denounce canonicity.
A feature by itself doesn't nudge crate's users in any particular direction, enabling/disabling it by default does. I think that decision at some point is up to you @frol.
To better understand performance impact, a benchmark with some data type(s) used in practice is due as part of this task/another pr.
My intuition is that performance impact must be negligible for most practical types, which don't attempt to allocate memory on comparisons. But, as it often happens with intuition, it may be wrong.

borsh/src/de/mod.rs

…p` impl [✓] (re)write `HashSet/BTreeSet/HashMap/BTreeMap` implementations ├── [✓] `HashSet` │ ├── [✓] rewrite to no extra `Vec` allocation │ └── [✓] `#cfg(...)` addon ├── [✓] `HashMap` │ ├── [✓] add `length_hint` in capacity allocation │ └── [✓] `#cfg(...)` addon ├── [✓] `BTreeSet` │ ├── [✓] leave it as is, as `BTreeSet` │ │ └── [✓] has optimisation for initialization from Iterator (with sorting) │ │ └── [✓] [set source](https://doc.rust-lang.org/src/alloc/collections/btree/set.rs.html#1210) │ └── [✓] `#cfg(...)` addon └── [✓] `BTreeMap` ├── [✓] replace with parsing to `Vec` first │ └── [✓] as `BTreeMap` has optimisation for init from Iterator (with sorting) │ └── [✓] [map source](https://doc.rust-lang.org/src/alloc/collections/btree/map.rs.html#2237) └── [✓] `#cfg(...)` addon

dj8yfo · 2023-06-28T14:30:44Z

benches cmp:
- baseline master and master2 correspond to same code 0f3e8c6fd80e4b9e60476b53d05499dcd4e7bda5 , warm-up-time 6, measurement-time 45
  - difference between master and master2 for .*10000/ benches is within 4%
  - difference between master and master2 for all benches is within 8%
critcmp master baseline_canonicity -t 5, baseline_canonicity is this branch without de_strict_order enabled

group                                                                                                     baseline_canonicity                    master
-----                                                                                                     -------------------                    ------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/       1.00  1029.6±11.21ns        ? ?/sec    1.13  1166.6±10.50ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/     1.00    146.2±0.94µs        ? ?/sec    1.55    227.2±1.35µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/    1.00  1675.0±13.88µs        ? ?/sec    1.65      2.8±0.02ms        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/       1.00    352.6±1.54ns        ? ?/sec    1.18    415.8±1.23ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/     1.00     38.0±0.12µs        ? ?/sec    2.21     84.0±0.41µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/    1.00    405.1±2.23µs        ? ?/sec    2.63   1064.4±6.20µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/                              1.00   620.4±20.01ns        ? ?/sec    1.08   667.0±16.22ns        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/           1.00  1643.7±18.87ns        ? ?/sec    1.27      2.1±0.02µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/         1.00    193.7±1.23µs        ? ?/sec    1.44    279.6±1.62µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/        1.00      2.2±0.02ms        ? ?/sec    1.33      2.9±0.10ms        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/           1.00    510.4±2.21ns        ? ?/sec    1.40    714.3±4.10ns        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/         1.00     46.0±0.25µs        ? ?/sec    1.77     81.4±0.22µs        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/        1.00    487.6±3.85µs        ? ?/sec    1.57    763.7±3.31µs        ? ?/sec

frol · 2023-06-28T14:43:40Z

baseline master and master2 correspond to same code

I don't see any other references to master2. Was it intended to be included as one more column in the benchmark table?

critcmp master baseline_canonicity -t 5, baseline_canonicity is this branch without de_strict_order enabled

Do I read it correctly that performance degraded even without de_strict_order getting enabled? And this is why you did 02ea305 ?

What is the current state? It sounds like you are still working on it, and we are not ready to merge, so I will turn this PR into draft (please, do it yourself next time to communicate this state). Turn it back "ready for review" once you are ready.

dj8yfo · 2023-06-29T10:59:51Z

This is a renewed summary of current pr.
Summary of changes:

HashSet default behaviour - unchanged
HashMap default behaviour -> changed to parse Vec<(K, V)> first
BTreeSet default behaviour - unchanged
BTreeMap default behaviour -> changed to parse Vec<(K, V)> first
all four of HashSet/HashMap/BTreeSet/BTreeMap got #[cfg(feature = "de_strict_order")] addon

Below is an example of expected errors in measurements (warm-up-time 30, measurement-time 210):

# compare master with itself; error up to 7%
❯ critcmp master master2 -t 6
group                                                                             master                                 master2
-----                                                                             ------                                 -------
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/    1.07     80.7±2.65µs        ? ?/sec    1.00     75.4±1.36µs        ? ?/sec

Next, is difference between master and baseline_canonicity (branch of pr with default features):

❯ critcmp master baseline_canonicity
group                                                                                                     baseline_canonicity                    master
-----                                                                                                     -------------------                    ------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/       1.08   1254.0±2.68ns        ? ?/sec    1.00   1156.9±3.08ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/     1.00    154.2±0.29µs        ? ?/sec    1.46    224.8±0.71µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/    1.00   1654.9±5.81µs        ? ?/sec    1.65      2.7±0.01ms        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/       1.00    369.2±1.90ns        ? ?/sec    1.12    414.9±1.39ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/     1.00     39.0±0.09µs        ? ?/sec    2.16     84.4±0.54µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/    1.00    413.6±0.72µs        ? ?/sec    2.52   1042.4±7.39µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/                              1.11    718.2±8.19ns        ? ?/sec    1.00    645.1±2.90ns        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/                            1.00     77.3±0.32µs        ? ?/sec    1.04     80.7±2.65µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10000/borsh_de/                           1.03    887.7±1.92µs        ? ?/sec    1.00    863.7±4.21µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10/borsh_de/                              1.00    172.2±3.60ns        ? ?/sec    1.02    176.0±3.02ns        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_1000/borsh_de/                            1.02     18.3±0.22µs        ? ?/sec    1.00     18.0±0.05µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10000/borsh_de/                           1.02    188.1±0.65µs        ? ?/sec    1.00    184.0±0.62µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/           1.00   1590.3±6.73ns        ? ?/sec    1.30      2.1±0.05µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/         1.00    198.2±5.08µs        ? ?/sec    1.41    279.9±0.86µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/        1.00      2.2±0.05ms        ? ?/sec    1.31      2.8±0.01ms        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/           1.00    521.8±9.18ns        ? ?/sec    1.36    707.5±1.65ns        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/         1.00     47.3±0.13µs        ? ?/sec    1.70     80.4±0.48µs        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/        1.00    505.3±1.18µs        ? ?/sec    1.49    751.3±2.40µs        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10/borsh_de/                                  1.04   1066.7±1.40ns        ? ?/sec    1.00   1024.6±2.85ns        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_1000/borsh_de/                                1.03    119.9±2.29µs        ? ?/sec    1.00    116.1±1.33µs        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10000/borsh_de/                               1.00  1255.9±14.32µs        ? ?/sec    1.01  1272.7±40.84µs        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/                                  1.13   381.0±30.18ns        ? ?/sec    1.00    335.7±2.65ns        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_1000/borsh_de/                                1.00     29.5±0.89µs        ? ?/sec    1.00     29.6±0.27µs        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10000/borsh_de/                               1.00    306.3±1.98µs        ? ?/sec    1.02    312.9±1.26µs        ? ?/sec

This list has 2 anomalies, which must be due to some intermittent error on specific hardware, which i couldn't get rid of even with prolonged measument times.

group                                                                                                     baseline_canonicity                    master
-----                                                                                                     -------------------                    ------
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/                              1.11    718.2±8.19ns        ? ?/sec    1.00    645.1±2.90ns        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/                                  1.13   381.0±30.18ns        ? ?/sec    1.00    335.7±2.65ns        ? ?/sec

BTreeSet and HashSet implementations with default features hasn't changed, so it must be 0% instead of 11% and 13%.

Finally, raw unadjusted data of difference between baseline_canonicity and canonicity_de_strict_order (branch of pr with de_strict_order feature enabled):

❯ critcmp  baseline_canonicity canonicity_de_strict_order
group                                                                                                     baseline_canonicity                    canonicity_de_strict_order
-----                                                                                                     -------------------                    --------------------------
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10/borsh_de/       1.00   1254.0±2.68ns        ? ?/sec    1.01   1268.7±3.63ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_1000/borsh_de/     1.00    154.2±0.29µs        ? ?/sec    1.00    154.9±5.27µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<alloc::string::String, alloc::string::String>_10000/borsh_de/    1.00   1654.9±5.81µs        ? ?/sec    1.02   1689.3±4.42µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/       1.01    369.2±1.90ns        ? ?/sec    1.00    366.5±1.37ns        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/     1.00     39.0±0.09µs        ? ?/sec    1.00     39.1±0.07µs        ? ?/sec
alloc::collections::btree::map::BTreeMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/    1.00    413.6±0.72µs        ? ?/sec    1.01    415.9±0.78µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10/borsh_de/                              1.00    718.2±8.19ns        ? ?/sec    1.02    734.4±5.56ns        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_1000/borsh_de/                            1.00     77.3±0.32µs        ? ?/sec    1.08     83.5±0.32µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<alloc::string::String>_10000/borsh_de/                           1.00    887.7±1.92µs        ? ?/sec    1.06    936.8±5.02µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10/borsh_de/                              1.00    172.2±3.60ns        ? ?/sec    1.07    184.7±1.41ns        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_1000/borsh_de/                            1.00     18.3±0.22µs        ? ?/sec    1.07     19.6±0.34µs        ? ?/sec
alloc::collections::btree::set::BTreeSet<benchmarks::PublicKey>_10000/borsh_de/                           1.00    188.1±0.65µs        ? ?/sec    1.06    198.5±0.44µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10/borsh_de/           1.01   1590.3±6.73ns        ? ?/sec    1.00   1569.7±9.70ns        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_1000/borsh_de/         1.00    198.2±5.08µs        ? ?/sec    1.00    197.9±0.43µs        ? ?/sec
std::collections::hash::map::HashMap<alloc::string::String, alloc::string::String>_10000/borsh_de/        1.00      2.2±0.05ms        ? ?/sec    1.05      2.3±0.05ms        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10/borsh_de/           1.00    521.8±9.18ns        ? ?/sec    1.05   548.0±25.73ns        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_1000/borsh_de/         1.00     47.3±0.13µs        ? ?/sec    1.13     53.3±2.53µs        ? ?/sec
std::collections::hash::map::HashMap<benchmarks::PublicKey, benchmarks::PublicKey>_10000/borsh_de/        1.00    505.3±1.18µs        ? ?/sec    1.01    511.3±1.67µs        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10/borsh_de/                                  1.02   1066.7±1.40ns        ? ?/sec    1.00   1043.3±2.58ns        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_1000/borsh_de/                                1.01    119.9±2.29µs        ? ?/sec    1.00    119.2±1.13µs        ? ?/sec
std::collections::hash::set::HashSet<alloc::string::String>_10000/borsh_de/                               1.00  1255.9±14.32µs        ? ?/sec    1.02   1275.2±1.75µs        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10/borsh_de/                                  1.14   381.0±30.18ns        ? ?/sec    1.00    334.2±2.69ns        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_1000/borsh_de/                                1.00     29.5±0.89µs        ? ?/sec    1.04     30.7±0.36µs        ? ?/sec
std::collections::hash::set::HashSet<benchmarks::PublicKey>_10000/borsh_de/                               1.00    306.3±1.98µs        ? ?/sec    1.26    385.9±0.37µs        ? ?/sec

frol · 2023-06-29T13:12:42Z

@dj8yfo Do I read it right, that:

de_strict_order feature adds a negligible performance penalty almost indistinguishable from the measurement noise
The new baseline implementations with reading to Vec first bring a significant performance improvement

This is really nice!

frol

This PR not only brought canonicity checks (opt-in for now), but also x1.5-2 performance improvements to the deserialization of BTreeMap and HashMap collections!

dj8yfo requested a review from frol as a code owner June 26, 2023 11:55

dj8yfo force-pushed the enforce_canonicity_2 branch 3 times, most recently from 18d078f to bf30615 Compare June 26, 2023 13:54

frol requested changes Jun 26, 2023

View reviewed changes

borsh/tests/common_macro.rs Outdated Show resolved Hide resolved

borsh/tests/test_btree_map.rs Show resolved Hide resolved

.github/test.sh Outdated Show resolved Hide resolved

frol marked this pull request as draft June 26, 2023 15:14

dj8yfo force-pushed the enforce_canonicity_2 branch 7 times, most recently from e3db33a to a4e7039 Compare June 27, 2023 12:25

dj8yfo commented Jun 27, 2023

View reviewed changes

borsh/src/de/mod.rs Outdated Show resolved Hide resolved

dj8yfo changed the title ~~[wip] feat: enforce canonicity on HashSet/BTreeSet/HashMap/BTreeMap~~ feat: enforce canonicity on HashSet/BTreeSet/HashMap/BTreeMap Jun 27, 2023

dj8yfo marked this pull request as ready for review June 27, 2023 12:32

frol requested changes Jun 27, 2023

View reviewed changes

dj8yfo force-pushed the enforce_canonicity_2 branch 2 times, most recently from c0f3e1c to 6061892 Compare June 27, 2023 15:11

dj8yfo mentioned this pull request Jun 27, 2023

create minimal benches for sets/maps #163

Closed

frol mentioned this pull request Jun 28, 2023

feat: sets/maps benches for reference point #164

Merged

dj8yf0μl added 5 commits June 28, 2023 14:26

test: add more test cases for HashMap/HashSet/BTreemap/BTreeSet

8a5109d

test: construct failing test cases for the de_strict_order feature

383e63c

doc: add de_strict_order mention to rustdoc

c019d81

chore: canonicity review fixes

c52c2fa

dj8yfo force-pushed the enforce_canonicity_2 branch from 6061892 to c52c2fa Compare June 28, 2023 11:27

feat: replace HashSet and HashMap de impl with faster versions

02ea305

frol marked this pull request as draft June 28, 2023 14:43

dj8yfo marked this pull request as ready for review June 29, 2023 11:04

frol approved these changes Jun 29, 2023

View reviewed changes

frol merged commit 1e2d74f into near:master Jun 29, 2023
5 checks passed

dj8yfo mentioned this pull request Aug 2, 2023

chore: release #146

Merged

frol mentioned this pull request Aug 9, 2023

chore: release #189

Closed

frol mentioned this pull request Aug 24, 2023

chore!: PartialOrd -> Ord bound for hashcollections #203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enforce canonicity on `HashSet/BTreeSet/HashMap/BTreeMap` #162

feat: enforce canonicity on `HashSet/BTreeSet/HashMap/BTreeMap` #162

dj8yfo commented Jun 26, 2023

frol left a comment

dj8yfo commented Jun 26, 2023

frol Jun 27, 2023

dj8yfo Jun 27, 2023 •

edited

dj8yfo Jun 27, 2023

dj8yfo commented Jun 28, 2023 •

edited

frol commented Jun 28, 2023

dj8yfo commented Jun 29, 2023

frol commented Jun 29, 2023 •

edited

frol left a comment

feat: enforce canonicity on HashSet/BTreeSet/HashMap/BTreeMap #162

feat: enforce canonicity on HashSet/BTreeSet/HashMap/BTreeMap #162

Conversation

dj8yfo commented Jun 26, 2023

frol left a comment

Choose a reason for hiding this comment

dj8yfo commented Jun 26, 2023

frol Jun 27, 2023

Choose a reason for hiding this comment

dj8yfo Jun 27, 2023 • edited

Choose a reason for hiding this comment

dj8yfo Jun 27, 2023

Choose a reason for hiding this comment

dj8yfo commented Jun 28, 2023 • edited

frol commented Jun 28, 2023

dj8yfo commented Jun 29, 2023

frol commented Jun 29, 2023 • edited

frol left a comment

Choose a reason for hiding this comment

feat: enforce canonicity on `HashSet/BTreeSet/HashMap/BTreeMap` #162

feat: enforce canonicity on `HashSet/BTreeSet/HashMap/BTreeMap` #162

dj8yfo Jun 27, 2023 •

edited

dj8yfo commented Jun 28, 2023 •

edited

frol commented Jun 29, 2023 •

edited