reporter: fuzz tests for btccache #98

gusin13 · 2022-11-04T01:29:48Z

Fixes

https://babylon-chain.atlassian.net/browse/BM-152

Description

Adds fuzz tests for btc cache

SebastianElvis

Thanks, I believe you get the fuzz tests right in the first place!

types/btccache_test.go

vitsalis

Nice work! I suggest testing a few more stuff:

vitsalis · 2022-11-04T08:50:30Z

testutil/datagen/reporter.go

+	blocks, _, _ := GenRandomBlockchainWithBabylonTx(numBlocks, 0, 0)
+	var ibs []*types.IndexedBlock
+
+	blockHeight := int32(numBlocks - 1)


Maybe we can have the starting height be a random int32 in the range of [0, max_int32-num_blocks]. Currently the height of the earliest block is always 0.

yup I agree, but blockHeight is the height of last/best block here, so I think we can do
blockHeight := rand.Int31() + int32(numBlocks) to randomize and then it would decrement subsequent block heights in loop.

Yep makes sense.

vitsalis · 2022-11-04T08:51:20Z

types/btccache_test.go

+		rand.Seed(seed)
+
+		// Create a new cache
+		maxEntries := datagen.RandomInt(100) + 2 // ensure maxEntries > 1


Why have 102 as the maximum size of the cache? We can test with more as well.

sure, changed to 1000 now.

types/btccache_test.go

vitsalis · 2022-11-04T08:57:48Z

types/btccache.go

-
-	return b.reverse()
-}
-
 // thread-unsafe version of Reverse
 func (b *BTCCache) reverse() error {


Why does this return an error if we always return nil?

maybe it was left over in a previous clean up, removing it now. We don't need to return errors in reverse.

types/btccache_test.go

vitsalis · 2022-11-04T09:07:52Z

types/btccache_test.go

+
+		// Get prev hash
+		tip := cache.Tip()
+		require.NotNil(t, tip)


Should we check that the tip is the last block of those that were inserted? Also, that the tip has the highest height than all other blocks.

Also, that the tip has the highest height than all other blocks.

Do we require this? The helper function which generates blocks here already ensures the heights are sorted.

Yep, the block generation mechanism does that, but we should test that this invariant holds in the cache as well. Minor though.

sure, fixed it now.

vitsalis · 2022-11-04T09:08:46Z

types/btccache_test.go

+		block, _ := vdatagen.GenRandomBlock(1, &prevHash)
+		newIb := types.NewIndexedBlockFromMsgBlock(rand.Int31(), block)
+		cache.Add(newIb)
+		require.Equal(t, numBlocks+1, cache.Size())


We should check that cache.Tip() is the new block that we added as well.

vitsalis · 2022-11-04T09:09:25Z

types/btccache_test.go

+			err = cache.RemoveLast()
+			require.NoError(t, err)
+		}
+		require.Equal(t, prevSize-deleteCount, cache.Size())


We should check the tip of the cache and also that the blocks that are still there correspond to the slice of the ones that we added.

I don't see this being added.

I thought this comment was for newly added blocks so I added it here? 🤔

ok, finally added this check now on deleted blocks here, as per our call yesterday.

require.Equal(t, cacheBlocksBeforeDeletion[:len(cacheBlocksBeforeDeletion)-int(deleteCount)], cacheBlocksAfterDeletion)

vitsalis

Thanks for the updates! Looks good overall, added some further comments

testutil/datagen/reporter.go

vitsalis · 2022-11-06T06:28:10Z

testutil/datagen/reporter.go

+	blocks, _, _ := GenRandomBlockchainWithBabylonTx(numBlocks, 0, 0)
+	var ibs []*types.IndexedBlock
+
+	blockHeight := rand.Int31() + int32(numBlocks)


Are there any overflow issues here? For example if rand.Int31 returns the largest positive integer for an int32, then we might have an overflow. That's why I suggested taking a random integer from [0, max_int32-numBlocks] and then adding numBlocks to it.

