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

soroban-simulation: simulate_invoke_host_function_op sometimes returns empty LedgerEntryDiffs #1448

Open
2opremio opened this issue Aug 23, 2024 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@2opremio
Copy link
Contributor

2opremio commented Aug 23, 2024

What version are you using?

soroban-simulation 21.1.0 (through soroban-rpc)

What did you do?

See stellar/soroban-rpc#239

The transaction to simulate was AAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAQAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAGAAAAAAAAAABqcyWObpdzFnUSN332FhvsFmwI8FSWHo/kRyy069qW5AAAAAGZGVwbG95AAAAAAACAAAADQAAABR9sHVzOQysoq5eXe/f+3AfT9WolQAAAA0AAABBBJFhCrhmp0SLcac+YTROcq+SKautVIDzg59ppjN7hir1Imo00C0b0uDYr/KDd4duQdSCqtfVFGR5H2yMmg1zxrgAAAAAAAAAAAAAAAAAAAA=

What did you expect to see?

No empty (None None ) pairs in the modified_entries vector of InvokeHostFunctionSimulationResult

What did you see instead?

This code:

    let invoke_hf_result = simulate_invoke_host_function_op(
        auto_restore_snapshot.clone(),
        &network_config,
        &adjustment_config,
        &ledger_info,
        invoke_hf_op.host_function,
        auth_entries,
        &source_account,
        rand::Rng::gen(&mut rand::thread_rng()),
        enable_debug,
    )?;
    
    for e in invoke_hf_result.modified_entries.iter() {
        println!("{:?}, {:?}",e.state_before, e.state_after);
    }

Printed:

None, None
None, Some(LedgerEntry { last_modified_ledger_seq: 0, data: ContractData(ContractDataEntry { ext: V0, contract: Contract(Hash(0ec5924105a46f557f8de0acfeef2ec85c32621ca19cbf66524bb809d420d894)), key: Bytes(ScBytes(BytesM(7db07573390caca2ae5e5defdffb701f4fd5a895))), durability: Persistent, val: Bytes(ScBytes(BytesM(0491610ab866a7448b71a73e61344e72af9229abad5480f3839f69a6337b862af5226a34d02d1bd2e0d8aff28377876e41d482aad7d51464791f6c8c9a0d73c6b8))) }), ext: V0 })
None, Some(LedgerEntry { last_modified_ledger_seq: 0, data: ContractData(ContractDataEntry { ext: V0, contract: Contract(Hash(0ec5924105a46f557f8de0acfeef2ec85c32621ca19cbf66524bb809d420d894)), key: LedgerKeyContractInstance, durability: Persistent, val: ContractInstance(ScContractInstance { executable: Wasm(Hash(25c3c28fd0928648c66c7294aacd5277f5bd4ab63438dc33db924839776a5915)), storage: Some(ScMap(VecM([ScMapEntry { key: Symbol(ScSymbol(StringM(sw_v1))), val: Void }]))) }) }), ext: V0 })

Note the first None, None pair

@2opremio 2opremio added the bug Something isn't working label Aug 23, 2024
@dmkozh
Copy link
Contributor

dmkozh commented Aug 23, 2024

Upon investigation it seems that there is no issue here. While None, None case seems confusing, it conforms to the semantics of the diffs, which is:

  • The first value is None when entry wasn't present in storage before contract execution
  • The second value is None when entry wasn't present in storage after contract execution

When both values are None, it would mean that the key hasn't existed before contract execution, then there was a number of write operations for the corresponding key, but the key doesn't have a persisted value after contract execution. Note, that remove is a write operation as well, so removing a non-existent entry will also result in None, None diff (on a side note, this is a bit inefficient and can be optimized at the SDK level).

I think this is actually a pretty useful signal, as the user still needs to pay for an RW footprint entry, but doesn't actually persist it. There also might be bugs where the user expected a new entry to be created, but it hasn't been persisted for some reason. So the library shouldn't be modified, but we might want to flag this somehow in the response to raise the users' awareness.

@anupsdf
Copy link
Contributor

anupsdf commented Aug 28, 2024

Hi @2opremio, How about addressing this in the presentation layer instead of the simulation library itself like @dmkozh suggests above?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants