fix(tree): make state methods work for historical blocks

[Rjected]

This fixes #11251 by allowing fetching state from disk OR from in-memory state.

Closes: #10875
Closes: #8305

[rkrasiuk]

should we error here instead of providing only partial response to range query?

[Rjected]

I think so, although current range methods on the db provider provide partial response over erroring, so we may want to refactor those separately

[rkrasiuk]

akin to checks in historical provider this method should check lowest available history blocks in case of a pruned node

[Rjected]

Now done in ChangeSetReader and StorageChangeSetReader, ensuring we don't create a db provider without checking prune checkpoints first: d7e6542

[joshieDo]

• can we add to the docs that it's only for historical (or persistence storage), if thats the case? doesn't seem to be the case
• this is creating a ton of new database transactions. a lil worried that this might also have some unwanted effects since they do not share the same view. if this is only for historical (is it?), maybe it would be better moved to DatabaseProvider?

[Rjected]

can we add to the docs that it's only for historical (or persistence storage), if thats the case?

Yes, this is only when we're using a databaseprovider, so only for persisted storage

this is creating a ton of new database transactions. a lil worried that this might also have some unwanted effects since they do not share the same view. if this is only for historical (is it?), maybe it would be better moved to DatabaseProvider?

You mean the provider calls which hit the DB? ie:

            let block_body = self
                .block_body_indices(block_num)?
                .ok_or(ProviderError::BlockBodyIndicesNotFound(block_num))?;
// ^ one db tx
            let changeset =
                self.account_block_changeset(block_num)?.into_iter().map(|elem| (block_num, elem));
// ^ may or may not be one db tx
            let changeset = self.storage_changeset(block_num)?;
// ^ also may or may not be one db tx

[rkrasiuk]

@Rjected a valid point from @joshieDo here. in scope of this function we perform a ton of lookups by block number, but there is no consistency guarantee here

[Rjected]

This now uses one database tx

[rkrasiuk]

what should we do with requests here?

[Rjected]

We should really be fetching them using the RequestsProvider, but that method requires a timestamp, which I'm not sure how to deal with yet

[mattsse]

whether we actually need to store requests is still tbd so imo this is fine, but please track so we don't forget

[Rjected]

#11437

[fgimenez]

awesome work, lgtm!

[joshieDo]

might be wrong but this feels unsafe when it touches in-memory ranges.

every query to self.canonical_in_memory_state.get/fetch/state should be viewed as a DatabaseTransaction in which the view might change in the next one.

Example:

We fetch block changesets on a loop which underneath calls self.canonical_in_memory_state.state_by_number . every call is basically another view decoupled from the previous call.

Example:

main: b1,b2,b3
reorg: b1',b2',b3'

we might end up with weird changeset lists like:

b1, b2', b3'

[Rjected]

might be wrong but this feels unsafe when it touches in-memory ranges.

When this is used outside of the tree, yes, calling get_state in a loop could result in different views. I think most of the provider methods suffer from this. In those cases we need to provide a safer API that is aware of the CanonicalInMemoryState locks. For example, right now it is unsafe to call most combinations of providers in sequence. For a safer API, do we want to put the provider methods behind a lock? Similar to how the DatabaseProvider works:

let blockchain_provider = BlockchainProvider::new(/* stuff */);
let provider_ro = blockchain_provider.read(); // acquires ro `CanonicalInMemoryState` lock
for block in blocks {
    let state = provider_ro.get_state(block);
}

[mattsse]

pending @rkrasiuk

[mattsse]

whether we actually need to store requests is still tbd so imo this is fine, but please track so we don't forget

[mattsse]

-.-

[rkrasiuk]

@Rjected a valid point from @joshieDo here. in scope of this function we perform a ton of lookups by block number, but there is no consistency guarantee here

[mattsse]

is this good to go?

imo the memory concerns that @joshieDo raised are valid but not an issue for the engine, because by the time this is called no disk io should be in progress.
I'd like to get this in and then look how to make it sound

[mattsse]

Suggested change

	///
	/// This gives the provider back if creating the historical state provider is not successful.

will be done separately