btw, once we identify the height, we could use the GetRandomIndexedBlocksFromHeight function. Currently those functions have overlap.

Yup, you are right, there will be overflow issues. Sure I can use [0, max_int32-numBlocks].

Currently those functions have overlap.

Sure I have refactored GetRandomIndexedBlocks, it can now call internally GetRandomIndexedBlocksFromHeight

vitsalis · 2022-11-06T06:33:24Z

types/btccache.go

@@ -35,7 +35,8 @@ func (b *BTCCache) Init(ibs []*IndexedBlock) error {
 		b.add(ib)
 	}

-	return b.reverse()
+	b.reverse()


It is weird that we expect the input to the Init function to be blocks in the reverse order. I think it would be better if this function received the blocks in the order that they should be added instead of having to do reversing stuff. The one that calls it should be responsible for providing the correct order.

Sure, I like this suggestion. I have refactored Init now, it expects blocks with sorted height, also we won't need the reverse now

vitsalis · 2022-11-06T06:33:54Z

testutil/datagen/reporter.go

+	blockHeight := rand.Int31() + int32(numBlocks)
+	for _, block := range blocks {
+		ibs = append(ibs, types.NewIndexedBlockFromMsgBlock(blockHeight, block))
+		blockHeight -= 1


It is weird that we need to construct a list in decreasing order in order to properly test Init(). See my comment on the function.

Sure, I have refactored Init, don't need to reverse it now.

vitsalis · 2022-11-06T06:36:54Z

types/btccache.go

@@ -139,7 +130,7 @@ func (b *BTCCache) GetAllBlocks() []*IndexedBlock {
 	return b.blocks
 }

-// FindBlock finds block at the given height in cache
+// FindBlock uses binary search to find the block with the given height in cache


vitsalis · 2022-11-06T06:40:28Z

types/btccache_test.go

+
+		// Add all indexed blocks to the cache
+		err = cache.Init(ibs)
+		if numBlocks > maxEntries {


Since this might happen more often that we want it to (did not do the exact probability calculation), this means that many test cases are just going to test this and stop. I suppose we don't want to many test cases to fail on this, so maybe you can only test this case 1/10 times? You can use the datagen.OneInN(10) method. We are already following this practice for some Babylon fuzz tests.

vitsalis · 2022-11-06T06:47:47Z

types/btccache_test.go

+		}
+		require.Equal(t, prevCacheHeight+int32(addCount), cache.Tip().Height)
+		require.Equal(t, newIbs[addCount-1], cache.Tip())
+		if addCount >= maxEntries {


From my understanding addCount is the of new blocks. But the cache already has numBlocks inside of it. Then, in that case, we should check that in the case that addCount+numBlocks>=maxEntries:

The last addCount blocks in the cache are equal to all the newIbs blocks

The first maxEntries-addCount blocks in the cache are equal to the last maxEntries-addCount blocks of the initial blocks that were added.

Otherwise, we should just check that the last addCount blocks in the cache are equal to all the newIbs blocks.

sure, good point. I have added these cases and some comments in the code to clarify as discussed in our call yesterday.

vitsalis · 2022-11-06T06:49:11Z

types/btccache_test.go

+			err = cache.RemoveLast()
+			require.NoError(t, err)
+		}
+		require.Equal(t, prevSize-deleteCount, cache.Size())


I don't see this being added.

vitsalis

Great work, thanks for the updates! Very minor comments, LGTM!

vitsalis · 2022-11-09T07:49:48Z

btcclient/query.go

@@ -71,5 +71,10 @@ func (c *Client) GetLastBlocks(stopHeight uint64) ([]*types.IndexedBlock, error)
 		}
 	}

+	// reverse the order of the blocks


Maybe add a note in the docstring about the ordering of the result?

sure, added.

vitsalis · 2022-11-09T07:49:52Z

types/btccache.go

