TestRequestBlockBytesErrors: Various fixes

[algonautshant]

Summary

The test TestRequestBlockBytesErrors has failed, and the reason is unidentifiable.
Various fixes are here, including more verbose error reporting to identify the issue when it occurs again.

• Add waitgrop to ledger to wait before closing the ledger and blockQ when goroutines are using them.
• In universalFetcher, return the error immediately in case of an error.
• Proper closing of objects in TestRequestBlockBytesErrors, and more verbose error reporting.

The waitGroup is needed for the following situation:

in blockService.go listenForCatchupReq

When, immediately after calling bq.handleCatchupReq, BlockService.Stop() is called, blockQueue will be nil by the time blockQueue.getEncodedBlockCert is called.

0  0x0000000004e16846 in github.com/algorand/go-algorand/ledger.(*blockQueue).checkEntry
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/ledger/blockqueue.go:215
1  0x0000000004e174a9 in github.com/algorand/go-algorand/ledger.(*blockQueue).getEncodedBlockCert
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/ledger/blockqueue.go:295
2  0x0000000004e3a2a7 in github.com/algorand/go-algorand/ledger.(*Ledger).EncodedBlockCert
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/ledger/ledger.go:516
3  0x0000000004f46eed in github.com/algorand/go-algorand/rpcs.topicBlockBytes
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/rpcs/blockService.go:355
4  0x0000000004f45e1b in github.com/algorand/go-algorand/rpcs.(*BlockService).handleCatchupReq
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/rpcs/blockService.go:299
5  0x0000000004f45318 in github.com/algorand/go-algorand/rpcs.(*BlockService).listenForCatchupReq
   at /Users/shantkarakashian/go/src/github.com/algorand/go-algorand/rpcs/blockService.go:245

Test Plan

Test is enhanced.

[codecov-commenter]

Codecov Report

Merging #2928 (b653bd2) into master (01d853f) will increase coverage by 0.02%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master    #2928      +/-   ##
==========================================
+ Coverage   47.27%   47.30%   +0.02%     
==========================================
  Files         356      356              
  Lines       57239    57244       +5     
==========================================
+ Hits        27062    27081      +19     
+ Misses      27108    27100       -8     
+ Partials     3069     3063       -6     

Impacted Files	Coverage Δ
rpcs/blockService.go	52.73% <0.00%> (-0.80%)	⬇️
catchup/universalFetcher.go	94.96% <100.00%> (+0.06%)	⬆️
ledger/roundlru.go	90.56% <0.00%> (-5.67%)	⬇️
network/wsNetwork.go	60.90% <0.00%> (ø)
ledger/acctupdates.go	63.09% <0.00%> (+0.59%)	⬆️
network/requestTracker.go	71.12% <0.00%> (+0.86%)	⬆️
catchup/peerSelector.go	100.00% <0.00%> (+1.04%)	⬆️
crypto/merkletrie/node.go	93.48% <0.00%> (+1.86%)	⬆️
crypto/merkletrie/trie.go	68.61% <0.00%> (+2.18%)	⬆️
cmd/algoh/blockWatcher.go	80.95% <0.00%> (+3.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01d853f...b653bd2. Read the comment docs.

[tsachiherman]

I'm not sure why the ledger need to be modfied. Wouldn't it be enough to have the wait group on the block service ? i.e. tsachiherman:tsachi/TestRequestBlockBytesErrors

[algonautshant]

Yes, wait group is sufficient to be on the block service, as long as the block service is closed before the ledger service, which is already the case.
I will merge your change here.