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

Env::from_snapshot should do additional testutils setup? #1127

Closed
brson opened this issue Oct 31, 2023 · 2 comments
Closed

Env::from_snapshot should do additional testutils setup? #1127

brson opened this issue Oct 31, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@brson
Copy link
Contributor

brson commented Oct 31, 2023

What problem does your feature solve?

In many of my tests I use snapshots to destroy and reconstruct the Env from scratch to simulate the advancement of time, e.g.

fn reset_env(prev_env: &Env, advance_time: u64) -> Env {
    let mut snapshot = prev_env.to_snapshot();
    snapshot.sequence_number += 1;
    snapshot.timestamp = snapshot.timestamp.saturating_add(advance_time);

    let env = Env::from_snapshot(snapshot);
    env.budget().reset_unlimited();

    env
}

The reason I do this is because I want the most accurate emulation of what happens in production, with the Env being destroyed and recreated every transaction, and I don't think the common approach of mutating the sequence_number and timestamp without destroying the whole Env does that. In particular I want to ensure that:

  • events are cleared
  • the internal memory of the Host is cleared of all old Vals

Completely recreating the Env is the obvious way to ensure it's clean for the next transaction.

(If I really should not be destroying and recreating the Env during tests let me know, but it seems reasonable).


I have recently discovered that this doesn't do what I expect: with testutils configured Env::default (default_with_testutils) enables several configurations for convenience that Env::from_snapshot does not do. It seems that since from_snapshot is also a testutils function for creating an Env, it might be appropriate for it to do the same setup steps as Env::default.

In particular, Env::default turns on debug diagnostics and sets the prng seed; while Env::from_snapshot does not:

    fn default_with_testutils() -> Env {

        // ... snip details ...

        let rf = Rc::new(EmptySnapshotSource());
        let storage = internal::storage::Storage::with_recording_footprint(rf);
        let budget = internal::budget::Budget::default();
        let env_impl = internal::EnvImpl::with_storage_and_budget(storage, budget.clone());
        env_impl
            .set_source_account(xdr::AccountId(xdr::PublicKey::PublicKeyTypeEd25519(
                xdr::Uint256(random()),
            )))
            .unwrap();
        env_impl
            .set_diagnostic_level(internal::DiagnosticLevel::Debug)
            .unwrap();
        env_impl.set_base_prng_seed([0; 32]).unwrap();
        let env = Env {
            env_impl,
            snapshot: None,
        };

        env.ledger().set(internal::LedgerInfo {
            protocol_version: 20,
            sequence_number: 0,
            timestamp: 0,
            network_id: [0; 32],
            base_reserve: 0,
            min_persistent_entry_ttl: 4096,
            min_temp_entry_ttl: 16,
            max_entry_ttl: 6_312_000,
        });

        env
    }

    pub fn from_snapshot(s: LedgerSnapshot) -> Env {
        let info = s.ledger_info();

        let rs = Rc::new(s.clone());
        let storage = internal::storage::Storage::with_recording_footprint(rs.clone());
        let budget = internal::budget::Budget::default();
        let env_impl = internal::EnvImpl::with_storage_and_budget(storage, budget.clone());
        env_impl.switch_to_recording_auth(true).unwrap();

        let env = Env {
            env_impl,
            snapshot: Some(rs.clone()),
        };
        env.ledger().set(info);
        env
    }

What would you like to see?

Env::from_snapshot should also turn on debug diagnostics and the prng.

What alternatives are there?

My assumptions here about how from_snapshot should work may just be wrong.


cc stellar/soroban-examples#286 (comment)

@leighmcculloch
Copy link
Member

leighmcculloch commented Oct 31, 2023

I think this is the same as:

This is definitely a bug.

@leighmcculloch leighmcculloch added the bug Something isn't working label Oct 31, 2023
@brson
Copy link
Contributor Author

brson commented Oct 31, 2023

Dupe.

@brson brson closed this as completed Oct 31, 2023
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

2 participants