Refactor Blockhash lib

[Amxx]

Looking at #5642, I noticed that the if statement was quite complexe. It comes with an implicit expectation that the static call (right part of the if) is executed BEFORE the returndatasize (left part of the if). While that expectation might be correct today, I'm not sure the compiler garantees it will stay that way.

Considering that the if does not trigger a revert if the staticcall fails, I believe the proposed version is just as effective, and IMO easier to read. All tests (unit and fuzzing) pass just fine.

Given that an MSTORE is just 3 gas, this version is also cheaper !

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

⚠️ No Changeset found

Latest commit: da5c867

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Amxx]

Note: I was also thinking that doing

return distance < 256 ? blockhash(blockNumber) : _historyStorageCall(blockNumber)

Would be more future proof is the history is ever extended beyound 8192 blocks. That passes unit tests, but not fuzzing. Do we know why ?

[arr00]

Note: I was also thinking that doing

return distance < 256 ? blockhash(blockNumber) : _historyStorageCall(blockNumber)

Would be more future proof is the history is ever extended beyound 8192 blocks. That passes unit tests, but not fuzzing. Do we know why ?

Should be

return distance < 257 ? blockhash(blockNumber) : _historyStorageCall(blockNumber)

[arr00]

Looking at #5642, I noticed that the if statement was quite complexe. It comes with an implicit expectation that the static call (right part of the if) is executed BEFORE the returndatasize (left part of the if). While that expectation might be correct today, I'm not sure the compiler garantees it will stay that way.

@ernestognw and I discussed this previously and found that this yul behaviour is pretty well documented and not just an implementation detail (documented here). Will defer to ernesto on this.

[arr00]

#5642 (comment)

[ernestognw]

Yeah we did check the order of execution but I did find it a pretty confusing behavior. Overall I agree with simplifying the if statement and the implementation @Amxx proposes. Just updated with @arr00 changes:

return distance < 257 ? blockhash(blockNumber) : _historyStorageCall(blockNumber)