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

Support for state override parameter in some RPC methods #7362

Draft
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

alexb5dh
Copy link
Contributor

@alexb5dh alexb5dh commented Aug 26, 2024

Resolves #4021, resolves #6120, resolves #7306.

Changes

  • Support for state override set in the following RPC methods:
    • eth_call, eth_estimategas
    • trace_call
    • debug_traceCall
  • Fixes double-serialization of some of the parameters in the RPC tests.
  • Makes trace_call, trace_callMany, and trace_rawTransaction to run on top of the specified block, instead of the previous one, similar to other clients.

What types of changes does your code introduce?

  • New feature (a non-breaking change that adds functionality)

Testing

Requires testing

  • Yes

If yes, did you write tests?

  • Yes

Documentation

Requires documentation update

  • Yes

Requires explanation in Release Notes

  • Yes, RPC changes should probably be included.

@alexb5dh alexb5dh self-assigned this Aug 26, 2024
@@ -24,4 +23,7 @@ public override TrieNode FindCachedOrUnknown(Hash256? address, in TreePath path,

public override byte[]? TryLoadRlp(Hash256? address, in TreePath path, Hash256 hash, ReadFlags flags = ReadFlags.None) =>
base.TryLoadRlp(address, in path, hash, flags) ?? store.TryLoadRlp(address, in path, hash, flags);

// TODO clarify is ClearCache is reliable enough to use
public void ResetOverrides() => ClearCache();
Copy link
Contributor Author

@alexb5dh alexb5dh Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on top of ClearCache says that it's added "to support testing", but it seems like a good way to reset state-override changes applied on top of OverlayTrieStore.

IReleaseSpec currentSpec,
AccountOverride accountOverride,
Address address)
{
if (accountOverride.Code is not null)
{
stateProvider.InsertCode(address, accountOverride.Code, currentSpec);
Copy link
Contributor Author

@alexb5dh alexb5dh Oct 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to prevent an "Account {address} is null when updating storage hash" error in StateProvider when overridden code writes to address storage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlegJakushkin maybe this solves some issues on StateRoot in eth_simulate?

@LukaszRozmej LukaszRozmej mentioned this pull request Oct 7, 2024
15 tasks
…verride

# Conflicts:
#	src/Nethermind/Nethermind.Evm/OverridableCodeInfoRepository.cs
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think it would be good to separate ReadOnlyTxProcessingEnv with OverridableTxProcessingEnv instead of changing the first.

  2. That double test serialization was also changed in different PR Avoid double serialization in RPC tests #7547

@@ -76,10 +76,11 @@ public NethermindApi(IConfigProvider configProvider, IJsonSerializer jsonSeriali
public IBlockchainBridge CreateBlockchainBridge()
{
ReadOnlyBlockTree readOnlyTree = BlockTree!.AsReadOnly();
OverridableWorldStateManager overridableWorldStateManager = new(DbProvider!, WorldStateManager!.TrieStore, LogManager);

// TODO: reuse the same trie cache here
ReadOnlyTxProcessingEnv readOnlyTxProcessingEnv = new(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe let's create OverridableTxProcessingEnv?

@@ -2,12 +2,14 @@
// SPDX-License-Identifier: LGPL-3.0-only

using Nethermind.Core.Crypto;
using Nethermind.Evm;
using Nethermind.Evm.TransactionProcessing;
using Nethermind.State;

namespace Nethermind.Consensus.Processing;

public class ReadOnlyTxProcessingScope(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe move to OverridableTxProcessingScope?

IReleaseSpec currentSpec,
AccountOverride accountOverride,
Address address)
{
if (accountOverride.Code is not null)
{
stateProvider.InsertCode(address, accountOverride.Code, currentSpec);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlegJakushkin maybe this solves some issues on StateRoot in eth_simulate?

src/Nethermind/Nethermind.Facade/IBlockchainBridge.cs Outdated Show resolved Hide resolved
Comment on lines +32 to +34
ITrieStore trieStore = overlayTrieStore;
if ((forWarmup as IPreBlockCaches)?.Caches is { } preBlockCaches)
trieStore = new PreCachedTrieStore(trieStore, preBlockCaches.RlpCache);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ITrieStore trieStore = overlayTrieStore;
if ((forWarmup as IPreBlockCaches)?.Caches is { } preBlockCaches)
trieStore = new PreCachedTrieStore(trieStore, preBlockCaches.RlpCache);
ITrieStore trieStore = (forWarmup as IPreBlockCaches)?.Caches is { } preBlockCaches ? new PreCachedTrieStore(trieStore, preBlockCaches.RlpCache) : overlayTrieStore;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trieStore will be undefined in new PreCachedTrieStore(trieStore, preBlockCaches.RlpCache)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants