Fix GetAsOf() for commitment , so that it handles dereferencing

[antonis19]

Fixes #17488, and is a prerequisite for fixing:
#17561

In GetAsOf() if a value is not found in history, DomainRoTx.GetLatest() is called. However, for commitment domain AggregatorRoTx.GetLatest() needs to be called instead, which handles foreign key dereferencing (account and storage offsets).

[yperbasis]

Please add a test

[antonis19]

@yperbasis just added some more test cases. The last test case will fetch branch data at blockNum=5 via getLatestFromDB() because that prefix was last updated at blockNum=4, so even though blockNum=5 is historical, because there hasn't been any further update for this key the value is still in latest domain.

So the test case tests exactly the scenario in the PR description.

[AskAlexSharov]

1. this condition was introduced in historical eth_getProof fails on some requests on hoodi #15924 -> historical eth_getProof fails on some requests on hoodi #15924 and it also has link to 4 other issues historical eth_getProof fails on some requests on hoodi #15924 (comment) need to re-check them?

2. I guess we need some test which will create commitment.kv file with refferences to acc.kv - so AggregatorRoTx.replaceShortenedKeysInBranch will be triggered and reads from commitment.v/commitment.kv files will happen.

I see that rpc package tests do not trigger replaceShortenedKeysInBranch method:

Maybe rpc package tests not the best place for it (up to you). Also we have TestAggregatorV3_Merge test - which does call domains.GetLatest(kv.CommitmentDomain and replaceShortenedKeysInBranch but doesn't call lookupByShortenedKey. Also weFuzz_AggregatorV3_MergeValTransform

[awskii]

reading from commitment domain required to be done on aggregator or shared domain level - this is lowest possible level where we have access to other domains as well,but here if we read the value, it will contain accounts and storage file pointers and will become error on another level - while unfolding the branch

[antonis19]

reading from commitment domain required to be done on aggregator or shared domain level - this is lowest possible level where we have access to other domains as well,but here if we read the value, it will contain accounts and storage file pointers and will become error on another level - while unfolding the branch

So what is the change you would like me to make? Even with this if guard not removed, I don't see the read from shared domains happening.

As I see it this is a separate issue, I don't see any fallback to shared domains.

[awskii]

please move !ok as first condition - it just almost slit from me when i started CR

[awskii]

but with !ok && commitmentDomain it looks cleaner. Also you have to make sure [Commitment].GetAsOf returns nil, false right before domain reading with proper comment about branch plain keys dereferencing bad reads protection.

Currently this fix wount work - OK=true in case of successful reading from commtiment domain - value will contain references and this fallback will not take place

[antonis19]

I thought the theory was that that when [Commitment].GetAsOf() returns nil, false then we fall back to func (at *AggregatorRoTx) GetLatest() . I already put back this code in DomainRoTx.GetAsOf() :

	if dt.name == kv.CommitmentDomain {
		// we need to dereference commitment keys to get actual value. DomainRoTx itself does not have
		// pointers to storage and account domains to do the reference. Aggregator tx must be called instead
		return nil, false, nil
	}

Are you saying that if Commitment].GetAsOf() returns ok = true from history, that still needs special handling???

[awskii]

yes indeed that was introduced way ago 0774823
no problem then

[awskii]

but with !ok && commitmentDomain it looks cleaner. Also you have to make sure [Commitment].GetAsOf returns nil, false right before domain reading with proper comment about branch plain keys dereferencing bad reads protection.

Currently this fix wount work - OK=true in case of successful reading from commtiment domain - value will contain references and this fallback will not take place

[awskii]

yes indeed that was introduced way ago 0774823
no problem then