eth/protocols/snap: sort trienode heal requests by path (ethereum#24779)

* sort snap trienode heal requests

* eth/protocols/snap: remove debug code

* eth/protocols/snap: simplify sort, generate pathsets later

* eth/protocols/snap: review concern

* eth/protocols/snap: renamings

* eth/protocols/snap: add comments in Merge

* eth/protocols/snap: remove variable 'last' in Merge

* eth/protocols/snap: fix lint flaws in test

Co-authored-by: Felix Lange <fjl@twurst.com>

eth/protocols/snap/sort_test.go

@@ -0,0 +1,109 @@
+// Copyright 2022 The go-ethereum Authors
+// This file is part of the go-ethereum library.
+//
+// The go-ethereum library is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Lesser General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// The go-ethereum library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU Lesser General Public License for more details.
+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package snap
+
+import (
+	"bytes"
+	"fmt"
+	"testing"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/trie"
+)
+
+func hexToNibbles(s string) []byte {
+	if len(s) >= 2 && s[0] == '0' && s[1] == 'x' {
+		s = s[2:]
+	}
+	var s2 []byte
+	for _, ch := range []byte(s) {
+		s2 = append(s2, '0')
+		s2 = append(s2, ch)
+	}
+	return common.Hex2Bytes(string(s2))
+}
+
+func TestRequestSorting(t *testing.T) {
+
+	//   - Path 0x9  -> {0x19}
+	//   - Path 0x99 -> {0x0099}
+	//   - Path 0x01234567890123456789012345678901012345678901234567890123456789019  -> {0x0123456789012345678901234567890101234567890123456789012345678901, 0x19}
+	//   - Path 0x012345678901234567890123456789010123456789012345678901234567890199 -> {0x0123456789012345678901234567890101234567890123456789012345678901, 0x0099}
+	var f = func(path string) (trie.SyncPath, TrieNodePathSet, common.Hash) {
+		data := hexToNibbles(path)
+		sp := trie.NewSyncPath(data)
+		tnps := TrieNodePathSet([][]byte(sp))
+		hash := common.Hash{}
+		return sp, tnps, hash
+	}
+	var (
+		hashes   []common.Hash
+		paths    []trie.SyncPath
+		pathsets []TrieNodePathSet
+	)
+	for _, x := range []string{
+		"0x9",
+		"0x012345678901234567890123456789010123456789012345678901234567890195",
+		"0x012345678901234567890123456789010123456789012345678901234567890197",
+		"0x012345678901234567890123456789010123456789012345678901234567890196",
+		"0x99",
+		"0x012345678901234567890123456789010123456789012345678901234567890199",
+		"0x01234567890123456789012345678901012345678901234567890123456789019",
+		"0x0123456789012345678901234567890101234567890123456789012345678901",
+		"0x01234567890123456789012345678901012345678901234567890123456789010",
+		"0x01234567890123456789012345678901012345678901234567890123456789011",
+	} {
+		sp, tnps, hash := f(x)
+		hashes = append(hashes, hash)
+		paths = append(paths, sp)
+		pathsets = append(pathsets, tnps)
+	}
+	_, paths, pathsets = sortByAccountPath(hashes, paths)
+	{
+		var b = new(bytes.Buffer)
+		for i := 0; i < len(paths); i++ {
+			fmt.Fprintf(b, "\n%d. paths %x", i, paths[i])
+		}
+		want := `
+0. paths [0099]
+1. paths [0123456789012345678901234567890101234567890123456789012345678901 00]
+2. paths [0123456789012345678901234567890101234567890123456789012345678901 0095]
+3. paths [0123456789012345678901234567890101234567890123456789012345678901 0096]
+4. paths [0123456789012345678901234567890101234567890123456789012345678901 0097]
+5. paths [0123456789012345678901234567890101234567890123456789012345678901 0099]
+6. paths [0123456789012345678901234567890101234567890123456789012345678901 10]
+7. paths [0123456789012345678901234567890101234567890123456789012345678901 11]
+8. paths [0123456789012345678901234567890101234567890123456789012345678901 19]
+9. paths [19]`
+		if have := b.String(); have != want {
+			t.Errorf("have:%v\nwant:%v\n", have, want)
+		}
+	}
+	{
+		var b = new(bytes.Buffer)
+		for i := 0; i < len(pathsets); i++ {
+			fmt.Fprintf(b, "\n%d. pathset %x", i, pathsets[i])
+		}
+		want := `
+0. pathset [0099]
+1. pathset [0123456789012345678901234567890101234567890123456789012345678901 00 0095 0096 0097 0099 10 11 19]
+2. pathset [19]`
+		if have := b.String(); have != want {
+			t.Errorf("have:%v\nwant:%v\n", have, want)
+		}
+	}
+}

eth/protocols/snap/sync.go

@@ -1331,12 +1331,13 @@ func (s *Syncer) assignTrienodeHealTasks(success chan *trienodeHealResponse, fai
 
 			hashes = append(hashes, hash)
 			paths = append(paths, pathset)
-			pathsets = append(pathsets, [][]byte(pathset)) // TODO(karalabe): group requests by account hash
 
 			if len(hashes) >= cap {
 				break
 			}
 		}
+		// Group requests by account hash
+		hashes, paths, pathsets = sortByAccountPath(hashes, paths)
 		req := &trienodeHealRequest{
 			peer:    idle,
 			id:      reqid,
@@ -2908,3 +2909,76 @@ func (s *capacitySort) Swap(i, j int) {
 	s.ids[i], s.ids[j] = s.ids[j], s.ids[i]
 	s.caps[i], s.caps[j] = s.caps[j], s.caps[i]
 }
+
+// healRequestSort implements the Sort interface, allowing sorting trienode
+// heal requests, which is a prerequisite for merging storage-requests.
+type healRequestSort struct {
+	hashes []common.Hash
+	paths  []trie.SyncPath
+}
+
+func (t *healRequestSort) Len() int {
+	return len(t.hashes)
+}
+
+func (t *healRequestSort) Less(i, j int) bool {
+	a := t.paths[i]
+	b := t.paths[j]
+	switch bytes.Compare(a[0], b[0]) {
+	case -1:
+		return true
+	case 1:
+		return false
+	}
+	// identical first part
+	if len(a) < len(b) {
+		return true
+	}
+	if len(b) < len(a) {
+		return false
+	}
+	if len(a) == 2 {
+		return bytes.Compare(a[1], b[1]) < 0
+	}
+	return false
+}
+
+func (t *healRequestSort) Swap(i, j int) {
+	t.hashes[i], t.hashes[j] = t.hashes[j], t.hashes[i]
+	t.paths[i], t.paths[j] = t.paths[j], t.paths[i]
+}
+
+// Merge merges the pathsets, so that several storage requests concerning the
+// same account are merged into one, to reduce bandwidth.
+// OBS: This operation is moot if t has not first been sorted.
+func (t *healRequestSort) Merge() []TrieNodePathSet {
+	var result []TrieNodePathSet
+	for _, path := range t.paths {
+		pathset := TrieNodePathSet([][]byte(path))
+		if len(path) == 1 {
+			// It's an account reference.
+			result = append(result, pathset)
+		} else {
+			// It's a storage reference.
+			end := len(result) - 1
+			if len(result) == 0 || !bytes.Equal(pathset[0], result[end][0]) {
+				// The account doesn't doesn't match last, create a new entry.
+				result = append(result, pathset)
+			} else {
+				// It's the same account as the previous one, add to the storage
+				// paths of that request.
+				result[end] = append(result[end], pathset[1])
+			}
+		}
+	}
+	return result
+}
+
+// sortByAccountPath takes hashes and paths, and sorts them. After that, it generates
+// the TrieNodePaths and merges paths which belongs to the same account path.
+func sortByAccountPath(hashes []common.Hash, paths []trie.SyncPath) ([]common.Hash, []trie.SyncPath, []TrieNodePathSet) {
+	n := &healRequestSort{hashes, paths}
+	sort.Sort(n)
+	pathsets := n.Merge()
+	return n.hashes, n.paths, pathsets
+}

trie/sync.go

@@ -71,9 +71,9 @@ type request struct {
 //   - Path 0x012345678901234567890123456789010123456789012345678901234567890199 -> {0x0123456789012345678901234567890101234567890123456789012345678901, 0x0099}
 type SyncPath [][]byte
 
-// newSyncPath converts an expanded trie path from nibble form into a compact
+// NewSyncPath converts an expanded trie path from nibble form into a compact
 // version that can be sent over the network.
-func newSyncPath(path []byte) SyncPath {
+func NewSyncPath(path []byte) SyncPath {
 	// If the hash is from the account trie, append a single item, if it
 	// is from the a storage trie, append a tuple. Note, the length 64 is
 	// clashing between account leaf and storage root. It's fine though
@@ -238,7 +238,7 @@ func (s *Sync) Missing(max int) (nodes []common.Hash, paths []SyncPath, codes []
 		hash := item.(common.Hash)
 		if req, ok := s.nodeReqs[hash]; ok {
 			nodeHashes = append(nodeHashes, hash)
-			nodePaths = append(nodePaths, newSyncPath(req.path))
+			nodePaths = append(nodePaths, NewSyncPath(req.path))
 		} else {
 			codeHashes = append(codeHashes, hash)
 		}