persistlog: reverting incorrect GC, test changes

This commit reverts incorrect garbage collector and test changes
from previous commits. Specifically, boltdb's Delete funcion should
not have been called in a ForEach loop. This is undefined behavior.
The CLTV's were being treated as a relative timelock instead of an
absolute timelock. Finally, t.Parallel() should not have been used
in conjunction with boltdb.

Author: Crypt-iQ

persistlog/decayedlog.go

@@ -75,57 +75,35 @@ outer:
 						"is nil")
 				}
 
+				var expiredCltv [][]byte
 				sharedHashes.ForEach(func(k, v []byte) error {
 					// The CLTV value in question.
-					cltv := uint32(binary.BigEndian.Uint32(v[:4]))
-					// The last recorded block height that
-					// the garbage collector received.
-					lastHeight := uint32(binary.BigEndian.Uint32(v[4:8]))
-
-					if cltv == 0 {
-						// This CLTV just expired. We
-						// must delete it from the db.
-						err := sharedHashes.Delete(k)
-						if err != nil {
-							return err
-						}
-					} else if lastHeight != 0 && uint32(epoch.Height)-lastHeight > cltv {
-						// This CLTV just expired or
-						// expired in the past but the
-						// garbage collector was not
-						// running and therefore could
-						// not handle it. We delete it
-						// from the db now.
-						err := sharedHashes.Delete(k)
-						if err != nil {
-							return err
-						}
-					} else {
-						// The CLTV is still valid. We
-						// decrement the CLTV value and
-						// store the new CLTV value along
-						// with the current block height.
-						var scratch [8]byte
-
-						// Store decremented CLTV in
-						// scratch[:4]
-						cltv--
-						binary.BigEndian.PutUint32(scratch[:4], cltv)
-
-						// Store current blockheight in
-						// scratch[4:8]
-						binary.BigEndian.PutUint32(scratch[4:8], uint32(epoch.Height))
-
-						// Store <hash, CLTV + blockheight>
-						err := sharedHashes.Put(k, scratch[:])
-						if err != nil {
-							return err
-						}
+					cltv := uint32(binary.BigEndian.Uint32(v))
+
+					// Current blockheight
+					height := uint32(epoch.Height)
+
+					if cltv < height {
+						// This CLTV is expired. We must
+						// add it to an array which we'll
+						// loop over and delete every
+						// hash contained from the db.
+						expiredCltv = append(expiredCltv, k)
 					}
 
 					return nil
 				})
 
+				// Delete every item in the array. This must
+				// be done explicitly outside of the ForEach
+				// function for safety reasons.
+				for _, hash := range expiredCltv {
+					err := sharedHashes.Delete(hash)
+					if err != nil {
+						return err
+					}
+				}
+
 				return nil
 			})
 			if err != nil {
@@ -189,14 +167,14 @@ func (d *DecayedLog) Get(hash []byte) (uint32, error) {
 				"not retrieve CLTV value")
 		}
 
-		// Retrieve the bytes which represents the CLTV + blockheight.
+		// Retrieve the bytes which represents the CLTV
 		valueBytes := sharedHashes.Get(hash)
 		if valueBytes == nil {
 			return nil
 		}
 
 		// The first 4 bytes represent the CLTV, store it in value.
-		value = uint32(binary.BigEndian.Uint32(valueBytes[:4]))
+		value = uint32(binary.BigEndian.Uint32(valueBytes))
 
 		return nil
 	})
@@ -211,10 +189,10 @@ func (d *DecayedLog) Get(hash []byte) (uint32, error) {
 func (d *DecayedLog) Put(hash []byte, cltv uint32) error {
 	// The CLTV will be stored into scratch and then stored into the
 	// sharedHashBucket.
-	var scratch [8]byte
+	var scratch [4]byte
 
 	// Store value into scratch
-	binary.BigEndian.PutUint32(scratch[:4], cltv)
+	binary.BigEndian.PutUint32(scratch[:], cltv)
 
 	return d.db.Batch(func(tx *bolt.Tx) error {
 		sharedHashes, err := tx.CreateBucketIfNotExists(sharedHashBucket)

persistlog/decayedlog_test.go

@@ -14,7 +14,7 @@ import (
 )
 
 const (
-	cltv uint32 = 5
+	cltv uint32 = 100000
 )
 
 var (
@@ -121,10 +121,8 @@ func shutdown(d *DecayedLog) {
 
 // TestDecayedLogGarbageCollector tests the ability of the garbage collector
 // to delete expired cltv values every time a block is received. Expired cltv
-// values are cltv values that are <= current block height.
+// values are cltv values that are < current block height.
 func TestDecayedLogGarbageCollector(t *testing.T) {
-	t.Parallel()
-
 	d, notifier, hashedSecret, err := startup(true)
 	if err != nil {
 		t.Fatalf("Unable to start up DecayedLog: %v", err)
@@ -137,37 +135,52 @@ func TestDecayedLogGarbageCollector(t *testing.T) {
 		t.Fatalf("Unable to store in channeldb: %v", err)
 	}
 
+	// Wait for database write (GC is in a goroutine)
+	time.Sleep(500 * time.Millisecond)
+
 	// Send block notifications to garbage collector. The garbage collector
-	// should remove the entry we just added to sharedHashBucket as it will
-	// expire by the 6th block notification.
-	for i := 0; i < 6; i++ {
-		notifier.epochChan <- &chainntnfs.BlockEpoch{
-			Height: int32(100 + i),
-		}
+	// should remove the entry by block 100001.
+
+	// Send block 100000
+	notifier.epochChan <- &chainntnfs.BlockEpoch{
+		Height:	100000,
+	}
+
+	// Assert that hashedSecret is still in the sharedHashBucket
+	val, err := d.Get(hashedSecret[:])
+	if err != nil {
+		t.Fatalf("Get failed - received an error upon Get: %v", err)
+	}
+
+	if val != cltv {
+		t.Fatalf("GC incorrectly deleted CLTV")
+	}
+
+	// Send block 100001 (expiry block)
+	notifier.epochChan <- &chainntnfs.BlockEpoch{
+		Height:	100001,
 	}
 
 	// Wait for database write (GC is in a goroutine)
 	time.Sleep(500 * time.Millisecond)
 
 	// Assert that hashedSecret is not in the sharedHashBucket
-	val, err := d.Get(hashedSecret[:])
+	val, err = d.Get(hashedSecret[:])
 	if err != nil {
-		t.Fatalf("Delete failed - received an error upon Get: %v", err)
+		t.Fatalf("Get failed - received an error upon Get: %v", err)
 	}
 
 	if val != math.MaxUint32 {
-		t.Fatalf("cltv was not deleted")
+		t.Fatalf("CLTV was not deleted")
 	}
 }
 
 // TestDecayedLogPersistentGarbageCollector tests the persistence property of
-// the garbage collector. A block will be sent to the garbage collector, the
-// garbage collector will be shut down, and then a much later block will be sent
-// (past the expiry of our test CLTV) that causes the <sharedHash, cltv) key-pair
-// to be deleted.
+// the garbage collector. The garbage collector will be restarted immediately and
+// a block that expires the stored CLTV value will be sent to the ChainNotifier.
+// We test that this causes the <hashedSecret, CLTV> pair to be deleted even
+// on GC restarts.
 func TestDecayedLogPersistentGarbageCollector(t *testing.T) {
-	t.Parallel()
-
 	d, notifier, hashedSecret, err := startup(true)
 	if err != nil {
 		t.Fatalf("Unable to start up DecayedLog: %v", err)
@@ -179,11 +192,6 @@ func TestDecayedLogPersistentGarbageCollector(t *testing.T) {
 		t.Fatalf("Unable to store in channeldb: %v", err)
 	}
 
-	// Send a single block notification to the garbage collector.
-	notifier.epochChan <- &chainntnfs.BlockEpoch{
-		Height: int32(100),
-	}
-
 	// Wait for database write (GC is in a goroutine)
 	time.Sleep(500 * time.Millisecond)
 
@@ -195,9 +203,10 @@ func TestDecayedLogPersistentGarbageCollector(t *testing.T) {
 		t.Fatalf("Unable to restart DecayedLog: %v", err)
 	}
 
-	// Send a much later block notification to the garbage collector.
+	// Send a block notification to the garbage collector that expires
+	// the stored CLTV.
 	notifier.epochChan <- &chainntnfs.BlockEpoch{
-		Height: int32(150),
+		Height: int32(100001),
 	}
 
 	// Wait for database write (GC is in a goroutine)
@@ -214,12 +223,10 @@ func TestDecayedLogPersistentGarbageCollector(t *testing.T) {
 	}
 }
 
-// TestDecayedLogInsertionAndRetrieval inserts a cltv value into the nested
+// TestDecayedLogInsertionAndRetrieval inserts a cltv value into the
 // sharedHashBucket and then deletes it and finally asserts that we can no
 // longer retrieve it.
 func TestDecayedLogInsertionAndDeletion(t *testing.T) {
-	t.Parallel()
-
 	d, _, hashedSecret, err := startup(false)
 	if err != nil {
 		t.Fatalf("Unable to start up DecayedLog: %v", err)
@@ -256,8 +263,6 @@ func TestDecayedLogInsertionAndDeletion(t *testing.T) {
 // cltv value is indeed still stored in the sharedHashBucket. We then delete
 // the cltv value and check that it persists upon startup.
 func TestDecayedLogStartAndStop(t *testing.T) {
-	t.Parallel()
-
 	d, _, hashedSecret, err := startup(false)
 	if err != nil {
 		t.Fatalf("Unable to start up DecayedLog: %v", err)
@@ -322,8 +327,6 @@ func TestDecayedLogStartAndStop(t *testing.T) {
 // via the nested sharedHashBucket and finally asserts that the original stored
 // and retrieved cltv values are equal.
 func TestDecayedLogStorageAndRetrieval(t *testing.T) {
-	t.Parallel()
-
 	d, _, hashedSecret, err := startup(false)
 	if err != nil {
 		t.Fatalf("Unable to start up DecayedLog: %v", err)