ingest: Update ingestion library and horizon to adapt to protocol 23 representation of evictions #5619
Conversation
There was a problem hiding this comment.
This looks good from a change perspective if I understand it correctly, but I don't feel great about reviewing the asset changes since I'm relatively unfamiliar with them.
Either way, though, integration tests will reveal issues once RPC gets tagged off, assuming we have some for SAC+eviction+asset stats?
services/horizon/internal/ingest/processors/asset_stats_processor.go
Outdated
Show resolved
Hide resolved
| } else if err != nil { | ||
| return errors.Wrap(err, "error querying asset by asset code and issuer") | ||
| } else if dbContractID, ok := stat.GetContractID(); ok { | ||
| // the asset stat already has a column_id set which is unexpected (the column should be NULL) |
There was a problem hiding this comment.
is this condition likely to ever happen?
There was a problem hiding this comment.
no, it should never happen which is why this code path returns an error
| var rowsAffected int64 | ||
| stat, err := p.assetStatsQ.GetAssetStatByContract(ctx, contractID) | ||
| if pkgerrors.Is(err, sql.ErrNoRows) { | ||
| // we cannot know for sure if the contract id belongs to a legitimate SAC |
There was a problem hiding this comment.
if the contractId is even passed to this removeContractId, then it must be a SAC contract, right?
Meaning, this condition if pkgErrors.Is(err, sqlErrNoRows shud never be triggered, right?
So, doesnt it make sense to return a meaningful error instead of nil?
There was a problem hiding this comment.
unfortunately we cannot know if the contract id is a SAC unless we have the full ledger entry containing the asset metadata which is stored in persistent contract data. But, in protocol 23 we will not have the full ledger entry for evictions. We will only have the ledger keys which were evicted
There was a problem hiding this comment.
Overall, the changes look good to me conceptually. This PR does a great job of refactoring/simplifying some of the existing logic around error handling. I spent a couple of days reviewing it but there are still many nuances to the AssetStatsProcessor that are difficult to fully grasp.
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
CAP-62 and CAP-66 updates the LedgerCloseMeta xdr to no longer populate the
evictedPersistentLedgerEntriesfield. Instead, both persistent and temporary ledger keys will be included in theevictedTemporaryLedgerKeysfield which will be renamed toevictedKeysin protocol 23.The ingestion library previously presented evictions of persistent ledger entries as a
Changeinstance with theReasonset toLedgerEntryChangeReasonEviction. This allowed downstream consumers of aChangestream to handle evictions as if they were equivalent to ledger entry deletions. However, in protocol 23 we can no longer maintain this strategy because the full contents of the evicted ledger entry will not be present in the LedgerCloseMeta xdr. We will only have the ledger key for the evicted ledger entry.This means that ledger entry evictions will need to be handled outside of
Changestream processing by downstream services. The second commit of this PR fixes the asset stats processor in Horizon to adapt to this new reality where evictions no longer appear in theChangestream.The asset stats processor can ignore evicted contract balance ledger entries because contract balance ledger entries are deleted from the horizon db immediately upon expiration. However, the asset stats processor still needs to ingest the stellar asset contract instance storage so that we can link stellar classic assets with their corresponding contract id. So when the stellar asset contract instance storage is evicted we also need to handle that event so we can remove the link between stellar classic asset and the stellar asset contract id.
Also fixes #5578
Known limitations
[N/A]