Adding more property tests and fixing edge case of dropped operation

[citizen-stig]

As follow up of #931

Includes a fix for:

RangeUpdater::new computed range_end with binary_search_by_key(...).unwrap_or_else(|i| i).

When a key in read_write exactly equals the worker's key_range_end (the max key path of the shard's region), binary_search_by_key returns Ok(i) and unwrap_or_else returns i instead of the required exclusive end i+1.

The matching key is dropped from the worker's range, so the seek is never pushed, no RootPagePending::SubTrie is emitted, and the second commit's root just inherits the first commit's leaf-root unchanged.

Tracing the failing case showed range_end=0 for the second commit (read_write contains only KEY_B = [0xFF; 32], and key_range_end = [0xFF; 32] for the single shard's universe region).

Note from @rphmeier

i will note that this situation would be extremely unlikely to occur with genuinely hashed keys due to the way we shard the key-space among workers, as it essentially requires a hash collision. but good future proofing.

Notable changes

the old account_path filled only bytes 0..16 (for i in 0..4 { path[i*4..][..4]... }); the new one in nomt-test-utils/src/lib.rs:599-611 uses chunks_exact_mut(4) and fills all 32 bytes. This is the real reason add_remove_1000's expected_roots changed — not a "fixture refresh" but a different key distribution. The new behavior is arguably better (more entropy), but it silently changes any test that depended on the old layout

[rphmeier]

this smells like a breaking change?

[citizen-stig]

Yeah...
Byt as far I understand, this is for the test fixture, in reality this will unlikely produce different root hash.

[rphmeier]

Can you explain why the root hashes changed here and would not in production?

[citizen-stig]

I've mentioned it in PR description. This PR changed how KeyPath is derived from id

Before it has been filling only 16 bytes:

    for i in 0..4 {
        // max(i) == 3, path[12..][..4]
        path[i * 4..][..4].copy_from_slice(&rng.next_u32().to_le_bytes());
    }

nomt/nomt/tests/common/mod.rs

Lines 13 to 24 in 7ef155b

	pub fn account_path(id: u64) -> KeyPath {
	// KeyPaths must be uniformly distributed, but we don't want to spend time on a good hash. So
	// the next best option is to use a PRNG seeded with the id.
	use rand::{Rng as _, SeedableRng as _};
	let mut seed = [0; 16];
	seed[0..8].copy_from_slice(&id.to_le_bytes());
	let mut rng = rand_pcg::Lcg64Xsh32::from_seed(seed);
	let mut path = KeyPath::default();
	for i in 0..4 {
	path[i * 4..][..4].copy_from_slice(&rng.next_u32().to_le_bytes());
	}
	path

And now it fills all 32 bytes:

nomt/nomt-test-utils/src/lib.rs

Lines 342 to 353 in ed8ad2c

	fn seeded_key(seed: u64) -> KeyPath {
	let mut rng_seed = [0; 16];
	rng_seed[..8].copy_from_slice(&seed.to_le_bytes());
	let mut rng = Lcg64Xsh32::from_seed(rng_seed);
	let mut key = [0; 32];
	for chunk in key.chunks_exact_mut(4) {
	chunk.copy_from_slice(&rng.next_u32().to_le_bytes());
	}
	key

[citizen-stig]

If we want to play safe, we can do following:

1. Merge: Remove unused dependencies and align benchtop #939
2. Do a patch release 1.0.3 -> 1.0.4
3. Merge this PR
4. Do a breaking change release, 1.0.4 -> 1.1.0 or even more 2.0 to be more aligned with semver

[rphmeier]

No need for that, I think this is OK. i see now that it's just because the test environment changing.

[rphmeier]

i see, if key_range_end is actually in the set of ops we would drop it from the range silently. seems like a good fix.

i will note that this situation would be extremely unlikely to occur with genuinely hashed keys due to the way we shard the key-space among workers, as it essentially requires a hash collision. but good future proofing.