Skip to content

Commit 73f3382

Browse files
author
Larry Ruane
committed
Change hash representation from slice to 32-byte array
This change is non-functional, just a clean-up, reducing tech debt. This codebase manipulates many 32-byte hashes, such as block hashes, transaction IDs, and merkle roots. These should always have been represented as fixed-size 32-byte arrays rather than variable-length slices. This prevents bugs (a slice that's supposed to be of length 32 bytes could be a different length) and makes assigning, comparing, function argument passing, and function return value passing simpler and less fragile. The new hash type, hash32.T, which is defined as [32]byte (32-byte array of bytes), can be treated like any simple variable, such as an integer. A slice, in contrast, is like a string; it's really a structure that includes a length, capacity, and pointer to separately-allocated memory to hold the elements of the slice. The only point of friction is that protobuf doesn't support fixed-sized arrays, only (in effect) slices. So conversion must be done in each direction.
1 parent f17648d commit 73f3382

21 files changed

Lines changed: 297 additions & 223 deletions

‎common/cache.go‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"path/filepath"
1515
"sync"
1616

17+
"github.com/zcash/lightwalletd/hash32"
1718
"github.com/zcash/lightwalletd/walletrpc"
1819
"google.golang.org/protobuf/proto"
1920
)
@@ -22,10 +23,10 @@ import (
2223
type BlockCache struct {
2324
lengthsName, blocksName string // pathnames
2425
lengthsFile, blocksFile *os.File
25-
starts []int64 // Starting offset of each block within blocksFile
26-
firstBlock int // height of the first block in the cache (usually Sapling activation)
27-
nextBlock int // height of the first block not in the cache
28-
latestHash []byte // hash of the most recent (highest height) block, for detecting reorgs.
26+
starts []int64 // Starting offset of each block within blocksFile
27+
firstBlock int // height of the first block in the cache (usually Sapling activation)
28+
nextBlock int // height of the first block not in the cache
29+
latestHash hash32.T // hash of the most recent (highest height) block, for detecting reorgs.
2930
mutex sync.RWMutex
3031
}
3132

@@ -44,18 +45,18 @@ func (c *BlockCache) GetFirstHeight() int {
4445
}
4546

4647
// GetLatestHash returns the hash (block ID) of the most recent (highest) known block.
47-
func (c *BlockCache) GetLatestHash() []byte {
48+
func (c *BlockCache) GetLatestHash() hash32.T {
4849
c.mutex.RLock()
4950
defer c.mutex.RUnlock()
5051
return c.latestHash
5152
}
5253

5354
// HashMatch indicates if the given prev-hash matches the most recent block's hash
5455
// so reorgs can be detected.
55-
func (c *BlockCache) HashMatch(prevhash []byte) bool {
56+
func (c *BlockCache) HashMatch(prevhash hash32.T) bool {
5657
c.mutex.RLock()
5758
defer c.mutex.RUnlock()
58-
return c.latestHash == nil || bytes.Equal(c.latestHash, prevhash)
59+
return c.latestHash == hash32.Nil || c.latestHash == prevhash
5960
}
6061

6162
// Make the block at the given height the lowest height that we don't have.
@@ -167,7 +168,7 @@ func (c *BlockCache) readBlock(height int) *walletrpc.CompactBlock {
167168

168169
// Caller should hold c.mutex.Lock().
169170
func (c *BlockCache) setLatestHash() {
170-
c.latestHash = nil
171+
c.latestHash = hash32.Nil
171172
// There is at least one block; get the last block's hash
172173
if c.nextBlock > c.firstBlock {
173174
// At least one block remains; get the last block's hash
@@ -176,8 +177,7 @@ func (c *BlockCache) setLatestHash() {
176177
c.recoverFromCorruption(c.nextBlock - 10000)
177178
return
178179
}
179-
c.latestHash = make([]byte, len(block.Hash))
180-
copy(c.latestHash, block.Hash)
180+
c.latestHash = hash32.T(block.Hash)
181181
}
182182
}
183183

@@ -312,10 +312,7 @@ func (c *BlockCache) Add(height int, block *walletrpc.CompactBlock) error {
312312
offset := c.starts[len(c.starts)-1]
313313
c.starts = append(c.starts, offset+int64(len(data)+8))
314314

315-
if c.latestHash == nil {
316-
c.latestHash = make([]byte, len(block.Hash))
317-
}
318-
copy(c.latestHash, block.Hash)
315+
c.latestHash = hash32.T(block.Hash)
319316
c.nextBlock++
320317
// Invariant: m[firstBlock..nextBlock) are valid.
321318
return nil

‎common/cache_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"os"
1010
"testing"
1111

12+
"github.com/zcash/lightwalletd/hash32"
1213
"github.com/zcash/lightwalletd/parser"
1314
"github.com/zcash/lightwalletd/walletrpc"
1415
)
@@ -84,7 +85,7 @@ func TestCache(t *testing.T) {
8485

8586
// Reorg to before the first block moves back to only the first block
8687
cache.Reorg(289459)
87-
if cache.latestHash != nil {
88+
if cache.latestHash != hash32.Nil {
8889
t.Fatal("unexpected latestHash, should be nil")
8990
}
9091
if cache.nextBlock != 289460 {

‎common/common.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
package common
66

77
import (
8-
"bytes"
98
"encoding/hex"
109
"encoding/json"
1110
"errors"
@@ -15,6 +14,7 @@ import (
1514
"time"
1615

1716
"github.com/sirupsen/logrus"
17+
"github.com/zcash/lightwalletd/hash32"
1818
"github.com/zcash/lightwalletd/parser"
1919
"github.com/zcash/lightwalletd/walletrpc"
2020
"google.golang.org/grpc/codes"
@@ -373,12 +373,12 @@ func getBlockFromRPC(height int) (*walletrpc.CompactBlock, error) {
373373
return nil, errors.New("received unexpected height block")
374374
}
375375
for i, t := range block.Transactions() {
376-
txidBigEndian, err := hex.DecodeString(block1.Tx[i])
376+
txidBigEndian, err := hash32.Decode(block1.Tx[i])
377377
if err != nil {
378378
return nil, fmt.Errorf("error decoding getblock txid: %w", err)
379379
}
380380
// convert from big-endian
381-
t.SetTxID(parser.Reverse(txidBigEndian))
381+
t.SetTxID(hash32.Reverse(txidBigEndian))
382382
}
383383
r := block.ToCompact()
384384
r.ChainMetadata.SaplingCommitmentTreeSize = block1.Trees.Sapling.Size
@@ -430,13 +430,13 @@ func BlockIngestor(c *BlockCache, rep int) {
430430
if err != nil {
431431
Log.Fatal("bad getbestblockhash return:", err, result)
432432
}
433-
lastBestBlockHashBE, err := hex.DecodeString(hashHex)
433+
lastBestBlockHashBE, err := hash32.Decode(hashHex)
434434
if err != nil {
435435
Log.Fatal("error decoding getbestblockhash", err, hashHex)
436436
}
437437

438438
height := c.GetNextHeight()
439-
if bytes.Equal(lastBestBlockHashBE, parser.Reverse(c.GetLatestHash())) {
439+
if lastBestBlockHashBE == hash32.Reverse(c.GetLatestHash()) {
440440
// Synced
441441
c.Sync()
442442
if lastHeightLogged != height-1 {
@@ -454,14 +454,14 @@ func BlockIngestor(c *BlockCache, rep int) {
454454
Time.Sleep(8 * time.Second)
455455
continue
456456
}
457-
if block != nil && c.HashMatch(block.PrevHash) {
457+
if block != nil && c.HashMatch(hash32.T(block.PrevHash)) {
458458
if err = c.Add(height, block); err != nil {
459459
Log.Fatal("Cache add failed:", err)
460460
}
461461
// Don't log these too often.
462462
if DarksideEnabled || Time.Now().Sub(lastLog).Seconds() >= 4 {
463463
lastLog = Time.Now()
464-
Log.Info("Adding block to cache ", height, " ", displayHash(block.Hash))
464+
Log.Info("Adding block to cache ", height, " ", displayHash(hash32.T(block.Hash)))
465465
}
466466
continue
467467
}
@@ -567,6 +567,6 @@ func ParseRawTransaction(message json.RawMessage) (*walletrpc.RawTransaction, er
567567
}, nil
568568
}
569569

570-
func displayHash(hash []byte) string {
571-
return hex.EncodeToString(parser.Reverse(hash))
570+
func displayHash(hash hash32.T) string {
571+
return hash32.Encode(hash32.Reverse(hash))
572572
}

‎common/common_test.go‎

Lines changed: 35 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,13 @@ var (
3232
testcache *BlockCache
3333
)
3434

35+
const (
36+
testTxid = "1234000000000000000000000000000000000000000000000000000000000000"
37+
testBlockid40 = "0000000000000000000000000000000000000000000000000000000000380640"
38+
testBlockid41 = "0000000000000000000000000000000000000000000000000000000000380641"
39+
testBlockid42 = "0000000000000000000000000000000000000000000000000000000000380642"
40+
)
41+
3542
// TestMain does common setup that's shared across multiple tests
3643
func TestMain(m *testing.M) {
3744
output, err := os.OpenFile("test-log", os.O_CREATE|os.O_TRUNC|os.O_WRONLY, 0644)
@@ -196,36 +203,36 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
196203
case 1:
197204
checkSleepMethod(0, 0, "getbestblockhash", method)
198205
// This hash doesn't matter, won't match anything
199-
r, _ := json.Marshal("010101")
206+
r, _ := json.Marshal(strings.Repeat("01", 32))
200207
return r, nil
201208
case 2:
202209
checkSleepMethod(0, 0, "getblock", method)
203210
if arg != "380640" {
204211
testT.Fatal("incorrect height requested")
205212
}
206213
// height 380640
207-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380640\"}"), nil
214+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid40 + "\"}"), nil
208215
case 3:
209216
checkSleepMethod(0, 0, "getblock", method)
210-
if arg != "0000380640" {
217+
if arg != testBlockid40 {
211218
testT.Fatal("incorrect hash requested")
212219
}
213220
return blocks[0], nil
214221
case 4:
215222
checkSleepMethod(0, 0, "getbestblockhash", method)
216223
// This hash doesn't matter, won't match anything
217-
r, _ := json.Marshal("010101")
224+
r, _ := json.Marshal(strings.Repeat("01", 32))
218225
return r, nil
219226
case 5:
220227
checkSleepMethod(0, 0, "getblock", method)
221228
if arg != "380641" {
222229
testT.Fatal("incorrect height requested")
223230
}
224231
// height 380641
225-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380641\"}"), nil
232+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid41 + "\"}"), nil
226233
case 6:
227234
checkSleepMethod(0, 0, "getblock", method)
228-
if arg != "0000380641" {
235+
if arg != testBlockid41 {
229236
testT.Fatal("incorrect hash requested")
230237
}
231238
return blocks[1], nil
@@ -244,18 +251,18 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
244251
case 9:
245252
// Simulate new block (any non-matching hash will do)
246253
checkSleepMethod(2, 4, "getbestblockhash", method)
247-
r, _ := json.Marshal("aabb")
254+
r, _ := json.Marshal(strings.Repeat("ab", 32))
248255
return r, nil
249256
case 10:
250257
checkSleepMethod(2, 4, "getblock", method)
251258
if arg != "380642" {
252259
testT.Fatal("incorrect height requested")
253260
}
254261
// height 380642
255-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380642\"}"), nil
262+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid42 + "\"}"), nil
256263
case 11:
257264
checkSleepMethod(2, 4, "getblock", method)
258-
if arg != "0000380642" {
265+
if arg != testBlockid42 {
259266
testT.Fatal("incorrect hash requested")
260267
}
261268
return blocks[2], nil
@@ -270,7 +277,7 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
270277
// simulate a 1-block reorg, new version (replacement) of 380642
271278
checkSleepMethod(3, 6, "getbestblockhash", method)
272279
// hash doesn't matter, just something that doesn't match
273-
r, _ := json.Marshal("4545")
280+
r, _ := json.Marshal(strings.Repeat("45", 32))
274281
return r, nil
275282
case 14:
276283
// It thinks there may simply be a new block, but we'll say
@@ -284,7 +291,7 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
284291
// It will re-ask the best hash (let's make no change)
285292
checkSleepMethod(3, 6, "getbestblockhash", method)
286293
// hash doesn't matter, just something that doesn't match
287-
r, _ := json.Marshal("4545")
294+
r, _ := json.Marshal(strings.Repeat("45", 32))
288295
return r, nil
289296
case 16:
290297
// It should have backed up one block
@@ -293,10 +300,10 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
293300
testT.Fatal("incorrect height requested")
294301
}
295302
// height 380642
296-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380642\"}"), nil
303+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid42 + "\"}"), nil
297304
case 17:
298305
checkSleepMethod(3, 6, "getblock", method)
299-
if arg != "0000380642" {
306+
if arg != testBlockid42 {
300307
testT.Fatal("incorrect height requested")
301308
}
302309
return blocks[2], nil
@@ -305,7 +312,7 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
305312
// we'll make it back up 2 blocks (rather than one)
306313
checkSleepMethod(3, 6, "getbestblockhash", method)
307314
// hash doesn't matter, just something that doesn't match
308-
r, _ := json.Marshal("5656")
315+
r, _ := json.Marshal(strings.Repeat("56", 32))
309316
return r, nil
310317
case 19:
311318
// It thinks there may simply be a new block, but we'll say
@@ -318,7 +325,7 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
318325
case 20:
319326
checkSleepMethod(3, 6, "getbestblockhash", method)
320327
// hash doesn't matter, just something that doesn't match
321-
r, _ := json.Marshal("5656")
328+
r, _ := json.Marshal(strings.Repeat("56", 32))
322329
return r, nil
323330
case 21:
324331
// Like case 13, it should have backed up one block, but
@@ -331,18 +338,18 @@ func blockIngestorStub(method string, params []json.RawMessage) (json.RawMessage
331338
case 22:
332339
checkSleepMethod(3, 6, "getbestblockhash", method)
333340
// hash doesn't matter, just something that doesn't match
334-
r, _ := json.Marshal("5656")
341+
r, _ := json.Marshal(strings.Repeat("56", 32))
335342
return r, nil
336343
case 23:
337344
// It should have backed up one more
338345
checkSleepMethod(3, 6, "getblock", method)
339346
if arg != "380641" {
340347
testT.Fatal("incorrect height requested")
341348
}
342-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380641\"}"), nil
349+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid41 + "\"}"), nil
343350
case 24:
344351
checkSleepMethod(3, 6, "getblock", method)
345-
if arg != "0000380641" {
352+
if arg != testBlockid41 {
346353
testT.Fatal("incorrect height requested")
347354
}
348355
return blocks[1], nil
@@ -385,17 +392,17 @@ func getblockStub(method string, params []json.RawMessage) (json.RawMessage, err
385392
step++
386393
switch step {
387394
case 1:
388-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380640\"}"), nil
395+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid40 + "\"}"), nil
389396
case 2:
390-
if arg != "0000380640" {
397+
if arg != testBlockid40 {
391398
testT.Error("unexpected hash")
392399
}
393400
// Sunny-day
394401
return blocks[0], nil
395402
case 3:
396-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380641\"}"), nil
403+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid41 + "\"}"), nil
397404
case 4:
398-
if arg != "0000380641" {
405+
if arg != testBlockid41 {
399406
testT.Error("unexpected hash")
400407
}
401408
// Sunny-day
@@ -475,9 +482,9 @@ func getblockStubReverse(method string, params []json.RawMessage) (json.RawMessa
475482
testT.Error("unexpected height")
476483
}
477484
// Sunny-day
478-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380642\"}"), nil
485+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid42 + "\"}"), nil
479486
case 2:
480-
if arg != "0000380642" {
487+
if arg != testBlockid42 {
481488
testT.Error("unexpected hash")
482489
}
483490
return blocks[2], nil
@@ -486,9 +493,9 @@ func getblockStubReverse(method string, params []json.RawMessage) (json.RawMessa
486493
testT.Error("unexpected height")
487494
}
488495
// Sunny-day
489-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380641\"}"), nil
496+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid41 + "\"}"), nil
490497
case 4:
491-
if arg != "0000380641" {
498+
if arg != testBlockid41 {
492499
testT.Error("unexpected hash")
493500
}
494501
return blocks[1], nil
@@ -497,9 +504,9 @@ func getblockStubReverse(method string, params []json.RawMessage) (json.RawMessa
497504
testT.Error("unexpected height")
498505
}
499506
// Sunny-day
500-
return []byte("{\"Tx\": [\"00\"], \"Hash\": \"0000380640\"}"), nil
507+
return []byte("{\"Tx\": [\"" + testTxid + "\"], \"Hash\": \"" + testBlockid40 + "\"}"), nil
501508
case 6:
502-
if arg != "0000380640" {
509+
if arg != testBlockid40 {
503510
testT.Error("unexpected hash")
504511
}
505512
return blocks[0], nil

0 commit comments

Comments
 (0)