Reduce memory consumption of state verification #128
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the state verification logic by moving entry filtering from the StateVerifier's transform function to the history archive adapter level using the ingest.WithFilter option. The filtering logic for ConfigSetting, ContractCode, and ContractData entries is now applied at the source when reading from history archives, rather than during verification.
Key changes:
- Removed the
TransformLedgerEntryFunctiontype and related transform function logic from StateVerifier - Implemented filtering directly in
historyArchiveAdapter.GetState()usingingest.WithFilter - Updated
NewStateVerifierto remove the transform function parameter - Added network passphrase parameter to
newHistoryArchiveAdapterto support filtering logic - Added deduplication logic in test helper
generateRandomLedgerEntriesto ensure unique ledger keys - Updated test cases to remove transform function tests and adjust constructor calls
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ingest/verify.go | Removed TransformLedgerEntryFunction type and transform function logic; simplified StateVerifier constructor |
| internal/ingest/verify_test.go | Removed transform function tests; updated constructor call; added deduplication logic for test data generation |
| internal/ingest/history_archive_adapter.go | Added networkPassphrase parameter; implemented filtering using ingest.WithFilter |
| internal/ingest/history_archive_adapter_test.go | Updated constructor call to include network passphrase |
| internal/ingest/main.go | Updated constructor call to include network passphrase |
| internal/ingest/sample_changes_test.go | Updated constructor call to include network passphrase |
| go.mod | Updated stellar/go dependency version |
| go.sum | Updated checksums for stellar/go dependency |
Comments suppressed due to low confidence (1)
internal/ingest/verify.go:35
- The StateVerifier documentation still references
transformFunctionwhich has been removed. This step should be removed or updated to reflect the new filtering approach where filters are applied at the history archive adapter level.
// 0. Develop `transformFunction`. It should remove all fields and objects not
// stored in your app. For example, if you only store accounts, all other
// ledger entry types should be ignored (return ignore = true).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| ledgerEntryFilter := func(ledgerEntry xdr.LedgerEntry) bool { | ||
| if ledgerEntry.Data.Type == xdr.LedgerEntryTypeConfigSetting || | ||
| ledgerEntry.Data.Type == xdr.LedgerEntryTypeContractCode { | ||
| return false | ||
| } | ||
| if ledgerEntry.Data.Type == xdr.LedgerEntryTypeContractData { | ||
| _, assetFound := sac.AssetFromContractData(ledgerEntry, haa.networkPassphrase) | ||
| _, _, balanceFound := sac.ContractBalanceFromContractData(ledgerEntry, haa.networkPassphrase) | ||
| return assetFound || balanceFound | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| ledgerKeyFilter := func(ledgerKey xdr.LedgerKey) bool { | ||
| if ledgerKey.Type == xdr.LedgerEntryTypeConfigSetting || | ||
| ledgerKey.Type == xdr.LedgerEntryTypeContractCode { | ||
| return false | ||
| } | ||
| if ledgerKey.Type == xdr.LedgerEntryTypeContractData { | ||
| return sac.ValidContractBalanceLedgerKey(ledgerKey) || | ||
| sac.ValidAssetEntryLedgerKey(ledgerKey) | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Do we have test coverage for this? If not, is it possible to add some?
There was a problem hiding this comment.
there is an integration test for state verification which should exercise this code. I just updated the state verification unit tests to invoke these filters
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
CHANGELOG.mdwithin the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Reduce memory consumption of state verification by discarding bucket entries corresponding to contract data, contract code, and config setting ledger entries which are ignored by horizon.
depends on stellar/go#5827
Why
Since eviction has been disabled we have observed a significant increase in memory consumption during state verification runs:
Prior to the SLP which disabled background eviction on oct 9 / 10, we had a peak memory consumption of 14-15 GB. Afterwards, the peak memory consumption reached upwards of 32 GB.
If the SLP is the root cause of the increased memory consumption that would imply that most of the ledger entries we are encountering while streaming from the history archives are contract data / contract code ledger entries which ordinarily would be evicted. But, horizon actually only ingests a tiny subset of contract data. In particular, horizon only cares about SAC contract data ledger entries.
This PR takes advantage of that fact by introducing a filter which discards any contract data ledger entries that are not likely SAC ledger entries. By doing so we have eliminated a significant amount of memory consumption:
Now, the peak memory consumption is 11-12 GB which is even less than what we observed before the SLP.
Known limitations
[N/A]