+	if sortedByHeight := sort.SliceIsSorted(ibs, func(i, j int) bool {
+		return ibs[i].Height < ibs[j].Height
+	}); !sortedByHeight {
+		return ErrorUnsortedBlocks


Great check!

vitsalis · 2022-11-09T07:51:30Z

types/btccache_test.go

+
+		cache, err := types.NewBTCCache(maxEntries)
+		if err != nil {
+			require.ErrorIs(t, err, types.ErrInvalidMaxEntries)


If datagen.OneInN(10) returns false and the types.NewBTCCache returns an error, this check will pass, but it should not.

sure added this check

if !invalidMaxEntries { t.Fatalf("NewBTCCache returned error %s", err) }

vitsalis · 2022-11-09T07:52:12Z

types/btccache_test.go

+		// Add all indexed blocks to the cache
+		err = cache.Init(ibs)
+		if err != nil {
+			require.ErrorIs(t, err, types.ErrTooManyEntries)


sure, fixed now added check same as above.

Co-authored-by: Vitalis Salis <[email protected]>

gusin13 requested review from SebastianElvis, fishermanymc, gitferry, KonradStaniec and vitsalis November 4, 2022 02:05

gusin13 marked this pull request as ready for review November 4, 2022 02:06

SebastianElvis approved these changes Nov 4, 2022

View reviewed changes

types/btccache_test.go Show resolved Hide resolved

types/btccache_test.go Outdated Show resolved Hide resolved

types/btccache_test.go Outdated Show resolved Hide resolved

types/btccache_test.go Outdated Show resolved Hide resolved

types/btccache_test.go Outdated Show resolved Hide resolved

vitsalis reviewed Nov 4, 2022

View reviewed changes

vitsalis approved these changes Nov 6, 2022

View reviewed changes

vitsalis changed the base branch from main to dev November 6, 2022 07:23

vitsalis approved these changes Nov 9, 2022

View reviewed changes

gusin13 and others added 19 commits November 9, 2022 17:20

btc cache tests

e322d97

add more comments

e4f9dce

add more tests

096f642

assert size

2a0d972

refactor fns

5e9f8bf

delete random blocks

5173336

fix reverse fn

633d58e

increase max entries size in cache

e03bf71

add random blocks to cache

2ebee91

randomize height in add blocks

c62b999

shifted algo to binary search

fae9f80

add randomized block helper fn

7f2dc13

more checks for added blocks

a1826ae

catch error in init

006f235

check last added block is tip

54b95d1

add doc string

cea52f6

rename vars

a8521c0

Co-authored-by: Vitalis Salis <[email protected]>

add doc string

ab37985

ensure block heights in cache are in increasing order

a213ccd

gusin13 added 9 commits November 9, 2022 17:21

don't reverse in init

0c26348

refactor index blocks fn:

553def7

add one in n probabilistic error generation

a3d886f

fix happy path

404aada

add more checks

fc01f04

add comments

78731fb

add more assertions for block deletion

a8a1db0

add doc string

3ce7ba2

add more granular checks for 1/10 invalid case generation

dd9e756

gusin13 force-pushed the BM-152-types-btccache-test-go branch from 437a06c to dd9e756 Compare November 9, 2022 22:21

gusin13 merged commit e7cfa6b into dev Nov 9, 2022

gusin13 deleted the BM-152-types-btccache-test-go branch November 9, 2022 22:27

reporter: fuzz tests for btccache #98

reporter: fuzz tests for btccache #98

Conversation

gusin13 commented Nov 4, 2022 • edited Loading

Fixes

Description

SebastianElvis left a comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gusin13 commented Nov 4, 2022 •

edited

Loading

gusin13 Nov 5, 2022 •

edited

Loading

gusin13 Nov 4, 2022 •

edited

Loading

gusin13 Nov 8, 2022 •

edited

Loading

gusin13 Nov 8, 2022 •

edited

Loading

gusin13 Nov 8, 2022 •

edited

Loading

gusin13 Nov 8, 2022 •

edited

Loading