fix: fix potential memory leak in BTCCache

[SebastianElvis]

This PR fixed a potential memory leak issue in BTCCache. To remove an element in a slice, one needs to first dereference it so that the dereferenced object will be garbage-collected. This PR is based on a detailed discussion with @vitsalis and @KonradStaniec

Tested docker deployment locally and the PR does not affect current logic

[SebastianElvis]

Tested a little bit about this by using the following unit test

func MemUsed() uint64 {
	runtime.GC()
	var stats runtime.MemStats
	runtime.ReadMemStats(&stats)
	return stats.Alloc
}

func TestBtcCacheAdd(t *testing.T) {
	cache, err := types.NewBTCCache(10)
	require.NoError(t, err)

	runtime.GC()
	memUsedBefore := MemUsed()

	for n := 0; n < 100000; n++ {
		block := vdatagen.GetRandomIndexedBlocks(1)[0]
		cache.Add(block)
	}

	runtime.GC()
	memUsedAfter := MemUsed()

	t.Logf("Memory used before: %d bytes\n", memUsedBefore)
	t.Logf("Memory used after: %d bytes\n", memUsedAfter)
}

Without dereferencing, the result is

=== RUN   TestBtcCacheAdd
    btccache_test.go:37: Memory used before: 13410160 bytes
    btccache_test.go:38: Memory used after: 13410536 bytes
--- PASS: TestBtcCacheAdd (2.06s)

With dereferencing, the result is

=== RUN   TestBtcCacheAdd
    btccache_test.go:37: Memory used before: 13377368 bytes
    btccache_test.go:38: Memory used after: 13378104 bytes
--- PASS: TestBtcCacheAdd (1.88s)
PASS

So the dereferencing indeed saves some bytes. This looks like this is really the root cause.

[vitsalis]

Looks good! Added a suggestion. Let's also wait for @KonradStaniec to weigh in as well

[vitsalis]

Given that the above checks b.size() >= b.maxEntries, there's a case in which we need to remove more than only one element to have b.maxEntries elements. I know this case can't appear in the current code, but since we have such condition, we should handle it properly. Otherwise, if we're sure that the case b.size() > b.maxEntries can't happen, we should panic in this case and handle the below using a condition b.size() == b.maxEntries

[SebastianElvis]

Agree, I'm inclined to panic when b.size() > b.maxEntries since BTC cache does not support adding multiple elements at the same time

[KonradStaniec]

In general code looks good 👍

I am bit about unsure about panic as it is disaster waiting to happen. i.e some adding in the future function adding few elements without revising this method, I think golang standard library collections avoid panicing as much as possible, but this is no blocker for me for now.

Also I am still unsure about

So the dereferencing indeed saves some bytes. This looks like this is really the root cause.

The fact that there were some bytes saved in that case may only mean that garabage collector can retrive bytes faster in that case, and not that they won't be collected ever.

The only way to to be sure that this was indeed a root cause would be to have some kinda integration test which would excerise whole handleConnectedBlocks reporter logic, and then to run this test with memory profiler and see that memory does not grow in time. Wdyt about creating such test in separate task ?

[SebastianElvis]

The only way to to be sure that this was indeed a root cause would be to have some kinda integration test which would excerise whole handleConnectedBlocks reporter logic, and then to run this test with memory profiler and see that memory does not grow in time. Wdyt about creating such test in separate task ?

Agree that finding the root cause may need battletesting the reporter while using a memory profiler. The thing is that such an integration test may need to take a long time to execute in order to have enough number of BTC blocks. Any ideas?

[KonradStaniec]

Agree that finding the root cause may need battletesting the reporter while using a memory profiler. The thing is that such an integration test may need to take a long time to execute in order to have enough number of BTC blocks. Any ideas?

Hmm I see two options:

1. Have mock btc client which always respond with some big btc blocks (and feeds those block to reporter through block event chan)
2. Have mock btc node which always respond with some big btc blocks

First option is easier but does not test our interaction with rpcClient, second option is probably more complicated but tests every component. Either way we should have some integration tests :)