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 no longer creates an Env consistent with Env::default #1101

Closed
leighmcculloch opened this issue Oct 3, 2023 · 2 comments · Fixed by #1133
Closed

Env::from_snapshot no longer creates an Env consistent with Env::default #1101

leighmcculloch opened this issue Oct 3, 2023 · 2 comments · Fixed by #1133
Labels
bug Something isn't working

Comments

@leighmcculloch
Copy link
Member

What version are you using?

v20.0.0-rc2

What did you do?

Serialized an Env to a LedgerSnapshot, then reloaded it into an Env.

What did you expect to see?

The Env loaded with the same state as if Env::default() had been called and the ledger state loaded, meaning auth recording is off, and a default base prng seed is set.

What did you see instead?

An Env loaded with auth recording on and no default base prng seed.

Discussion

It looks like when we added new behavior, and removed auth recording, in Env::default:

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();

We forgot to make the same changes in Env::from_snapshot:

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
}

We should probably have Env::from_snapshot and Env::default share the same setup logic and the same storage type, where default is just an empty LedgerSnapshot.

@leighmcculloch leighmcculloch added the bug Something isn't working label Oct 3, 2023
@leighmcculloch leighmcculloch changed the title Env::from_snapshot no longer restores state Env::from_snapshot no longer creates an Env consistent with Env::default Oct 3, 2023
ElliotFriend added a commit to ElliotFriend/rs-soroban-sdk that referenced this issue Oct 9, 2023
I've noticed some inconsistencies with snapshot creation and use
where the various `*_entry_expiration` values are not set properly
when create an `Env` from a snapshot that did have those values
set.

This could possibly be related to stellar#1101, but I'm not sure, since
Leigh seemed more focused on prng.

Refs: stellar#1101
@leighmcculloch leighmcculloch self-assigned this Oct 9, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2023
I've noticed some inconsistencies with snapshot creation and use where
the various `*_entry_expiration` values are not set properly when create
an `Env` from a snapshot that did have those values set.

This could possibly be related to #1101, but I'm not sure, since Leigh
seemed more focused on prng.

Refs: #1101

### What

This commit updates the ledger-snapshot crate to also set the various
ledger entry expiration values, if they are set on the snapshot being
set in the environment.

### Why

When trying to load a ledger snapshot that I've created from an `Env`
with valid entry expiration values, the new Env uses the defaults (which
turn out to be `0`) for those expiration values.

I noticed trouble trying to run `env.register_contract()` or
`env.register_stellar_asset_contract()` in a test file after loading a
previously valid snapshot, and receive the error: `trying to bump past
max expiration ledger`. (That expiration ledger, being `0` ledgers from
now.)

### Known limitations

It's possible this is deliberate, and I'm misunderstanding how to use
snapshots. If that's the case, we should probably bolster documentation
around ledger snapshots to communicate that.
@leighmcculloch leighmcculloch removed their assignment Nov 1, 2023
@leighmcculloch
Copy link
Member Author

@brson You're welcome to take this, especially if it's blocking your work.

@brson
Copy link
Contributor

brson commented Nov 3, 2023

Yep. I will try to fix it.

brson added a commit to brson/rs-soroban-sdk that referenced this issue Nov 6, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 7, 2023
…e same (#1133)

### What

Make Env::default (when cfg testutils) and Env::from_snapshot configure
the environment with a source account, a prng seed, and the debug
diagnostics level. Do not turn on recording auth.

Fixes #1101

### Why

Users writing tests will expect a test environment to be configured
similarly regardless of how it was constructed. At present these two
constructors create differently-configured environments.

### Known limitations

na

---------

Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
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

Successfully merging a pull request may close this issue.

2 participants