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

Added proof requirements to entry recreation #1572

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

SirTyson
Copy link
Contributor

@SirTyson SirTyson commented Nov 15, 2024

This update makes minor updates to evicted entry meta, provides an optimization preventing some writes of DELETED keys to the AST, and distinguishes between proof requirements when creating an entry for the first time vs. recreating an entry that was previously deleted.

The optimization for deleted keys checks the archive before writing a deleted key, and only writes the key if an ARCHIVED entry with the same key exists in the AST. If no ARCHIVED entry exists, we don't need to write the deleted key, as there in no double restore attack to prevent. This can be done safely and efficiently via the archival filters.

Unfortunately, I've recently realized an issue in new entry creation proofs wrt recreated entries. If a key has never existed before, we can optimize proofs-of-nonexistence via the archival filter. However, an issue arises when a key is archived, restored, deleted, then recreated.

Suppose LedgerEntry e with LedgerKey k is archived in AST[i]. The entry is later restored, then deleted such that the DELETED entry is written in a later epoch AST[k].

When k is recreated, the proof-of-nonexistence optimization is no longer possible. When the validator goes to check the binary fuse filter, it will see that the entry already exists in epoch i and k. Thus, a proof-of-existence for the DELETED entry in k is necessary for the recreation to be secure against eclipse attacks.

I've gone through several draft proposals, such as dividing DELETED and ARCHIVED keys into seperate filters, using multiple overlapping filters, etc. Unfortunately, I haven't found a solution where false positives don't result in an eclipse attack.

This seems like an edge case that is not bound to occur regularly, but still may provide a frustrating experience to the user. In particular, an entry must be archived, restored at least 1 epoch after being archived (any sooner and the restoration would merge with and delete the ARCHIVAL entry) , be restored, be deleted, then be recreated again. In this case, entry creation will require a proof.

Talking with some smart contract devs in the ecosystem, the only use case I could find for entry deletion then follow up recreation is in a temporary escrow accounts proposal to allow trading between classic DEX and Soroban DEX. TLDR: a temporary account is used to facilitate funds offloading from the classic DEX and entering Soroban. While the account is only used for a short time, it can't be a temporary entry because it holds funds.

The pattern I've abstracted from this proposal is that persistent entry deletion and recreation is usually done for operational reasons were the deletion occurs shortly after the initial creation. The DELETED key optimization I've included directly addresses this case.

I think we're probably ok here. The only time a recreation proof is required is when there is a relatively significant amount of time between the entry creation and follow up deletion (plus an archival and restore event before the deletion too). That being said, it probably warrants some conversation.

@SirTyson SirTyson force-pushed the cap-57-1.2 branch 2 times, most recently from 2f6efc1 to e772874 Compare November 15, 2024 08:25
core/cap-0057.md Outdated Show resolved Hide resolved
core/cap-0057.md Outdated Show resolved Hide resolved
core/cap-0057.md Outdated Show resolved Hide resolved
core/cap-0057.md Outdated
@@ -930,7 +1078,8 @@ persisted on validators via `lastArchivalEpochPersisted`. This will be useful to
as it is the cutoff point at which a proof will need to be generated for `RestoreFootprintOp`.

Whenever a `PERSISTENT` entry is evicted (i.e. removed from the Live State BucketList and added to the Hot Archive),
the full entry will be emitted via `evictedPersistentLedgerEntries`.
the entry key and its associated TTL key will be emitted via `evictedTemporaryLedgerKeys` (this field is unfortunately
named for legacy reasons).
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can just rename the field; the rename won't change the XDR compatibility, will it? There also shouldn't be consumers of this field either, as we don't populate it currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, especially since we need a new LedgerCloseMeta version anyway.

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.

2 participants