stellar / rs-soroban-env

Rust environment for Soroban contracts.
Apache License 2.0
60 stars 40 forks source link

soroban-simulation: simulate_invoke_host_function_op sometimes returns empty LedgerEntryDiffs #1448

Open 2opremio opened 3 weeks ago

2opremio commented 3 weeks ago

What version are you using?

soroban-simulation 21.1.0 (through soroban-rpc)

What did you do?

See https://github.com/stellar/soroban-rpc/issues/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

dmkozh commented 3 weeks ago

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:

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 commented 2 weeks ago

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