Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

services/horizon/internal/ingest/processors: Accommodate eviction of soroban ledger entries in asset stats endpoint #5033

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Aug 31, 2023

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    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

#5031 describes a scenario where the asset stats endpoint will return incorrect data for contract holders when the ledger entries which store asset balances outlives the asset contract instance storage.

To fix this issue we will ingest all BalanceValue ledger entries and compute the number of contract holders and the total amount held by all contracts. These stats will be stored in a new table:

CREATE TABLE contract_asset_stats (
     -- contract id of the asset contract
     contract_id BYTEA PRIMARY KEY ,
     -- stat contains the number of contracts which hold this asset and
     -- the total amount held by all the holders
     stat       JSONB NOT NULL
);

We will still ingest AssetInfo ledger entries so we can map stellar classic assets to their corresponding soroban contracts.

In the asset stats endpoint we will fetch Stellar classic asset stats and, if we have a mapping from the Stellar classic asset to its soroban contract, we will join the contract_asset_stats table to fetch stats on contract holders. If there is no matching row in contract_asset_stats we will assume there are no contract holders.

With this fix, if an asset contract is evicted but still has live BalanceValue ledger entries, the asset stats endpoint will omit all the contract holders. However, once the asset contract is restored, the asset stats endpoint will correctly include all the contract holders.

Why

See #5031

Known limitations

It is possible for Horizon to ingest BalanceValue ledger entries which do not belong to real Stellar asset contracts. The stats for such fake asset contracts will never be included in the asset stats endpoint response, but, they will still occupy space in the contract_asset_stats table. There will be 1 row in contract_asset_stats for each "fake" asset contract.

@tamirms tamirms changed the title services/horizon/internal/ingest/processors: Accomodate eviction of asset contract ledger entries in asset stats e… services/horizon/internal/ingest/processors: Accommodate eviction of soroban ledger entries in asset stats endpoint Aug 31, 2023
@tamirms tamirms marked this pull request as ready for review August 31, 2023 08:36
@tamirms tamirms requested a review from a team August 31, 2023 12:52
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nitpicks with a couple of deeper questions.

I know I missed the discussion of this issue earlier in the week, but I'm wondering if there's another approach. I added a comment on the issue about it; it's probably better to discuss there: #5031 (comment).

Comment on lines -583 to -585
// all balances remaining in contractAssetStats do not belong to
// stellar asset contracts (because all stellar asset contracts must
// be in contractToAsset) so we can ignore them
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on how this is now possible (from the PR description):

It is possible for Horizon to ingest BalanceValue ledger entries which do not belong to real Stellar asset contracts.

I think I understand but I just want to make sure, also for other reviewers. Is this an "acts like a duck" situation?

services/horizon/internal/db2/history/main.go Outdated Show resolved Hide resolved
return nil
rowsAffected, err = p.assetStatsQ.InsertAssetContractStat(ctx, assetContractStat.ConvertToHistoryObject())
if err != nil {
return errors.Wrap(err, "error inserting asset contract stat")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could include contract ID or some other details here? unless this would be visible elsewhere in the logs

@Shaptic Shaptic requested a review from a team August 31, 2023 23:29
@tamirms tamirms changed the base branch from master to release-horizon-v2.27.0 November 6, 2023 10:45
@tamirms tamirms force-pushed the asset-stats branch 2 times, most recently from 273cd2f to f32583f Compare November 6, 2023 12:03
@tamirms
Copy link
Contributor Author

tamirms commented Nov 7, 2023

I have updated this PR to fix #5071 ( Horizon does not account for expired contract balances in asset stats endpoint). Now we store all asset balances held by contracts in the contract_asset_balances table:

CREATE TABLE contract_asset_balances (
     key_hash BYTEA PRIMARY KEY,
     asset_contract_id BYTEA NOT NULL,
     amount       numeric(39,0) NOT NULL, -- 39 digits is sufficient for a 128 bit integer
     expiration_ledger integer NOT NULL
);

CREATE INDEX "contract_asset_balances_by_expiration" ON contract_asset_balances USING btree (expiration_ledger);

When ingesting contract asset balances count how many balances are active vs archived (expired). We make sure to update these statistics when balances expire or are restored.

I recommend reviewing the PR in the following order for a better understanding of the code:

  • First look at the schema changes in the migrations to understand the data model
  • Then, review the db functions in services/horizon/internal/db2/history/asset_stats.go
  • Finally, review the asset stats processor changes

@tamirms
Copy link
Contributor Author

tamirms commented Nov 7, 2023

Unfortunately I was not able to add any integration tests which cover the case where an asset balance is expired / restored because the stellar asset contract configures the lifetime of asset balances to span a month's worth of ledgers:

https://github.com/stellar/rs-soroban-env/blob/main/soroban-env-host/src/builtin_contracts/stellar_asset_contract/balance.rs#L42-L47

even with accelerated ledger close times, it would take too long for an asset balance to expire. So I have only tested the expiration / restoration functionality through unit tests.

@2opremio
Copy link
Contributor

2opremio commented Nov 7, 2023

Maybe we should ask core to provide a way to test this more easily?

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for addressing feedback!

Copy link
Contributor

@urvisavla urvisavla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! However, the VerifyRange integration test is failing. Not sure if that needs to be addressed.

@tamirms
Copy link
Contributor Author

tamirms commented Nov 17, 2023

However, the VerifyRange integration test is failing. Not sure if that needs to be addressed.

This issue was fixed by @sreuland in the master branch.

@tamirms tamirms merged commit 11e5322 into stellar:release-horizon-v2.27.0 Nov 17, 2023
35 of 36 checks passed
@tamirms tamirms deleted the asset-stats branch November 17, 2023 14:38
tamirms added a commit to tamirms/go that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants