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

rs-soroban-env/soroban-env-host/src/storage.rs

Line 382 in 36d33cb

pub(crate) fn extend_ttl(

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.

rs-soroban-env/soroban-env-host/src/storage.rs

Lines 436 to 446 in 36d33cb

	if new_live_until > host.max_live_until_ledger()? {
	if let Some(durability) = get_key_durability(&key) {
	if matches!(durability, ContractDataDurability::Persistent) {
	new_live_until = host.max_live_until_ledger()?;
	} else {
	return Err(host.err(
	ScErrorType::Storage,
	ScErrorCode::InvalidAction,
	"trying to extend past max live_until ledger",
	&[new_live_until.into()],
	));

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: stellar/rs-soroban-sdk#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.