Remove byte swap of valtree hash on big endian

[ecnelises]

This addresses problem reported in #103183. The code was originally introduced in e14b34c. (see #96591)

On big-endian environment, this operation sequence actually put the other half from 128-bit result, thus we got different hash result on LE and BE.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

[ecnelises]

FYI @lcnr @b-naber

Another suspicious thing is how downcast from u128 to u64 is handled in each platform. Per my understanding, it should always be cut so higher 64-bits are removed.

[lcnr]

i remember this not working before, i guess lets go with

@bors r+ rollup=iffy

[bors]

📌 Commit 7b5a366 has been approved by lcnr

It is now in the queue for this repository.

[b-naber]

Wasn't the problem that the StableHasherResult impl for u64 was wrong? We just use the first value of the tuple that StableHasher::finalize returns. This causes different values values on LE and BE machines afaict. Seems like that should be fixed instead.

[bors]

⌛ Testing commit 7b5a366 with merge f8c86c8...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing f8c86c8 to master...

[rust-timer]

Finished benchmarking commit (f8c86c8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean1	range	count2
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.3%	[4.6%, 5.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Footnotes

1. the arithmetic mean of the percent change ↩

2. number of relevant changes ↩

[ecnelises]

Wasn't the problem that the StableHasherResult impl for u64 was wrong? We just use the first value of the tuple that StableHasher::finalize returns. This causes different values values on LE and BE machines afaict. Seems like that should be fixed instead.

Hi, I tried print the value:

impl StableHasherResult for u128 {
    #[inline]
    fn finish(hasher: StableHasher) -> Self {
        let (_0, _1) = hasher.finalize();
        u128::from(_0) | (u128::from(_1) << 64)
    }
}

impl StableHasherResult for u64 {
    #[inline]
    fn finish(hasher: StableHasher) -> Self {
        let f = hasher.finalize();
        println!("{} {}", f.0, f.1);
        f.0
    }
}

For the case, both little-endian and big-endian machine print:

7154186058328073752 6777639898671907003

Not sure if this can trigger the failure you mentioned, but here the cut for u64 and u128 construction look correct to me.

[b-naber]

Thanks for investigating. I'm not really sure what's going on, it's seems weird to me. I remember that when we had the CI failure in that PR the values were flipped on that one machine and we had the hypothesis that this was an endianness problem and making the change in that commit did fix the CI failure. Can't explain why the original version of that code works now.