ledger: use single SP verification hash/data query for catchpoint tracking & generation #5579
Conversation
cce
changed the title
Codecov Report
@@ Coverage Diff @@
## master #5579 +/- ##
==========================================
+ Coverage 55.79% 55.83% +0.04%
==========================================
Files 446 446
Lines 63418 63428 +10
==========================================
+ Hits 35381 35417 +36
+ Misses 25668 25642 -26
Partials 2369 2369
... and 7 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Pavel Zbitskiy <65323360+algorandskiy@users.noreply.github.com>
| @@ -201,6 +201,23 @@ func (ct *catchpointTracker) GetLastCatchpointLabel() string { | |||
| return ct.lastCatchpointLabel | |||
| } | |||
|
|
|||
| func (ct *catchpointTracker) getSPVerificationData() (encodedData []byte, spVerificationHash crypto.Digest, err error) { | |||
| err = ct.dbs.Transaction(func(ctx context.Context, tx trackerdb.TransactionScope) error { | |||
There was a problem hiding this comment.
do you need a Transaction scope if you're just reading?
There was a problem hiding this comment.
Oh, good point, the place I pulled this out of was inside a Transaction, but now it is read only
| b.StopTimer() | ||
| b.ReportMetric(float64(accountsNumber), "accounts") | ||
| } | ||
|
|
||
| func TestCatchpointReproducibleLabels(t *testing.T) { | ||
| partitiontest.PartitionTest(t) | ||
|
|
||
| if runtime.GOARCH == "arm" || runtime.GOARCH == "arm64" { |
There was a problem hiding this comment.
this seems not related to the PR so just curious what the purpose of the chang eis.
| // every other iteration modify CatchpointTracking to ensure labels generation does not depends on catchpoint file creation | ||
| if rnd%2 == 0 { | ||
| cfg2.CatchpointTracking = int64(crypto.RandUint63())%2 + 1 //values 1 or 2 | ||
| } |
There was a problem hiding this comment.
this is not guaranteed to modify the tracking value, for what looks like 2 reasons:
-
the 50% chance assignment, depending on the duration of this loop, might fail to modify the value (rare but flakey behavior)
-
the loop modifies this
rndvalue byrnd -= cfg.CatchpointInterval, which in this test appears to be 50. So I would expectrnd%2to always be true/false for a given test
why not just unconditionally set CatchpointTracking = (CatchpointTracking % 2) + 1 on every loop?
From #5574:
Summary
CatchpointTracking=1properly - its hash calculation is a side effect of writing spver data chunk into catchpoint file. Fixed that by calculating hash similarly to balance hash on demand.Test Plan
TestCatchpointReproducibleLabelsfor CatchpointTracking=1