all: add support for new state expiration ledger entries #913

2opremio · 2023-09-01T08:28:53Z

What

Add support for the new state expiration ledger entries,

This PR also changes getLedgerEntries method so that doesn't filter out expired ledger entries. It simply returns whatever is in the database.

Why

Fixes #873

Known limitations

I left a few TODOs in the CLI, namely:

We should test that some invariants hold in meta matches for the bump/restore commands.
I haven't been able to update get_contract_spec_from_storage() with respect to expirations since I don't think the storage book-keeps expiration ledger entries.

@willemneal could you please take a look at the items above? Also, could you do some testing on the CLI once this is merged? Unfortunately, the XDR is out of sync with futurenet so you will have to use a local network.

We should probably start improving CLI testing considerably (particularly, integration tests against soroban-rpc). I wasn't comfortable fixing (2) since there were no tests covering it (AFICT).

Also, we should probably just get rid of the sandbox mode once and for all.

2opremio · 2023-09-01T11:11:12Z

To reviewers: Given the tight deadline for the testnet release, please let's not get picky with stuff which wouldn't be a problem in production.

It's fine to note it as a comment and create separate tickets, but let's not block the PR.

cmd/soroban-cli/src/commands/contract/bump.rs

cmd/soroban-cli/src/commands/contract/read.rs

cmd/soroban-rpc/internal/preflight/preflight.go

cmd/soroban-cli/src/utils.rs

cmd/soroban-rpc/internal/db/ledgerentry_test.go

tsachiherman

As you've already identified yourself, querying both the ledgerEntry and it's corresponding ledgerExpirationEntry would be better done on the go side; I think that a good starting point for that would be adding an optional expiration ledger sequence to the LedgerKeyAndEntry data structure.
Also - we could use some clarification from core whether the access to the expiration ledger entry is counted as an additional entry access or not. ( it might be a special case ).

2opremio · 2023-09-01T13:28:52Z

Also - we could use some clarification from core whether the access to the expiration ledger entry is counted as an additional entry access or not. ( it might be a special case ).

I doubt it since the host doesn't track them and AFAIU otherwise integration tests wouldn't had worked. But this is a question for @sisuresh

tsachiherman

I believe that one thing that is missing is the deletion of EvictedTemporaryLedgerKeys.
These need to be deleted as soon as they become stale, since temporary entries could not be restored.

2opremio · 2023-09-01T13:32:58Z

I believe that one thing that is missing is the deletion of EvictedTemporaryLedgerKeys. These need to be deleted as soon as they become stale, since temporary entries could not be restored.

Yes, that's why I had created #914

tsachiherman · 2023-09-01T13:41:19Z

I think that it shouldn't be a testnet blocker here, but we should add some type of validation when inserting the ledger entries into the database to ensure that we always have "pairs" of entry + expiration.
The alternative to that is to add this during the simulation & getledgerenttries, but that won't fix the situation.

2opremio · 2023-09-01T13:43:05Z

I think that it shouldn't be a testnet blocker here, but we should add some type of validation when inserting the ledger entries into the database to ensure that we always have "pairs" of entry + expiration.

This is Core's responsibility, I don't think we should be testing the metadata. Regardless, please create a separate ticket if you think this is important and let's get this merged.

sisuresh · 2023-09-01T14:03:58Z

Also - we could use some clarification from core whether the access to the expiration ledger entry is counted as an additional entry access or not. ( it might be a special case ).

I doubt it since the host doesn't track them and AFAIU otherwise integration tests wouldn't had worked. But this is a question for @sisuresh

Accessing the ExpirationEntry does not count as an additional entry access wrt the number of entries accessed, but we do charge for the bytes written in compute_rent_fee (which I believe you already use).

cmd/soroban-cli/src/utils.rs

willemneal · 2023-09-01T16:14:35Z

cmd/soroban-cli/src/utils.rs

