catchpoints: don't make duplicate account hashes in prepareNormalizedBalances

[cce]

Summary

When an account is split over multiple catchpoint file chunks, we attempt to add its account hash to the catchpoint merkle trie twice, yielding a database error when processing the catchpoint. This is an alternate method to #4666 for fixing this issue, that stops the duplicate merkle trie hash issue at the source, rather than later at insert time.

Test Plan

New tests added to exercise accounts split over multiple catchpoint records/chunks.

[codecov]

Codecov Report

Merging #4668 (d792a77) into master (b29d7ff) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4668      +/-   ##
==========================================
+ Coverage   54.38%   54.45%   +0.06%     
==========================================
  Files         403      403              
  Lines       51928    51938      +10     
==========================================
+ Hits        28243    28284      +41     
+ Misses      21308    21280      -28     
+ Partials     2377     2374       -3     

Impacted Files	Coverage Δ
ledger/accountdb.go	73.00% <100.00%> (+0.13%)	⬆️
ledger/catchupaccessor.go	62.40% <100.00%> (+0.36%)	⬆️
cmd/algoh/blockWatcher.go	77.77% <0.00%> (-3.18%)	⬇️
agreement/cryptoVerifier.go	67.60% <0.00%> (-2.12%)	⬇️
agreement/proposalManager.go	96.07% <0.00%> (-1.97%)	⬇️
ledger/acctupdates.go	70.19% <0.00%> (+0.59%)	⬆️
ledger/ledger.go	67.30% <0.00%> (+0.96%)	⬆️
ledger/tracker.go	78.72% <0.00%> (+3.82%)	⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[algorandskiy]

The hashes fix looks ok to me

[jannotti]

All the stagingWriter jazz is so we can mock it to test it?

[algorandskiy]

All the stagingWriter jazz is so we can mock it to test it?

Correct. I mocked the minimal subset of functions otherwise it is a rabbit hole.

[cce]

I updated TestFullCatchpointWriter and TestFullCatchpointWriterOverflowAccounts to call accessor.BuildMerkleTrie() after accessor.ProgressStagingBalances(). This led to TestFullCatchpointWriterOverflowAccounts failing with the fix reverted, so that's good..

A more complete test might be to instantiate the catchpointService itself, which is what calls BuildMerkleTrie() after downloading the catchpoint file. Today we copy-paste much of the catchpointService’s logic into the tests (but not this part, to call BuildMerkleTrie). that kind of whole-service test would require mocking the network peers that provide catchpoint files and blocks.

[algorandskiy]

I guess good enough for now