Extending the ttl of a temporal storage to exisiting maximum live until ledger would always fail

[yanliu18]

What version are you using?

20.1.0

What did you do?

Formal Auditting for Client Code https://github.com/stellar/rs-soroban-env/blob/36d33cb6c986c9a8a9200b7eb04cf02e2c3f0ef4/soroban-env-host/src/storage.rs#L382

When call to the extend_ttl() function in host/storage, when the function parameter extend_to = li.max_entry_ttl (li.max_entry_ttl = host.with_ledger_info(|li| li.max_entry_ttl)) and parameter key matches ContractData(LedgerKeyContractData {_, _, StorageType::Temporal, }), the call to extend_ttl will always return an error.

This is because of the following condition checking where, new_live_until = li_sequece_number + extend_to host.max_live_until_ledger() = li_sequece_number + li.max_entry_ttl - 1 will always go to the else branch. https://github.com/stellar/rs-soroban-env/blob/36d33cb6c986c9a8a9200b7eb04cf02e2c3f0ef4/soroban-env-host/src/storage.rs#L436-L446

What did you expect to see?

Though it is rare for a contract instance to set its temporal stroage to the exisiting max livable ledger sequence, it is still possible. Maybe it is reasonable to calculate the new_live_until in the same way as the max_live_live_until_ledger.

[jayz22]

Thanks @yanliu18 for reporting this issue. Indeed some of the terminologies defined can be a little confusing. You've brought up a couple good points worth clarifying:

• The internal logic for calculating new_live_until is treating the input as "extend by" (not including the current ledger) where as the li.max_entry_ttl is being treated as number of ledgers to live including the current ledger.
• The Temporary entry cannot be attempted to be extended past the ledger max, it throws error instead of being clamped to the max. This was an intentional design (@dmkozh gave a good explanation here) but it's worth clarifying in the documentation.

Although not a protocol bug, I think both of these points worth being clarified via better documentation, I will open a PR to do so. The SDK has already been updated to make sure the max_entry_ttl getter takes care of the boundary offset: https://github.com/stellar/rs-soroban-sdk/pull/1269, so that user can't trigger the error using it.

[yanliu18]

Thanks @yanliu18 for reporting this issue. Indeed some of the terminologies defined can be a little confusing. You've brought up a couple good points worth clarifying:

• The internal logic for calculating new_live_until is treating the input as "extend by" (not including the current ledger) where as the li.max_entry_ttl is being treated as number of ledgers to live including the current ledger.
• The Temporary entry cannot be attempted to be extended past the ledger max, it throws error instead of being clamped to the max. This was an intentional design (@dmkozh gave a good explanation here) but it's worth clarifying in the documentation.

Although not a protocol bug, I think both of these points worth being clarified via better documentation, I will open a PR to do so. The SDK has already been updated to make sure the max_entry_ttl getter takes care of the boundary offset: stellar/rs-soroban-sdk#1269, so that user can't trigger the error using it.

Thank you @jayz22 for addressing this issue and reply to us. The changes look good. I shall let our team and clients know about this update.