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

Fluffy: Portal EVM call #3119

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

Fluffy: Portal EVM call #3119

wants to merge 16 commits into from

Conversation

bhartnett
Copy link
Contributor

@bhartnett bhartnett commented Mar 4, 2025

This Portal EVM implementation uses the Nimbus in-memory EVM to execute transactions using the portal state network state data. Currently only call is supported.

Rather than wire in the portal state lookups into the EVM directly, the approach taken here is to optimistically execute the transaction multiple times with the goal of building the correct access list so that we can then lookup the required state from the portal network, store the state in the in-memory EVM and then finally execute the transaction using the correct state. The Portal EVM makes use of data in memory during the call and therefore each piece of state is never
fetched more than once. We know we have found the correct access list if it doesn't change after another execution of the transaction.

The assumption here is that network lookups for state data are generally much slower than the time it takes to execute a transaction in the EVM and therefore executing the transaction multiple times should not significantly slow down the
call given that we gain the ability to fetch the state concurrently.

There are multiple reasons for choosing this approach:

  • Firstly updating the existing Nimbus EVM to support using a different state backend (portal state in this case) is difficult and would require making non-trivial changes to the EVM.
  • This new approach allows us to look up the state concurrently in the event that multiple new state keys are discovered after executing the transaction. This should in theory result in improved performance for certain scenarios. The default approach where the state lookups are wired directly into the EVM gives the worst case performance because all state accesses inside the EVM are completely sequential.

@bhartnett bhartnett marked this pull request as draft March 4, 2025 11:14
@bhartnett bhartnett requested a review from kdeme March 4, 2025 11:27
@@ -217,7 +218,8 @@ proc init(com : CommonRef,
# By default, history begins at genesis.
com.startOfHistory = GENESIS_PARENT_HASH

com.initializeDb()
if initializeDb:
com.initializeDb()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this configurable so that it can be disabled in Fluffy which doesn't need it. Removes the logs also which are not relevant in the portal context.

if not equalsStorageMode(kd1.storageKeys, kd2.storageKeys):
return false

return true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is required to check if the keys have changed. Is there a better way to compare case object ref types?


# Store fetched state in the in-memory EVM
for q in accountQueries:
let acc = (await q.accFut).valueOr:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When awaiting these futures here the internal future type raises a CatchableError when the async proc called actually raises a CancelledError. Not sure what is causing this and how I should handle it.

if historyNetwork.isSome() and stateNetwork.isSome():
Opt.some(PortalEvm.init(historyNetwork.get(), stateNetwork.get()))
else:
Opt.none(PortalEvm),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PortalEvm is created only when both the history network and the state network are running. Should we start the portal evm using an alternative method such as having a separate cli argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually I do see the PortalEvm as an optional part that is not required to run a pure portal node. That is a node that only does its duties on a Portal Network protocol level and does not provide higher level API such as the EL JSON API. (We could even rewrite the types to have PortalClient or PortalLightClient which has the subtypes of PortalNode and PortalEvm.)

In that sense it should be possible to run a portal node with history/state/beacon subnetworks enabled but the evm not running. This could be achieved with an extra cli argument.

Another way to achieve this (with perhaps better UX?) could be to enable it based on enabling of linked functionality. I believe in this case that would just mean in case the EL (eth) JSON-RPC API gets enabled, which it currently default does.

This would avoid having to add additional "Not supported" errors for certain JSON-RPC endpoints in case you would use a separate extra cli argument.

If we would go that route, then I am uncertain whether we should then keep eth as default enabled, but I'm leaning towards yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conceptually I do see the PortalEvm as an optional part that is not required to run a pure portal node. That is a node that only does its duties on a Portal Network protocol level and does not provide higher level API such as the EL JSON API. (We could even rewrite the types to have PortalClient or PortalLightClient which has the subtypes of PortalNode and PortalEvm.)

In that sense it should be possible to run a portal node with history/state/beacon subnetworks enabled but the evm not running. This could be achieved with an extra cli argument.

Another way to achieve this (with perhaps better UX?) could be to enable it based on enabling of linked functionality. I believe in this case that would just mean in case the EL (eth) JSON-RPC API gets enabled, which it currently default does.

This would avoid having to add additional "Not supported" errors for certain JSON-RPC endpoints in case you would use a separate extra cli argument.

If we would go that route, then I am uncertain whether we should then keep eth as default enabled, but I'm leaning towards yes.

Sure, I'll remove it from PortalNode and only create the PortalEvm instance when eth rpc is enabled. I think this is the simplest way for now considering that it will only be used in the eth rpc at this point. In the future when we implement the transaction gossip sub-network the requirements may change and at that point it could be moved somewhere else if/when needed.

# maxFeePerGas*: Opt[Quantity] # (optional) MaxFeePerGas is the maximum fee per gas offered, in wei.
# maxPriorityFeePerGas*: Opt[Quantity] # (optional) MaxPriorityFeePerGas is the maximum miner tip per gas offered, in wei.
# value*: Opt[UInt256] # (optional) Integer of the value sent with this transaction.
# nonce*: Opt[Quantity] # (optional) integer of a nonce. This allows to overwrite your own pending transactions that use the same nonce
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The eth_call implementation here is not the core focus of this PR. It is simply used for some basic manual testing. I will create another future PR which refines and further tests the various eth_call scenarios.

@@ -262,10 +262,10 @@ proc processContentLoop(n: StateNetwork) {.async: (raises: []).} =
proc statusLogLoop(n: StateNetwork) {.async: (raises: []).} =
try:
while true:
await sleepAsync(60.seconds)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the sleep so that during startup the logs look better/cleaner.

@bhartnett bhartnett mentioned this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants