chore: remove weak hashing which does not pull its weight (#10005)

We've had weak/rolling hashing in the code for quite a while. It was a popular request for a while, based on the belief that rsync does this and we should too. However, the benefit is quite small; we save on average about 0.8% of transferred blocks over the population as a whole: <img width="974" alt="Screenshot 2025-03-28 at 17 09 02" src="https://github.com/user-attachments/assets/bbe10dea-f85e-4043-9823-7cef1220b4a2" /> This would be fine if the cost was comparably low, however the downside of attempting rolling hash matching is that we (by default) do a complete file read on the destination in order to look for matches before we starting pulling blocks for the file. For any larger file this means a sometimes long, I/O-intensive pause before the file starts syncing, for usually no benefit. I propose we simply rip off the bandaid and save the effort.
2025-03-29 13:21:10 +01:00
parent 1a25ae32ca
commit b1c8f88a44
38 changed files with 288 additions and 1030 deletions
@@ -16,7 +16,7 @@ import (
 )

 // HashFile hashes the files and returns a list of blocks representing the file.
-func HashFile(ctx context.Context, folderID string, fs fs.Filesystem, path string, blockSize int, counter Counter, useWeakHashes bool) ([]protocol.BlockInfo, error) {
+func HashFile(ctx context.Context, folderID string, fs fs.Filesystem, path string, blockSize int, counter Counter) ([]protocol.BlockInfo, error) {
 	fd, err := fs.Open(path)
 	if err != nil {
 		l.Debugln("open:", err)
@@ -36,7 +36,7 @@ func HashFile(ctx context.Context, folderID string, fs fs.Filesystem, path strin

 	// Hash the file. This may take a while for large files.

-	blocks, err := Blocks(ctx, fd, blockSize, size, counter, useWeakHashes)
+	blocks, err := Blocks(ctx, fd, blockSize, size, counter)
 	if err != nil {
 		l.Debugln("blocks:", err)
 		return nil, err
@@ -108,7 +108,7 @@ func (ph *parallelHasher) hashFiles(ctx context.Context) {
 				panic("Bug. Asked to hash a directory or a deleted file.")
 			}

-			blocks, err := HashFile(ctx, ph.folderID, ph.fs, f.Name, f.BlockSize(), ph.counter, true)
+			blocks, err := HashFile(ctx, ph.folderID, ph.fs, f.Name, f.BlockSize(), ph.counter)
 			if err != nil {
 				handleError(ctx, "hashing", f.Name, err, ph.outbox)
 				continue
@@ -10,8 +10,6 @@ import (
 	"bytes"
 	"context"
 	"crypto/sha256"
-	"hash"
-	"hash/adler32"
 	"io"

 	"github.com/syncthing/syncthing/lib/protocol"
@@ -24,7 +22,7 @@ type Counter interface {
 }

 // Blocks returns the blockwise hash of the reader.
-func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, counter Counter, useWeakHashes bool) ([]protocol.BlockInfo, error) {
+func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, counter Counter) ([]protocol.BlockInfo, error) {
 	if counter == nil {
 		counter = &noopCounter{}
 	}
@@ -32,15 +30,6 @@ func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, cou
 	hf := sha256.New()
 	const hashLength = sha256.Size

-	var weakHf hash.Hash32 = noopHash{}
-	var multiHf io.Writer = hf
-	if useWeakHashes {
-		// Use an actual weak hash function, make the multiHf
-		// write to both hash functions.
-		weakHf = adler32.New()
-		multiHf = io.MultiWriter(hf, weakHf)
-	}
-
 	var blocks []protocol.BlockInfo
 	var hashes, thisHash []byte

@@ -70,7 +59,7 @@ func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, cou
 		}

 		lr.N = int64(blocksize)
-		n, err := io.CopyBuffer(multiHf, lr, buf)
+		n, err := io.CopyBuffer(hf, lr, buf)
 		if err != nil {
 			return nil, err
 		}
@@ -87,17 +76,15 @@ func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, cou
 		thisHash, hashes = hashes[:hashLength], hashes[hashLength:]

 		b := protocol.BlockInfo{
-			Size:     int(n),
-			Offset:   offset,
-			Hash:     thisHash,
-			WeakHash: weakHf.Sum32(),
+			Size:   int(n),
+			Offset: offset,
+			Hash:   thisHash,
 		}

 		blocks = append(blocks, b)
 		offset += n

 		hf.Reset()
-		weakHf.Reset()
 	}

 	if len(blocks) == 0 {
@@ -112,14 +99,8 @@ func Blocks(ctx context.Context, r io.Reader, blocksize int, sizehint int64, cou
 	return blocks, nil
 }

-// Validate quickly validates buf against the 32-bit weakHash, if not zero,
-// else against the cryptohash hash, if len(hash)>0. It is satisfied if
-// either hash matches or neither hash is given.
-func Validate(buf, hash []byte, weakHash uint32) bool {
-	if weakHash != 0 && adler32.Checksum(buf) == weakHash {
-		return true
-	}
-
+// Validate validates the hash, if len(hash)>0.
+func Validate(buf, hash []byte) bool {
 	if len(hash) > 0 {
 		hbuf := sha256.Sum256(buf)
 		return bytes.Equal(hbuf[:], hash)
@@ -9,51 +9,40 @@ package scanner
 import (
 	"bytes"
 	"context"
-	"crypto/rand"
 	"crypto/sha256"
 	"fmt"
-	origAdler32 "hash/adler32"
 	mrand "math/rand"
 	"testing"
-	"testing/quick"
-
-	rollingAdler32 "github.com/chmduquesne/rollinghash/adler32"
-	"github.com/syncthing/syncthing/lib/protocol"
 )

 var blocksTestData = []struct {
 	data      []byte
 	blocksize int
 	hash      []string
-	weakhash  []uint32
 }{
 	{
 		[]byte(""), 1024,
 		[]string{
 			"e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
 		},
-		[]uint32{0},
 	},
 	{
 		[]byte("contents"), 1024,
 		[]string{
 			"d1b2a59fbea7e20077af9f91b27e95e865061b270be03ff539ab3b73587882e8",
 		},
-		[]uint32{0x0f3a036f},
 	},
 	{
 		[]byte("contents"), 9,
 		[]string{
 			"d1b2a59fbea7e20077af9f91b27e95e865061b270be03ff539ab3b73587882e8",
 		},
-		[]uint32{0x0f3a036f},
 	},
 	{
 		[]byte("contents"), 8,
 		[]string{
 			"d1b2a59fbea7e20077af9f91b27e95e865061b270be03ff539ab3b73587882e8",
 		},
-		[]uint32{0x0f3a036f},
 	},
 	{
 		[]byte("contents"), 7,
@@ -61,7 +50,6 @@ var blocksTestData = []struct {
 			"ed7002b439e9ac845f22357d822bac1444730fbdb6016d3ec9432297b9ec9f73",
 			"043a718774c572bd8a25adbeb1bfcd5c0256ae11cecf9f9c3f925d0e52beaf89",
 		},
-		[]uint32{0x0bcb02fc, 0x00740074},
 	},
 	{
 		[]byte("contents"), 3,
@@ -70,7 +58,6 @@ var blocksTestData = []struct {
 			"e4432baa90819aaef51d2a7f8e148bf7e679610f3173752fabb4dcb2d0f418d3",
 			"44ad63f60af0f6db6fdde6d5186ef78176367df261fa06be3079b6c80c8adba4",
 		},
-		[]uint32{0x02780141, 0x02970148, 0x015d00e8},
 	},
 	{
 		[]byte("conconts"), 3,
@@ -79,7 +66,6 @@ var blocksTestData = []struct {
 			"1143da2bc54c495c4be31d3868785d39ffdfd56df5668f0645d8f14d47647952",
 			"44ad63f60af0f6db6fdde6d5186ef78176367df261fa06be3079b6c80c8adba4",
 		},
-		[]uint32{0x02780141, 0x02780141, 0x015d00e8},
 	},
 	{
 		[]byte("contenten"), 3,
@@ -88,14 +74,13 @@ var blocksTestData = []struct {
 			"e4432baa90819aaef51d2a7f8e148bf7e679610f3173752fabb4dcb2d0f418d3",
 			"e4432baa90819aaef51d2a7f8e148bf7e679610f3173752fabb4dcb2d0f418d3",
 		},
-		[]uint32{0x02780141, 0x02970148, 0x02970148},
 	},
 }

 func TestBlocks(t *testing.T) {
 	for testNo, test := range blocksTestData {
 		buf := bytes.NewBuffer(test.data)
-		blocks, err := Blocks(context.TODO(), buf, test.blocksize, -1, nil, true)
+		blocks, err := Blocks(context.TODO(), buf, test.blocksize, -1, nil)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -119,9 +104,6 @@ func TestBlocks(t *testing.T) {
 				if h := fmt.Sprintf("%x", blocks[i].Hash); h != test.hash[i] {
 					t.Errorf("%d/%d: Incorrect block hash %q != %q", testNo, i, h, test.hash[i])
 				}
-				if h := blocks[i].WeakHash; h != test.weakhash[i] {
-					t.Errorf("%d/%d: Incorrect block weakhash 0x%08x != 0x%08x", testNo, i, h, test.weakhash[i])
-				}

 				i++
 			}
@@ -129,85 +111,10 @@ func TestBlocks(t *testing.T) {
 	}
 }

-func TestAdler32Variants(t *testing.T) {
-	// Verify that the two adler32 functions give matching results for a few
-	// different blocks of data.
-
-	hf1 := origAdler32.New()
-	hf2 := rollingAdler32.New()
-
-	checkFn := func(data []byte) bool {
-		hf1.Write(data)
-		sum1 := hf1.Sum32()
-
-		hf2.Write(data)
-		sum2 := hf2.Sum32()
-
-		hf1.Reset()
-		hf2.Reset()
-
-		// Make sure whatever we use in Validate matches too resp. this
-		// tests gets adjusted if we ever switch the weak hash algo.
-		return sum1 == sum2 && Validate(data, nil, sum1)
-	}
-
-	// protocol block sized data
-	data := make([]byte, protocol.MinBlockSize)
-	for i := 0; i < 5; i++ {
-		rand.Read(data)
-		if !checkFn(data) {
-			t.Errorf("Hash mismatch on block sized data")
-		}
-	}
-
-	// random small blocks
-	if err := quick.Check(checkFn, nil); err != nil {
-		t.Error(err)
-	}
-
-	// rolling should have the same result as the individual blocks
-	// themselves.
-
-	windowSize := 128
-
-	hf3 := rollingAdler32.New()
-	hf3.Write(data[:windowSize])
-
-	for i := windowSize; i < len(data); i++ {
-		if i%windowSize == 0 {
-			// let the reference function catch up
-			window := data[i-windowSize : i]
-			hf1.Reset()
-			hf1.Write(window)
-			hf2.Reset()
-			hf2.Write(window)
-
-			// verify that they are in sync with the rolling function
-			sum1 := hf1.Sum32()
-			sum2 := hf2.Sum32()
-			sum3 := hf3.Sum32()
-			t.Logf("At i=%d, sum2=%08x, sum3=%08x", i, sum2, sum3)
-			if sum2 != sum3 {
-				t.Errorf("Mismatch after roll; i=%d, sum2=%08x, sum3=%08x", i, sum2, sum3)
-				break
-			}
-			if sum1 != sum3 {
-				t.Errorf("Mismatch after roll; i=%d, sum1=%08x, sum3=%08x", i, sum1, sum3)
-				break
-			}
-			if !Validate(window, nil, sum1) {
-				t.Errorf("Validation failure after roll; i=%d", i)
-			}
-		}
-		hf3.Roll(data[i])
-	}
-}
-
 func BenchmarkValidate(b *testing.B) {
 	type block struct {
-		data     []byte
-		hash     [sha256.Size]byte
-		weakhash uint32
+		data []byte
+		hash [sha256.Size]byte
 	}
 	var blocks []block
 	const blocksPerType = 100
@@ -220,16 +127,6 @@ func BenchmarkValidate(b *testing.B) {
 		b.data = make([]byte, 128<<10)
 		r.Read(b.data)
 		b.hash = sha256.Sum256(b.data)
-		b.weakhash = origAdler32.Checksum(b.data)
-		blocks = append(blocks, b)
-	}
-	// Blocks where the hash matches, but the weakhash doesn't.
-	for i := 0; i < blocksPerType; i++ {
-		var b block
-		b.data = make([]byte, 128<<10)
-		r.Read(b.data)
-		b.hash = sha256.Sum256(b.data)
-		b.weakhash = 1 // Zeros causes Validate to skip the weakhash.
 		blocks = append(blocks, b)
 	}

@@ -238,7 +135,7 @@ func BenchmarkValidate(b *testing.B) {

 	for i := 0; i < b.N; i++ {
 		for _, b := range blocks {
-			Validate(b.data, b.hash[:], b.weakhash)
+			Validate(b.data, b.hash[:])
 		}
 	}
 }
@@ -162,7 +162,7 @@ func TestVerify(t *testing.T) {
 	progress := newByteCounter()
 	defer progress.Close()

-	blocks, err := Blocks(context.TODO(), buf, blocksize, -1, progress, false)
+	blocks, err := Blocks(context.TODO(), buf, blocksize, -1, progress)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -640,7 +640,7 @@ func BenchmarkHashFile(b *testing.B) {
 	b.ResetTimer()

 	for i := 0; i < b.N; i++ {
-		if _, err := HashFile(context.TODO(), "", testFs, testdataName, protocol.MinBlockSize, nil, true); err != nil {
+		if _, err := HashFile(context.TODO(), "", testFs, testdataName, protocol.MinBlockSize, nil); err != nil {
 			b.Fatal(err)
 		}
 	}