Restructure database to prevent double-slashes in keys, causing bugs in cursors (#1432)

* Add TestValidateAndInsertPruningPointWithSideBlocks

* Optimize infrastracture bucket paths

* Update infrastracture tests

* Refactor the consensus/database layer

* Remove utils/dbkeys

* Use consensus/database in consensus instead of infrastructure

* Fix a bug in dbBucketToDatabaseBucket and MakeBucket combination

Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>
Co-authored-by: stasatdaglabs <39559713+stasatdaglabs@users.noreply.github.com>

infrastructure/db/database/common_test.go

@@ -67,7 +67,7 @@ func populateDatabaseForTest(t *testing.T, db database.Database, testName string
 	// Prepare a list of key/value pairs
 	entries := make([]keyValuePair, 10)
 	for i := 0; i < 10; i++ {
-		key := database.MakeBucket().Key([]byte(fmt.Sprintf("key%d", i)))
+		key := database.MakeBucket(nil).Key([]byte(fmt.Sprintf("key%d", i)))
 		value := []byte("value")
 		entries[i] = keyValuePair{key: key, value: value}
 	}

infrastructure/db/database/cursor_test.go

@@ -15,7 +15,7 @@ import (
 )
 
 func prepareCursorForTest(t *testing.T, db database.Database, testName string) database.Cursor {
-	cursor, err := db.Cursor(database.MakeBucket())
+	cursor, err := db.Cursor(database.MakeBucket(nil))
 	if err != nil {
 		t.Fatalf("%s: Cursor unexpectedly "+
 			"failed: %s", testName, err)
@@ -241,7 +241,7 @@ func testCursorSeek(t *testing.T, db database.Database, testName string) {
 
 	// Seek to a value that doesn't exist and make sure that
 	// the returned error is ErrNotFound
-	err = cursor.Seek(database.MakeBucket().Key([]byte("doesn't exist")))
+	err = cursor.Seek(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err == nil {
 		t.Fatalf("%s: Seek unexpectedly "+
 			"succeeded", testName)
@@ -274,7 +274,7 @@ func testCursorCloseErrors(t *testing.T, db database.Database, testName string)
 		{
 			name: "Seek",
 			function: func() error {
-				return cursor.Seek(database.MakeBucket().Key([]byte{}))
+				return cursor.Seek(database.MakeBucket(nil).Key([]byte{}))
 			},
 		},
 		{

infrastructure/db/database/database_test.go

@@ -18,7 +18,7 @@ func TestDatabasePut(t *testing.T) {
 
 func testDatabasePut(t *testing.T, db database.Database, testName string) {
 	// Put value1 into the database
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value1 := []byte("value1")
 	err := db.Put(key, value1)
 	if err != nil {
@@ -65,7 +65,7 @@ func TestDatabaseGet(t *testing.T) {
 
 func testDatabaseGet(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err := db.Put(key, value)
 	if err != nil {
@@ -87,7 +87,7 @@ func testDatabaseGet(t *testing.T, db database.Database, testName string) {
 
 	// Try getting a non-existent value and make sure
 	// the returned error is ErrNotFound
-	_, err = db.Get(database.MakeBucket().Key([]byte("doesn't exist")))
+	_, err = db.Get(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err == nil {
 		t.Fatalf("%s: Get "+
 			"unexpectedly succeeded", testName)
@@ -104,7 +104,7 @@ func TestDatabaseHas(t *testing.T) {
 
 func testDatabaseHas(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err := db.Put(key, value)
 	if err != nil {
@@ -124,7 +124,7 @@ func testDatabaseHas(t *testing.T, db database.Database, testName string) {
 	}
 
 	// Make sure that Has returns false for a non-existent value
-	exists, err = db.Has(database.MakeBucket().Key([]byte("doesn't exist")))
+	exists, err = db.Has(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err != nil {
 		t.Fatalf("%s: Has "+
 			"unexpectedly failed: %s", testName, err)
@@ -141,7 +141,7 @@ func TestDatabaseDelete(t *testing.T) {
 
 func testDatabaseDelete(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err := db.Put(key, value)
 	if err != nil {

infrastructure/db/database/keys.go

@@ -1,11 +1,10 @@
 package database
 
 import (
-	"bytes"
 	"encoding/hex"
 )
 
-var bucketSeparator = []byte("/")
+var bucketSeparator = byte('/')
 
 // Key is a helper type meant to combine prefix
 // and suffix into a single database key.
@@ -48,23 +47,25 @@ func newKey(bucket *Bucket, suffix []byte) *Key {
 // and sub-buckets that can be used to create database
 // keys and prefix-based cursors.
 type Bucket struct {
-	path [][]byte
+	path []byte
 }
 
 // MakeBucket creates a new Bucket using the given path
 // of buckets.
-func MakeBucket(path ...[]byte) *Bucket {
+func MakeBucket(path []byte) *Bucket {
+	if len(path) > 0 && path[len(path)-1] != bucketSeparator {
+		path = append(path, bucketSeparator)
+	}
 	return &Bucket{path: path}
 }
 
 // Bucket returns the sub-bucket of the current bucket
 // defined by bucketBytes.
 func (b *Bucket) Bucket(bucketBytes []byte) *Bucket {
-	newPath := make([][]byte, len(b.path)+1)
-	copy(newPath, b.path)
-	copy(newPath[len(b.path):], [][]byte{bucketBytes})
-
-	return MakeBucket(newPath...)
+	newPath := make([]byte, 0, len(b.path)+len(bucketBytes)+1) // +1 for the separator in MakeBucket
+	newPath = append(newPath, b.path...)
+	newPath = append(newPath, bucketBytes...)
+	return MakeBucket(newPath)
 }
 
 // Key returns a key in the current bucket with the
@@ -75,11 +76,5 @@ func (b *Bucket) Key(suffix []byte) *Key {
 
 // Path returns the full path of the current bucket.
 func (b *Bucket) Path() []byte {
-	bucketPath := bytes.Join(b.path, bucketSeparator)
-
-	bucketPathWithFinalSeparator := make([]byte, len(bucketPath)+len(bucketSeparator))
-	copy(bucketPathWithFinalSeparator, bucketPath)
-	copy(bucketPathWithFinalSeparator[len(bucketPath):], bucketSeparator)
-
-	return bucketPathWithFinalSeparator
+	return b.path
 }

infrastructure/db/database/keys_test.go

@@ -6,6 +6,10 @@ import (
 	"testing"
 )
 
+func makeBucketJoin(path ...[]byte) *Bucket {
+	return MakeBucket(bytes.Join(path, []byte{bucketSeparator}))
+}
+
 func TestBucketPath(t *testing.T) {
 	tests := []struct {
 		bucketByteSlices [][]byte
@@ -23,14 +27,14 @@ func TestBucketPath(t *testing.T) {
 
 	for _, test := range tests {
 		// Build a result using the MakeBucket function alone
-		resultKey := MakeBucket(test.bucketByteSlices...).Path()
+		resultKey := makeBucketJoin(test.bucketByteSlices...).Path()
 		if !reflect.DeepEqual(resultKey, test.expectedPath) {
 			t.Errorf("TestBucketPath: got wrong path using MakeBucket. "+
 				"Want: %s, got: %s", string(test.expectedPath), string(resultKey))
 		}
 
 		// Build a result using sub-Bucket calls
-		bucket := MakeBucket()
+		bucket := MakeBucket(nil)
 		for _, bucketBytes := range test.bucketByteSlices {
 			bucket = bucket.Bucket(bucketBytes)
 		}
@@ -63,14 +67,14 @@ func TestBucketKey(t *testing.T) {
 			key:              []byte("test"),
 			expectedKeyBytes: []byte("hello/world/test"),
 			expectedKey: &Key{
-				bucket: MakeBucket([]byte("hello"), []byte("world")),
+				bucket: makeBucketJoin([]byte("hello"), []byte("world")),
 				suffix: []byte("test"),
 			},
 		},
 	}
 
 	for _, test := range tests {
-		resultKey := MakeBucket(test.bucketByteSlices...).Key(test.key)
+		resultKey := makeBucketJoin(test.bucketByteSlices...).Key(test.key)
 		if !reflect.DeepEqual(resultKey, test.expectedKey) {
 			t.Errorf("TestBucketKey: got wrong key. Want: %s, got: %s",
 				test.expectedKeyBytes, resultKey)

infrastructure/db/database/ldb/cursor_test.go

@@ -95,7 +95,7 @@ func TestCursorSanity(t *testing.T) {
 	validateCurrentCursorKeyAndValue(t, "TestCursorSanity", cursor, expectedKey, expectedValue)
 
 	// Seek to a non-existant key
-	err = cursor.Seek(database.MakeBucket().Key([]byte("doesn't exist")))
+	err = cursor.Seek(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err == nil {
 		t.Fatalf("TestCursorSanity: Seek " +
 			"unexpectedly succeeded")
@@ -155,7 +155,7 @@ func TestCursorCloseErrors(t *testing.T) {
 		{
 			name: "Seek",
 			function: func(cursor database.Cursor) error {
-				return cursor.Seek(database.MakeBucket().Key([]byte{}))
+				return cursor.Seek(database.MakeBucket(nil).Key([]byte{}))
 			},
 		},
 		{
@@ -186,7 +186,7 @@ func TestCursorCloseErrors(t *testing.T) {
 			defer teardownFunc()
 
 			// Open a new cursor
-			cursor, err := ldb.Cursor(database.MakeBucket())
+			cursor, err := ldb.Cursor(database.MakeBucket(nil))
 			if err != nil {
 				t.Fatalf("TestCursorCloseErrors: ldb.Cursor "+
 					"unexpectedly failed: %s", err)

infrastructure/db/database/ldb/leveldb_test.go

@@ -35,7 +35,7 @@ func TestLevelDBSanity(t *testing.T) {
 	defer teardownFunc()
 
 	// Put something into the db
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	putData := []byte("Hello world!")
 	err := ldb.Put(key, putData)
 	if err != nil {
@@ -71,7 +71,7 @@ func TestLevelDBTransactionSanity(t *testing.T) {
 	}
 
 	// Put something into the transaction
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	putData := []byte("Hello world!")
 	err = tx.Put(key, putData)
 	if err != nil {
@@ -115,7 +115,7 @@ func TestLevelDBTransactionSanity(t *testing.T) {
 
 	// Case 2. Write directly to the DB and then read from a tx
 	// Put something into the db
-	key = database.MakeBucket().Key([]byte("key2"))
+	key = database.MakeBucket(nil).Key([]byte("key2"))
 	putData = []byte("Goodbye world!")
 	err = ldb.Put(key, putData)
 	if err != nil {

infrastructure/db/database/ldb/transaction_test.go

@@ -20,14 +20,14 @@ func TestTransactionCloseErrors(t *testing.T) {
 		{
 			name: "Put",
 			function: func(dbTx *LevelDBTransaction) error {
-				return dbTx.Put(database.MakeBucket().Key([]byte("key")), []byte("value"))
+				return dbTx.Put(database.MakeBucket(nil).Key([]byte("key")), []byte("value"))
 			},
 			shouldReturnError: true,
 		},
 		{
 			name: "Get",
 			function: func(dbTx *LevelDBTransaction) error {
-				_, err := dbTx.Get(database.MakeBucket().Key([]byte("key")))
+				_, err := dbTx.Get(database.MakeBucket(nil).Key([]byte("key")))
 				return err
 			},
 			shouldReturnError: true,
@@ -35,7 +35,7 @@ func TestTransactionCloseErrors(t *testing.T) {
 		{
 			name: "Has",
 			function: func(dbTx *LevelDBTransaction) error {
-				_, err := dbTx.Has(database.MakeBucket().Key([]byte("key")))
+				_, err := dbTx.Has(database.MakeBucket(nil).Key([]byte("key")))
 				return err
 			},
 			shouldReturnError: true,
@@ -43,7 +43,7 @@ func TestTransactionCloseErrors(t *testing.T) {
 		{
 			name: "Delete",
 			function: func(dbTx *LevelDBTransaction) error {
-				return dbTx.Delete(database.MakeBucket().Key([]byte("key")))
+				return dbTx.Delete(database.MakeBucket(nil).Key([]byte("key")))
 			},
 			shouldReturnError: true,
 		},

infrastructure/db/database/transaction_test.go

@@ -33,7 +33,7 @@ func testTransactionPut(t *testing.T, db database.Database, testName string) {
 	}()
 
 	// Put value1 into the transaction
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value1 := []byte("value1")
 	err = dbTx.Put(key, value1)
 	if err != nil {
@@ -75,7 +75,7 @@ func TestTransactionGet(t *testing.T) {
 
 func testTransactionGet(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key1 := database.MakeBucket().Key([]byte("key1"))
+	key1 := database.MakeBucket(nil).Key([]byte("key1"))
 	value1 := []byte("value1")
 	err := db.Put(key1, value1)
 	if err != nil {
@@ -111,7 +111,7 @@ func testTransactionGet(t *testing.T, db database.Database, testName string) {
 
 	// Try getting a non-existent value and make sure
 	// the returned error is ErrNotFound
-	_, err = dbTx.Get(database.MakeBucket().Key([]byte("doesn't exist")))
+	_, err = dbTx.Get(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err == nil {
 		t.Fatalf("%s: Get "+
 			"unexpectedly succeeded", testName)
@@ -122,7 +122,7 @@ func testTransactionGet(t *testing.T, db database.Database, testName string) {
 	}
 
 	// Put a new value into the database outside of the transaction
-	key2 := database.MakeBucket().Key([]byte("key2"))
+	key2 := database.MakeBucket(nil).Key([]byte("key2"))
 	value2 := []byte("value2")
 	err = db.Put(key2, value2)
 	if err != nil {
@@ -141,7 +141,7 @@ func testTransactionGet(t *testing.T, db database.Database, testName string) {
 	}
 
 	// Put a new value into the transaction
-	key3 := database.MakeBucket().Key([]byte("key3"))
+	key3 := database.MakeBucket(nil).Key([]byte("key3"))
 	value3 := []byte("value3")
 	err = dbTx.Put(key3, value3)
 	if err != nil {
@@ -167,7 +167,7 @@ func TestTransactionHas(t *testing.T) {
 
 func testTransactionHas(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key1 := database.MakeBucket().Key([]byte("key1"))
+	key1 := database.MakeBucket(nil).Key([]byte("key1"))
 	value1 := []byte("value1")
 	err := db.Put(key1, value1)
 	if err != nil {
@@ -201,7 +201,7 @@ func testTransactionHas(t *testing.T, db database.Database, testName string) {
 	}
 
 	// Make sure that Has returns false for a non-existent value
-	exists, err = dbTx.Has(database.MakeBucket().Key([]byte("doesn't exist")))
+	exists, err = dbTx.Has(database.MakeBucket(nil).Key([]byte("doesn't exist")))
 	if err != nil {
 		t.Fatalf("%s: Has "+
 			"unexpectedly failed: %s", testName, err)
@@ -212,7 +212,7 @@ func testTransactionHas(t *testing.T, db database.Database, testName string) {
 	}
 
 	// Put a new value into the database outside of the transaction
-	key2 := database.MakeBucket().Key([]byte("key2"))
+	key2 := database.MakeBucket(nil).Key([]byte("key2"))
 	value2 := []byte("value2")
 	err = db.Put(key2, value2)
 	if err != nil {
@@ -238,7 +238,7 @@ func TestTransactionDelete(t *testing.T) {
 
 func testTransactionDelete(t *testing.T, db database.Database, testName string) {
 	// Put a value into the database
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err := db.Put(key, value)
 	if err != nil {
@@ -327,7 +327,7 @@ func testTransactionCommit(t *testing.T, db database.Database, testName string)
 	}()
 
 	// Put a value into the transaction
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err = dbTx.Put(key, value)
 	if err != nil {
@@ -388,7 +388,7 @@ func testTransactionRollback(t *testing.T, db database.Database, testName string
 	}()
 
 	// Put a value into the transaction
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err = dbTx.Put(key, value)
 	if err != nil {
@@ -448,7 +448,7 @@ func testTransactionRollbackUnlessClosed(t *testing.T, db database.Database, tes
 	}()
 
 	// Put a value into the transaction
-	key := database.MakeBucket().Key([]byte("key"))
+	key := database.MakeBucket(nil).Key([]byte("key"))
 	value := []byte("value")
 	err = dbTx.Put(key, value)
 	if err != nil {