Bump live_until of contract data when putting to a dead key

[brson]

What

In put_contract_data_into_ledger, if the storage key already exists, set the live_until ledger to the maximum of either the existing live_until, or the min_live_until_ledger.

Why

Note I need careful review here because I may not understand what I am doing.

In the native token contract, writing an allowance involves calling put_contract_data then extend_contract_data.

It is possible to put contract data to an existing storage key with a live_until value less than the current sequence number. In this case, the expired live_until is maintained. If this happens, then in the subsequent call to extend_contract_data, Storage::extend will see the expired live_until and return an error:

        // Extending deleted/non-existing/out-of-footprint entries will result in
        // an error.
        let (entry, old_live_until) = self.get_with_live_until_ledger(&key, host.budget_ref())?;
        let old_live_until = old_live_until.ok_or_else(|| {
            host.err(
                ScErrorType::Storage,
                ScErrorCode::InternalError,
                "trying to extend invalid entry",
                &[],
            )
        })?;

        let ledger_seq: u32 = host.get_ledger_sequence()?.into();
        if old_live_until < ledger_seq {
            return Err(host.err(
                ScErrorType::Storage,
                ScErrorCode::InternalError,
                "accessing no-longer-live entry",
                &[old_live_until.into(), ledger_seq.into()],
            ));
        }

The test case illustrates this.

Known limitations

It's possible this only happens in a test environment - maybe in a production environment these stale storage entries are not visible?

I see similar code in store_contract_instance but have not touched it.

[dmkozh]

It's possible this only happens in a test environment - maybe in a production environment these stale storage entries are not visible?

Yes, this assumption is correct. live_until < current_seq means that the entry has expired and is effectively inaccessible. The transaction will fail if it encounters such an entry and won't even spin up the host. That said, we currently don't strictly enforce this variant in the host, especially for the tests (but even for the enforcing footprint mode we probably should return an internal error).

I'm not strictly opposed to this change and it seems pretty harmless (though there is a small chance it would hide some real issue), but I would prefer making sure the invariant is maintained correctly. Currently there are much more ways to do weird things via advancing ledgers; maybe we should simulate entry expiration while in tests...

[dmkozh]

The transaction will fail if it encounters such an entry

Minor correction: that's only true for the persistent entries; the temporary entries (such as allowance) would simply be treated as non-existent.

[brson]

Thanks for the review. I understand this may not be the correct solution.

Currently there are much more ways to do weird things via advancing ledgers; maybe we should simulate entry expiration while in tests...

This seems like a reasonable alternative. I would like some resolution to this issue as presently the behavior under test seems incorrect.

Would exporting the snapshot from the Env and then reimporting it into a new Env cause the old entries to expire (vs. just mutating the sequence number on a single Env)? I don't understand exactly what snapshots are.

Minor correction: that's only true for the persistent entries; the temporary entries (such as allowance) would simply be treated as non-existent.

It doesn't appear to be the case that temporary entries are treated as non-existent - the bug I am seeing seems to be because the storage layer is confused about whether the entry exists or not - the put_contract_data method treats it as existing, the extend_contract_data method disagrees and thinks it doesn't exist.

[dmkozh]

It doesn't appear to be the case that temporary entries are treated as non-existent - the bug I am seeing seems to be because the storage layer is confused about whether the entry exists or not - the put_contract_data method treats it as existing, the extend_contract_data method disagrees and thinks it doesn't exist.

To be clear, that's the invariant we have in core. Host indeed doesn't implement it, which is why I'm saying that we have a bigger general problem with using host in the test environment.

When host is embedded by core, the invariants are:

• Ledger info is immutable (so the current sequence won't ever change)
• Storage is initialized with non-expired entries (so it's up to the embedder to fail when persistent entry has expired and to treat expired temp entries as non-existent)

Of course, neither of these invariants holds in the test environment at the moment. I think we should approach this holistically and probably try imitating the Core behavior via customizing Snapshot implementation. That would take some time though, so I've opened some issues: stellar/rs-soroban-sdk#1162, #1216

Would exporting the snapshot from the Env and then reimporting it into a new Env cause the old entries to expire

It depends on which snapshot you're talking about (there are storage snapshots and env snapshots). In any case, we probably want to emulate the proper expiration behavior.

[brson]

Since this isn't the right solution I'm closing it and following the linked issue.