+        } => match executable {
+            ContractExecutable::Token => {
+                // TODO/FIXME: I don't think it will work for token contracts, since we don't store them in the state?
+                let res = soroban_spec::read::parse_raw(&token::StellarAssetSpec::spec_xdr());


The spec doesn't come from the state, just the entry that points to the token contract so this does work.

Yes, I know the spec is obtained separately. What I meant is that, AFAIU, we don't store ContractInstance entries for token contracts since the user provides no wasm.

Maybe I am wrong, though, if that's the case, can you point me where that happens?

A non Wasm contract instance still needs to store the fact that it's a token.

A non Wasm contract instance still needs to store the fact that it's a token.

Yes, but my point is that we only seem to update the state when a wasm file is provided, which doesn't for token contracts.

Thus, when invoking a token contract, the state won't have its ledger entries.

tsachiherman · 2023-09-01T16:56:27Z

As you've already identified yourself, querying both the ledgerEntry and it's corresponding ledgerExpirationEntry would be better done on the go side; I think that a good starting point for that would be adding an optional expiration ledger sequence to the LedgerKeyAndEntry data structure. Also - we could use some clarification from core whether the access to the expiration ledger entry is counted as an additional entry access or not. ( it might be a special case ).

@2opremio I've created #916, which implements all the go-side code that would require to support retrieving the LedgerExpirationSeq while getting the ledger entries. I think that it would be best to fully merge your pr first before applying the other.

2opremio · 2023-09-01T19:45:11Z

I am not sure this makes sense unless we also hide the expiration ledger entries from the end user. Otherwise the expiration field is redundant.

tsachiherman · 2023-09-01T19:56:37Z

I don’t think this makes sense unless we also hide the expiration ledger entries from the end user. Otherwise the expiration field is redundant.

I wouldn't "hide" them explicitly, rather than allowing the user to explore remaining lifetime.
In particular, I'd like to save the js code of doing hash calculations and unnecessary parsing to get this value.

2opremio · 2023-09-01T20:00:34Z

I understand the motivation but What’s the point of allowing to query expiration entries if you are providing the expiration as part of the result? I would go one way or the other, otherwise it’s confusing.

2opremio · 2023-09-01T22:36:40Z

Intentionally merged with a few CLI TODOs:

we should test bump and restore commands, for which I added ledger entry pattern match which I am not 100% will work
there is still some code pending clarification all: add support for new state expiration ledger entries #913 (comment) (waiting for your reply @willemneal :) )

sisuresh · 2023-09-01T22:43:42Z

cmd/soroban-rpc/lib/preflight/src/fees.rs

-        .map(|c| c.encoded_key.len() as u32 + c.old_entry_size_bytes)
+        .map(
+            |c| {
+                let mut size = c.encoded_key.len() as u32 + c.old_entry_size_bytes;


Ah I missed this - We no longer need to count the key. @2opremio should I open a follow up PR?

Yeah, I was aware. I will fix it later on (it's not critical for the tesnet release since it will only add some overhead in the resource estimation)

But, if you want to open a PR, by all means! I was assuming you have enough on your plate already.

I'll leave it then since it's not critical.

2opremio requested a review from leighmcculloch September 1, 2023 08:28

2opremio force-pushed the state-expiration-updates branch 2 times, most recently from d9e0051 to 22f0dd5 Compare September 1, 2023 10:45

2opremio and others added 14 commits September 1, 2023 12:46

Bump Rust and Go dependencies

522ce05

Start migrating soroban-rpc

333f364

Fix get_ledger_changes() invocation

f3ddedb

wip migrating the soroban-cli (incomplete!)

13aae97

Appease rust

43f45c2

Bump core dependencies

3b76940

Bump Go dependencies again

7f8024a

Fix test comparisons

6e515b8

Fix BumpAndRestoreFootprint test

2781026

Improve restore test a bit further

f5a9803

Adjust tests further

1a58165

Fix typo

eb90f6b

Fix GetPreflight test

f1cd7c6

Get CLI to compile and fix most TODOs

0e414da

2opremio force-pushed the state-expiration-updates branch from 22f0dd5 to 0e414da Compare September 1, 2023 10:46

run cargo md-gen

ed9fb10

2opremio requested review from tsachiherman and tamirms September 1, 2023 10:58

Appease clippy

44bc323

2opremio mentioned this pull request Sep 1, 2023

all: add support for new expiration ledger entry stellar/go#5034

Merged

2opremio requested a review from paulbellamy September 1, 2023 11:33

Add comment about expiration entry efficiency

f6210f5

paulbellamy reviewed Sep 1, 2023

View reviewed changes

Address review feedback

ff1a045

tsachiherman reviewed Sep 1, 2023

View reviewed changes

Take a stab at extracting the spec from the state

315b6b1

2opremio force-pushed the state-expiration-updates branch from 2200d2f to 315b6b1 Compare September 1, 2023 13:29

tsachiherman reviewed Sep 1, 2023

View reviewed changes

tsachiherman mentioned this pull request Sep 1, 2023

soroban-rpc: evict temporary ledger entries #914

Closed

Merge branch 'main' into state-expiration-updates

b1519a4

2opremio enabled auto-merge (squash) September 1, 2023 15:20

paulbellamy approved these changes Sep 1, 2023

View reviewed changes

willemneal reviewed Sep 1, 2023

View reviewed changes

cmd/soroban-cli/src/utils.rs Outdated Show resolved Hide resolved

willemneal reviewed Sep 1, 2023

View reviewed changes

Include expiration entry size in read bytes

5887678

Minor tweaks

29a8791

2opremio mentioned this pull request Sep 1, 2023

soroban-rpc: get expiration ledger sequence at source #916

Merged

2opremio added 2 commits September 1, 2023 23:59

Address review feedback

bd66ea6

Fix integration test

ffcc55d

2opremio merged commit 135102e into main Sep 1, 2023
19 of 20 checks passed

2opremio deleted the state-expiration-updates branch September 1, 2023 22:34

sisuresh reviewed Sep 1, 2023

View reviewed changes

2opremio mentioned this pull request Sep 4, 2023

soroban-rpc: preflight: remove key sizes from read_bytes computation #922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all: add support for new state expiration ledger entries #913

all: add support for new state expiration ledger entries #913

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio commented Sep 1, 2023

tsachiherman left a comment

2opremio commented Sep 1, 2023

tsachiherman left a comment

2opremio commented Sep 1, 2023 •

edited

Loading

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023

sisuresh commented Sep 1, 2023

willemneal Sep 1, 2023

2opremio Sep 1, 2023

willemneal Sep 4, 2023

2opremio Sep 4, 2023 •

edited

Loading

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023 •

edited

Loading

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio commented Sep 1, 2023 •

edited

Loading

sisuresh Sep 1, 2023

2opremio Sep 1, 2023 •

edited

Loading

2opremio Sep 1, 2023

sisuresh Sep 1, 2023

all: add support for new state expiration ledger entries #913

all: add support for new state expiration ledger entries #913

Conversation

2opremio commented Sep 1, 2023 • edited Loading

What

Why

Known limitations

2opremio commented Sep 1, 2023

tsachiherman left a comment

Choose a reason for hiding this comment

2opremio commented Sep 1, 2023

tsachiherman left a comment

Choose a reason for hiding this comment

2opremio commented Sep 1, 2023 • edited Loading

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023

sisuresh commented Sep 1, 2023

willemneal Sep 1, 2023

Choose a reason for hiding this comment

2opremio Sep 1, 2023

Choose a reason for hiding this comment

willemneal Sep 4, 2023

Choose a reason for hiding this comment

2opremio Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023 • edited Loading

tsachiherman commented Sep 1, 2023

2opremio commented Sep 1, 2023 • edited Loading

2opremio commented Sep 1, 2023 • edited Loading

sisuresh Sep 1, 2023

Choose a reason for hiding this comment

2opremio Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

2opremio Sep 1, 2023

Choose a reason for hiding this comment

sisuresh Sep 1, 2023

Choose a reason for hiding this comment

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio Sep 4, 2023 •

edited

Loading

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio commented Sep 1, 2023 •

edited

Loading

2opremio Sep 1, 2023 •

edited

Loading