Ledger Deltas: AccountDelta retrieval through ledger/catchup service#4658
Ledger Deltas: AccountDelta retrieval through ledger/catchup service#4658winder merged 71 commits intoalgorand:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4658 +/- ##
==========================================
+ Coverage 53.76% 53.80% +0.04%
==========================================
Files 421 422 +1
Lines 53849 54002 +153
==========================================
+ Hits 28950 29056 +106
- Misses 22543 22589 +46
- Partials 2356 2357 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Eric-Warehime
changed the title
winder
left a comment
winder
left a comment
There was a problem hiding this comment.
Thanks for the updates, looks good!
|
|
||
| // deltas stores updates for every round after dbRound. | ||
| deltas []ledgercore.AccountDeltas | ||
| deltas []ledgercore.StateDelta |
There was a problem hiding this comment.
StateDelta contains AccountTotals too like what's in []roundTotals right? That is another duplicate now..
There was a problem hiding this comment.
StateDelta contains a block header pointer that contains the version data in []versions too ... IDK how far this goes but it would be duplicate memory too.. maybe having helper methods for each of the things being replaced?
There was a problem hiding this comment.
Yes it does, but combining it into the state deltas in the same way as the other deltas caused a bunch of test failures unrelated to these changes. So in favor of caution I decided it leave those separate.
There was a problem hiding this comment.
test compilation failures you didn't want to have to change or test run failures? which one, totals or versions or both?
There was a problem hiding this comment.
Combining the roundTotals caused test run failures--I'm struggling to remember exactly which test failed, but it caused incorrect totals to be reported. It's possible (probable?) this was just a mistake in my implementation.
The other problem with it is that the totals includes the currentDbRound which means it's longer than the size of deltas.
I'm happy to give it another shot if these are blocking issues.
There was a problem hiding this comment.
this one is easy to answer: totals and versions have the very first entry deducated for dbRound (latest committed), and deltas do not have it.
There was a problem hiding this comment.
It sounds like this shouldn't be a blocker.
There was a problem hiding this comment.
I've been working through this, removing the storage duplication, and I think it's best done in its own PR. @algorandskiy @cce Is it ok if we merge this for now and I'll follow up with a change to dedupe data stored in StateDelta/accountupdates?
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
LGTM, consider changing semantic of catchup.Service.syncRound
| syncStartNS int64 // at top of struct to keep 64 bit aligned for atomic.* ops | ||
| syncStartNS int64 // at top of struct to keep 64 bit aligned for atomic.* ops | ||
| // SyncRound, provided externally, which the ledger must keep in cache | ||
| syncRound uint64 |
There was a problem hiding this comment.
maybe syncRound -> targetSyncRound or something ?
the comment opens too much details about ledger to the service. What if the caller sets syncRound to X-cfg.MaxAcctLookback? in this case this would be a target sync round
There was a problem hiding this comment.
I've moved MaxAcctLookback access out of the catchup service which makes it more straightforward and avoids the catchup service knowing about the ledger trackers' cache size.
The new variable is disableSyncRound which is the first round we don't want to fetch from the network. Now the node passes in the correct round to the catchup service. This avoids requiring API users from having to think about cache settings when using the API.
Eric-Warehime
dismissed stale reviews from algorandskiy and winder
via
59563ba
7 participants
Summary
WIP PR that exposes AccountDeltas via the NodeInterface (APIs). The motivation for this is to be able to decouple the Indexer from having a go-algorand dependency via the local ledger--which requires us to be able to fetch Account changes per round.
This includes two new APIs:
The sync round API will only be exposed via an admin API, and will specify the minimum round that the user wants to ensure the cache will hold the AccountDeltas object for. In order to implement this we simply prevent the ledger from progressing via the agreement service (by not running it), and no-op any round fetches by the catchup service until the sync round changes.
TODO