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

ingest: Add support for ledger entry change type "restore" for Protocol 23 #5587

Merged
merged 19 commits into from
Feb 18, 2025

Conversation

urvisavla
Copy link
Contributor

@urvisavla urvisavla commented Feb 4, 2025

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 reviewed the changes in this PR and if I consider them worthwhile for being mentioned on release notes then I have updated the relevant CHANGELOG.md within 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.
  • 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

This PR adds support for a new ledger entry change type called restore in the ingest library, introduced in Protocol 23. Additionally, the meta for update and remove changes will now change if an entry was restored in the same transaction. For example, if an entry is restored and removed within the same transaction, the meta will be [restored, removed] and if restored and updated in the same transaction, the meta will be [restored, updated]. This differs from pre-Protocol 32, where updated and removed entries always appear as [state, updated] and [state, removed].

Main Changes

  1. Store the Change Type directly in the Change struct

Previously, the Change struct in the ingest library didn't store the change type explicitly but inferred it from the Pre and Post values. However, with the introduction of restore, distinguishing change types purely based on Pre and Post is no longer possible.

  1. Support for restore change type in change compactor

The compaction process in the ingest library combines multiple changes within the same ledger for a single change to reduce the number of DB operations. With the introduction of the restore change type, the compaction logic is updated to:

  • define how changes occurring after a restore change should be compacted.
  • define how a restore change should be compacted when it precedes other change types.
  1. Additionally, a new Change Compactor config EmitArchivedEntryRemovalChange has been added to control the handling REMOVED change for archived entries. By default, REMOVED changes are not emitted for entries that are created and removed within the same ledger. However, if an entry is restored and then removed in the same ledger, it is unclear whether a REMOVED change should be emitted, as this depends on the downstream system's strategy for storing archived entries. This setting allows REMOVED changes to be emitted for such entries which may be necessary for systems that store them. Systems that do not store archived entries (e.g. those running periodic garbage collection of archived entries) can set this to false to suppress the REMOVED change.

Why

#5421

Known limitations

  • Support for submitting the restore change type required for integration tests is currently unavailable so this change is only unit-tested.
  • This change updates the ingest library only. Updating downstream services such as Horizon and RPC are outside the scope of this PR.

@urvisavla urvisavla changed the base branch from master to protocol-23 February 4, 2025 23:20
@urvisavla urvisavla force-pushed the 5421/restore-change-type branch from 7433a31 to e443da6 Compare February 4, 2025 23:23
@urvisavla urvisavla changed the title (WIP) ingest: Add support for ledger entry change type "restore" ingest: Add support for ledger entry change type "restore" Feb 6, 2025
@urvisavla urvisavla changed the title ingest: Add support for ledger entry change type "restore" ingest: Add support for ledger entry change type "restore" for Protocol 23 Feb 6, 2025
@urvisavla urvisavla marked this pull request as ready for review February 6, 2025 20:12
@chowbao chowbao mentioned this pull request Feb 7, 2025
7 tasks
@urvisavla urvisavla marked this pull request as draft February 9, 2025 00:58
@urvisavla urvisavla changed the title ingest: Add support for ledger entry change type "restore" for Protocol 23 (WIP) ingest: Add support for ledger entry change type "restore" for Protocol 23 Feb 11, 2025
@urvisavla urvisavla force-pushed the 5421/restore-change-type branch from e239fcf to 586392c Compare February 11, 2025 23:04
@urvisavla urvisavla force-pushed the 5421/restore-change-type branch from d064abc to 25c51fa Compare February 13, 2025 02:16
@urvisavla urvisavla force-pushed the 5421/restore-change-type branch from 25c51fa to a954a67 Compare February 13, 2025 07:44
@urvisavla urvisavla changed the title (WIP) ingest: Add support for ledger entry change type "restore" for Protocol 23 ingest: Add support for ledger entry change type "restore" for Protocol 23 Feb 13, 2025
@urvisavla urvisavla marked this pull request as ready for review February 13, 2025 07:56
@urvisavla urvisavla requested a review from tamirms February 13, 2025 08:01
urvisavla and others added 4 commits February 13, 2025 09:25
@tamirms
Copy link
Contributor

tamirms commented Feb 14, 2025

@urvisavla the PR looks good, I think the only remaining task is to add a check to ensure that the Change Compactor instance is only used to compact changes from a single ledger

@urvisavla
Copy link
Contributor Author

@urvisavla the PR looks good, I think the only remaining task is to add a check to ensure that the Change Compactor instance is only used to compact changes from a single ledger

@tamirms
To ensure the change compactor processes changes from only one ledger sequence, I initially tried checking for mismatches like this:

for each existingChange in cache:
    if existingChange.LedgerSequenceNumber ≠ newChange.LedgerSequenceNumber:
        return "Ledger sequence mismatch: Cannot compact changes from different ledgers"

    break // Only check the first entry

However, I'm having trouble reliably getting the ledger sequence number from a Change. I've looked at Change::LedgerEntry::LastModifiedLedgerSeq (Pre & Post), Change::Transaction, Change::Ledger etc but none of them consistently provide the correct sequence number for all LedgerEntry changes.

What's the most reliable way to determine the current ledger sequence number for a Change without storing it in the ChangeCompactor object and then passing it with every AddChange call?

Also, it seems the new Change struct fields added in #5536 are not designed to be used with the Change Compactor which wasn't immediately obvious to me and caused some confusion.

@tamirms
Copy link
Contributor

tamirms commented Feb 18, 2025

@urvisavla you can get it from change.Ledger or, if that's not present you can check change.Transaction.Ledger, or you can fix #5578 and then it should always be present in change.Ledger

Also, it seems the new Change struct fields added in #5536 are not designed to be used with the Change Compactor which wasn't immediately obvious to me and caused some confusion.

The fields are not preserved after the compaction process but the changes fed into the change reader should still have valid fields

@urvisavla
Copy link
Contributor Author

The fields are not preserved after the compaction process but the changes fed into the change reader should still have valid fields

These fields must be preserved during compaction if we are to compare the ledger sequence number of incoming changes with existing ones in the cache. However, there are alternative solutions, such as storing the ledger sequence in the ChangeCompactor object. I have created #5607 to evaluate potential solutions and implement it separately to unblock this PR.

@tamirms
Copy link
Contributor

tamirms commented Feb 18, 2025

However, there are alternative solutions, such as storing the ledger sequence in the ChangeCompactor object.

yes, that's what I had in mind

I have created #5607 to evaluate potential solutions and implement it separately to unblock this PR.

👍

@urvisavla urvisavla merged commit 9ad4cdc into stellar:protocol-23 Feb 18, 2025
5 of 21 checks passed
@urvisavla urvisavla deleted the 5421/restore-change-type branch February 18, 2025 18:25
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