Merge pull request #576 from lavanet/CNS-467-panic-prune-or-document

CNS-467 panic - prune or document

common/fixation_entry_index.go

@@ -30,21 +30,21 @@ func (fs *FixationStore) getEntryIndexStore(ctx sdk.Context) *prefix.Store {
 }
 
 // setEntryIndex stores an Entry index in the store
-func (fs FixationStore) setEntryIndex(ctx sdk.Context, safeIndex string, live bool) {
+func (fs FixationStore) setEntryIndex(ctx sdk.Context, safeIndex types.SafeIndex, live bool) {
 	types.AssertSanitizedIndex(safeIndex, fs.prefix)
 	store := fs.getEntryIndexStore(ctx)
 	value := types.EntryIndexLive
 	if !live {
 		value = types.EntryIndexDead
 	}
-	store.Set(types.KeyPrefix(safeIndex), value)
+	store.Set(types.KeyPrefix(string(safeIndex)), value)
 }
 
 // removeEntryIndex removes an Entry index from the store
-func (fs FixationStore) removeEntryIndex(ctx sdk.Context, safeIndex string) {
+func (fs FixationStore) removeEntryIndex(ctx sdk.Context, safeIndex types.SafeIndex) {
 	types.AssertSanitizedIndex(safeIndex, fs.prefix)
 	store := fs.getEntryIndexStore(ctx)
-	store.Delete(types.KeyPrefix(safeIndex))
+	store.Delete(types.KeyPrefix(string(safeIndex)))
 }
 
 // AllEntryIndicesFilter returns all Entry indices with a given prefix and filtered
@@ -59,7 +59,7 @@ func (fs FixationStore) AllEntryIndicesFilter(ctx sdk.Context, prefix string, fi
 	indexList := []string{}
 	for ; iterator.Valid(); iterator.Next() {
 		key, value := iterator.Key(), iterator.Value()
-		safeIndex := string(key)
+		safeIndex := types.SafeIndex(key)
 		types.AssertSanitizedIndex(safeIndex, fs.prefix)
 		if filter == nil || filter(key, value) {
 			indexList = append(indexList, types.DesanitizeIndex(safeIndex))

common/fixation_entry_test.go

@@ -88,6 +88,12 @@ func testWithFixationTemplate(t *testing.T, playbook []fixationTemplate, countOb
 			} else {
 				require.False(t, found, what)
 			}
+		case "has": //nolint:goconst
+			has := fs[play.store].HasEntry(ctx, index, block)
+			require.Equal(t, !play.fail, has, what)
+		case "stale":
+			stale := fs[play.store].IsEntryStale(ctx, index, block)
+			require.Equal(t, play.fail, stale, what)
 		case "get":
 			found := fs[play.store].GetEntry(ctx, index, &dummy)
 			if !play.fail {
@@ -145,12 +151,18 @@ func TestFixationEntryAdditionAndRemoval(t *testing.T) {
 	playbook := []fixationTemplate{
 		{op: "append", name: "entry #1", count: block0, coin: 0},
 		{op: "find", name: "entry #1", count: block0, coin: 0},
+		{op: "has", name: "entry #1", count: block0, coin: 0},
+		{op: "stale", name: "entry #1", count: block0, coin: 0},
 		{op: "getall", name: "to check exactly one index", count: 1},
 		{op: "append", name: "entry #2", count: block1, coin: 1},
+		{op: "has", name: "entry #1 (again)", count: block0, coin: 0},
+		{op: "has", name: "entry #2", count: block1, coin: 0},
 		// entry #1 not deleted because not enough time with refcount = zero
+		{op: "has", name: "entry #1 (not stale yet)", count: block0},
 		{op: "find", name: "entry #1 (not stale yet)", count: block0},
 		{op: "block", name: "add STALE_ENTRY_TIME+1", count: types.STALE_ENTRY_TIME + 1},
 		// entry #1 now deleted because blocks advanced by STALE_ENTRY_TIME+1
+		{op: "has", name: "entry #1 (now stale/gone)", count: block0, fail: true},
 		{op: "find", name: "entry #1 (now stale/gone)", count: block0, fail: true},
 		{op: "find", name: "latest entry", coin: 1},
 		{op: "getall", name: "to check again exactly one index", count: 1},
@@ -223,13 +235,19 @@ func TestEntryStale(t *testing.T) {
 		{op: "append", name: "entry #3", count: block2, coin: 2},
 		// entry #1 should not be deleted because it has refcount != zero);
 		// entry #2 (refcount = zero) also not deleted because it is not oldest
+		{op: "has", name: "entry #1", count: block0, coin: 0},
+		{op: "has", name: "entry #2", count: block1, coin: 1},
 		{op: "find", name: "entry #1", count: block0 + 1, coin: 0},
 		{op: "find", name: "entry #2", count: block1 + 1, coin: 1},
 		{op: "block", name: "add STALE_ENTRY_TIME+1", count: types.STALE_ENTRY_TIME + 1},
 		// entry #2 now stale and therefore should not be visible
 		{op: "find", name: "entry #2", count: block1 + 1, fail: true},
+		// but should still be positive for HasEntry()
+		{op: "has", name: "entry #2 (still positive)", count: block1},
+		{op: "stale", name: "entry #2 (still positive)", count: block1, fail: true},
 		// entry #3 (refcount = zero) is old, but being the latest it always
 		// remains visible (despite of refcount and age).
+		{op: "has", name: "entry #3", count: block2, coin: 2},
 		{op: "find", name: "entry #3", count: block2 + 1, coin: 2},
 	}
 
@@ -323,7 +341,8 @@ func TestDelEntry(t *testing.T) {
 		{op: "find", name: "entry #1 version 0", coin: 0, count: block0},
 		{op: "find", name: "entry #1 version 1", coin: 1, count: block1},
 		// entry #1 find beyond the delete should fail
-		{op: "find", name: "entry #1 version 1", count: block2, fail: true},
+		{op: "has", name: "entry #1 version 1 (deleted)", count: block2, fail: true},
+		{op: "find", name: "entry #1 version 1 (deleted)", count: block2, fail: true},
 	}
 
 	testWithFixationTemplate(t, playbook, 3, 1)

common/fixation_migrate.go

@@ -187,7 +187,7 @@ func fixationMigrate3to4(ctx sdk.Context, fs *FixationStore) error {
 			// if StaleAt is set, then replace old style timer with new style timer
 			if entry.StaleAt != math.MaxUint && entry.StaleAt > ctxBlock {
 				fs.tstore.DelTimerByBlockHeight(ctx, entry.StaleAt, []byte{})
-				key := encodeForTimer(entry.Index, entry.Block, timerStaleEntry)
+				key := encodeForTimer(entry.SafeIndex(), entry.Block, timerStaleEntry)
 				fs.tstore.AddTimerByBlockHeight(ctx, entry.StaleAt, key, []byte{})
 			}
 

common/fixation_migrate_test.go

@@ -104,11 +104,11 @@ type mockEntry2to3 struct {
 }
 
 // V2_setEntryIndex stores an Entry index in the store
-func (fs FixationStore) V2_setEntryIndex(ctx sdk.Context, safeIndex string) {
+func (fs FixationStore) V2_setEntryIndex(ctx sdk.Context, safeIndex types.SafeIndex) {
 	storePrefix := types.EntryIndexPrefix + fs.prefix
 	store := prefix.NewStore(ctx.KVStore(fs.storeKey), types.KeyPrefix(storePrefix))
 	appendedValue := []byte(safeIndex) // convert the index value to a byte array
-	store.Set(types.KeyPrefix(storePrefix+safeIndex), appendedValue)
+	store.Set(types.KeyPrefix(storePrefix+string(safeIndex)), appendedValue)
 }
 
 // V2_setEntry modifies an existing entry in the store
@@ -167,7 +167,7 @@ func TestMigrate2to3(t *testing.T) {
 			numHeads += 1
 		}
 		entry := types.Entry{
-			Index:    safeIndex,
+			Index:    string(safeIndex),
 			Block:    tt.block,
 			StaleAt:  math.MaxUint64,
 			Data:     fs.cdc.MustMarshal(&coin),

common/timer.go

@@ -22,6 +22,8 @@ import (
 //    - WithCallbackByBlockTime(callback): sets the callback for block-time timers
 //    - AddTimerByBlockHeight(ctx, block, key, data): add a timer to expire at block height
 //    - AddTimerByBlockTime(ctx, timestamp, key, data): add timer to expire at block timestamp
+//    - HasTimerByBlockHeight(ctx, block, key): check whether a timer exists at block height
+//    - HasTimerByBlockTime(ctx, timestamp, key): check whether a timer exists at block timestamp
 //    - DelTimerByBlockHeight(ctx, block, key): delete a timer to expire at block height
 //    - DelTimerByBlockTime(ctx, timestamp, key): delete timer to expire at block timestamp
 //    - Tick(ctx): advance the timer to the ctx's block (height and timestamp)
@@ -37,6 +39,10 @@ import (
 // When the expiry block (or block time) arrives, the respective callback will be invoked with
 // the timer's _key_ and _data_. Adding the same timer again (i.e. same expiry block/block-time
 // and same key) will overwrite the exiting timer's data.
+// Trying to add a timer with expiry block not in the future, or expiry time not later than
+// current block's timestamp, will cause a panic.
+// Existence of a timer can be checked using HasTimerByBlockHeight() and HasTimerByBlockTimer(),
+// respectively. These return true if a timer exists that matches the block/block-time and key.
 // An existing timer can be deleted using DelTimerByBlockHeight() and AddTimerByBlockTime(),
 // respectively. The timer to be deleted must exactly match the block/block-time and key. Trying
 // to delete a non-existing timer will cause a panic.
@@ -196,10 +202,18 @@ func (tstore *TimerStore) addTimer(ctx sdk.Context, which types.TimerType, value
 	}
 }
 
+func (tstore *TimerStore) hasTimer(ctx sdk.Context, which types.TimerType, value uint64, key []byte) bool {
+	store := tstore.getStoreTimer(ctx, which)
+	timerKey := types.EncodeBlockAndKey(value, key)
+	return store.Has(timerKey)
+}
+
 func (tstore *TimerStore) delTimer(ctx sdk.Context, which types.TimerType, value uint64, key []byte) {
 	store := tstore.getStoreTimer(ctx, which)
 	timerKey := types.EncodeBlockAndKey(value, key)
 	if !store.Has(timerKey) {
+		// panic:ok: caller should only try to delete existing timers
+		// (use HasTimerByBlock{Height,Time} to check if a timer exists)
 		panic(fmt.Sprintf("delTimer which %d block %d key %v: no such timer", which, value, key))
 	}
 	store.Delete(timerKey)
@@ -209,6 +223,7 @@ func (tstore *TimerStore) delTimer(ctx sdk.Context, which types.TimerType, value
 // If a timer for that <block, key> tuple exists, it will be overridden.
 func (tstore *TimerStore) AddTimerByBlockHeight(ctx sdk.Context, block uint64, key []byte, data []byte) {
 	if block <= uint64(ctx.BlockHeight()) {
+		// panic:ok: caller should never add a timer with past expiry
 		panic(fmt.Sprintf("timer expiry block %d smaller than ctx block %d",
 			block, uint64(ctx.BlockHeight())))
 	}
@@ -219,18 +234,29 @@ func (tstore *TimerStore) AddTimerByBlockHeight(ctx sdk.Context, block uint64, k
 // If a timer for that <timestamp, key> tuple exists, it will be overridden.
 func (tstore *TimerStore) AddTimerByBlockTime(ctx sdk.Context, timestamp uint64, key []byte, data []byte) {
 	if timestamp <= uint64(ctx.BlockTime().UTC().Unix()) {
+		// panic:ok: caller should never add a timer with past expiry
 		panic(fmt.Sprintf("timer expiry time %d smaller than ctx time %d",
 			timestamp, uint64(ctx.BlockTime().UTC().Unix())))
 	}
 	tstore.addTimer(ctx, types.BlockTime, timestamp, key, data)
 }
 
+// HasTimerByBlockHeight checks whether a timer exists for the <block, key> tuple.
+func (tstore *TimerStore) HasTimerByBlockHeight(ctx sdk.Context, block uint64, key []byte) bool {
+	return tstore.hasTimer(ctx, types.BlockHeight, block, key)
+}
+
+// HasTimerByBlockTime checks whether a timer exists for the <timestamp, key> tuple.
+func (tstore *TimerStore) HasTimerByBlockTime(ctx sdk.Context, timestamp uint64, key []byte) bool {
+	return tstore.hasTimer(ctx, types.BlockTime, timestamp, key)
+}
+
 // DelTimerByBlockHeight removes an existing timer for the <block, key> tuple.
 func (tstore *TimerStore) DelTimerByBlockHeight(ctx sdk.Context, block uint64, key []byte) {
 	tstore.delTimer(ctx, types.BlockHeight, block, key)
 }
 
-// DelTimerByBlockHeight removes an existing timer for the <timestamp, key> tuple.
+// DelTimerByBlockTime removes an existing timer for the <timestamp, key> tuple.
 func (tstore *TimerStore) DelTimerByBlockTime(ctx sdk.Context, timestamp uint64, key []byte) {
 	tstore.delTimer(ctx, types.BlockTime, timestamp, key)
 }

common/timer_test.go

@@ -78,10 +78,16 @@ func testWithTimerTemplate(t *testing.T, playbook []timerTemplate, countTS int)
 		switch play.op {
 		case "addheight":
 			tstore[play.store].AddTimerByBlockHeight(ctx, play.value, key, data)
+		case "hasheight":
+			has := tstore[play.store].HasTimerByBlockHeight(ctx, play.value, key)
+			require.Equal(t, play.data == "has", has)
 		case "delheight":
 			tstore[play.store].DelTimerByBlockHeight(ctx, play.value, key)
 		case "addtime":
 			tstore[play.store].AddTimerByBlockTime(ctx, play.value, key, data)
+		case "hastime":
+			has := tstore[play.store].HasTimerByBlockTime(ctx, play.value, key)
+			require.Equal(t, play.data == "has", has)
 		case "deltime":
 			tstore[play.store].DelTimerByBlockTime(ctx, play.value, key)
 		case "nextheight":
@@ -118,11 +124,14 @@ func TestTimerBlockHeight(t *testing.T) {
 		{op: "nextheight", name: "next timeout infinity", value: math.MaxUint64},
 		{op: "tickheight", name: "tick without timers", value: 100, fire: 0},
 		{op: "addheight", name: "add timer no-1", value: 120, key: "a", data: "no-1."},
+		{op: "hasheight", name: "has timer no-1", value: 120, key: "a", data: "has"},
 		{op: "nextheight", name: "next timeout no-1", value: 120},
 		{op: "tickheight", name: "tick before timer no-1", value: 110, fire: 0},
 		{op: "tickheight", name: "tick after timer no-1", value: 130, key: "a", fire: 1, data: "no-1."},
+		{op: "hasheight", name: "gone timer no-1", value: 120, key: "a", data: "gone"},
 		{op: "nextheight", name: "next timeout no-1", value: math.MaxUint64},
-		{op: "addheight", name: "add timer no-2", value: 140, data: "no-2.", key: "a"},
+		{op: "addheight", name: "add timer no-2", value: 140, key: "a", data: "no-2."},
+		{op: "hasheight", name: "has timer no-2", value: 140, key: "a", data: "has"},
 		{op: "tickheight", name: "tick exactly on timer no-2", value: 140, key: "a", fire: 1, data: "no-2."},
 		{op: "nextheight", name: "next timeout infinity again", value: math.MaxUint64},
 	}
@@ -136,9 +145,11 @@ func TestTimerBlockTime(t *testing.T) {
 		{op: "nexttime", name: "next timeout infinity", value: math.MaxUint64},
 		{op: "ticktime", name: "tick without timers", value: 100, fire: 0},
 		{op: "addtime", name: "add timer no-1", value: 120, key: "b", data: "no-1."},
+		{op: "hastime", name: "has timer no-1", value: 120, key: "b", data: "has"},
 		{op: "nexttime", name: "next timeout no-1", value: 120},
 		{op: "ticktime", name: "tick before timer no-1", value: 110, fire: 0},
 		{op: "ticktime", name: "tick after timer no-1", value: 130, key: "b", fire: 1, data: "no-1."},
+		{op: "hastime", name: "gone timer no-1", value: 120, key: "b", data: "gone"},
 	}
 
 	testWithTimerTemplate(t, playbook, 1)
@@ -192,8 +203,10 @@ func TestDeleteTimers(t *testing.T) {
 		{op: "addheight", name: "add timer no 3", value: 140, key: "c", data: "no-3."},
 		{op: "tickheight", name: "tick before all", value: 110, fire: 0},
 		{op: "delheight", name: "del timer no 2a", value: 130, key: "bx"},
+		{op: "hasheight", name: "gone timer no 2a", value: 130, key: "bx", data: "gone"},
 		{op: "tickheight", name: "tick between no-2,no-3", value: 135, key: "aby", fire: 2, data: "no-1.no-2b."},
 		{op: "delheight", name: "del timer no 3", value: 140, key: "c"},
+		{op: "hasheight", name: "gone timer no 3", value: 140, key: "c", data: "gone"},
 		{op: "nextheight", name: "next timeout no-3", value: 140},
 		{op: "tickheight", name: "tick after all", value: 155, fire: 0, key: "", data: ""},
 	}

common/types/ascii.go

@@ -49,27 +49,3 @@ func ValidateString(s string, restrictType charRestrictionEnum, disallowedChars
 
 	return true
 }
-
-// sanitizeIdnex checks that a string contains only visible ascii characters
-// (i.e. Ascii 32-126), and appends a (ascii) DEL to the index; this ensures
-// that an index can never be a prefix of another index.
-func SanitizeIndex(index string) (string, error) {
-	for i := 0; i < len(index); i++ {
-		if index[i] < ASCII_MIN || index[i] > ASCII_MAX {
-			return index, ErrInvalidIndex
-		}
-	}
-	return index + string([]byte{ASCII_DEL}), nil
-}
-
-// desantizeIndex reverts the effect of sanitizeIndex - removes the trailing
-// (ascii) DEL terminator.
-func DesanitizeIndex(safeIndex string) string {
-	return safeIndex[0 : len(safeIndex)-1]
-}
-
-func AssertSanitizedIndex(safeIndex string, prefix string) {
-	if []byte(safeIndex)[len(safeIndex)-1] != ASCII_DEL {
-		panic("Fixation: prefix " + prefix + ": unsanitized safeIndex: " + safeIndex)
-	}
-}

common/types/fixationEntry.go

@@ -6,7 +6,42 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
-// IsStale tests whether an entry is stale, i.e. has refcount zero _and_
+// SafeIndex is a sanitized string, i.e. contains only visible ascii characters
+// (i.e. Ascii 32-126), and terminates with (ascii) DEL; this ensures that an
+// index can never be a prefix of another index.
+type SafeIndex string
+
+// sanitizeIndex checks that a string contains only visible ascii characters
+// (i.e. Ascii 32-126), and appends a (ascii) DEL to the index; this ensures
+// that an index can never be a prefix of another index.
+func SanitizeIndex(index string) (SafeIndex, error) {
+	for i := 0; i < len(index); i++ {
+		if index[i] < ASCII_MIN || index[i] > ASCII_MAX {
+			return SafeIndex(""), ErrInvalidIndex
+		}
+	}
+	return SafeIndex(index + string([]byte{ASCII_DEL})), nil
+}
+
+// desantizeIndex reverts the effect of SanitizeIndex - removes the trailing
+// (ascii) DEL terminator.
+func DesanitizeIndex(safeIndex SafeIndex) string {
+	return string(safeIndex[0 : len(safeIndex)-1])
+}
+
+func AssertSanitizedIndex(safeIndex SafeIndex, prefix string) {
+	if []byte(safeIndex)[len(safeIndex)-1] != ASCII_DEL {
+		// panic:ok: intended assertion
+		panic("Fixation: prefix " + prefix + ": unsanitized safeIndex: " + string(safeIndex))
+	}
+}
+
+// SafeIndex returns the entry's index
+func (entry Entry) SafeIndex() SafeIndex {
+	return SafeIndex(entry.Index)
+}
+
+// IsStaleBy tests whether an entry is stale, i.e. has refcount zero _and_
 // has passed its stale_at time (more than STALE_ENTRY_TIME since deletion).
 func (entry Entry) IsStaleBy(block uint64) bool {
 	if entry.GetRefcount() == 0 {
@@ -17,6 +52,12 @@ func (entry Entry) IsStaleBy(block uint64) bool {
 	return false
 }
 
+// IsStale tests whether an entry is currently stale, i.e. has refcount zero _and_
+// has passed its stale_at time (more than STALE_ENTRY_TIME since deletion).
+func (entry Entry) IsStale(ctx sdk.Context) bool {
+	return entry.IsStaleBy(uint64(ctx.BlockHeight()))
+}
+
 // IsDeletedBy tests whether an entry is deleted, with respect to a given
 // block, i.e. has entry.DeletAt smaller or equal to that that block.
 func (entry Entry) IsDeletedBy(block uint64) bool {