Nimbus Verified Proxy #3100

chirag-parmar · 2025-02-21T02:26:30Z

No description provided.

bhartnett · 2025-02-21T05:14:10Z

The approach you've taken here is interesting.

Unfortunately, it will only work if the RPC provider returns the correct access list. We don't want to trust the RPC provider and so we can't really trust that they will provide a correct access list. They could for example provide you with an access list that will cause the eth_call result to be completely different, even though you are verifying the account and storage proofs of each account and slot there is no way to verify that you are looking up the correct pre-state before the transaction is executed.

I think using the access list purely as a performance optimization would be ok as long as the EVM itself still looks up the correct state during transaction execution. In this scenarios the worst that the RPC provider could do is a DOS by sending you a very large fake access list but since the RPC provider can already effectively DOS your application by not responding to requests this is likely acceptable.

bhartnett · 2025-02-21T14:26:15Z

Unfortunately, it will only work if the RPC provider returns the correct access list. We don't want to trust the RPC provider and so we can't really trust that they will provide a correct access list. They could for example provide you with an access list that will cause the eth_call result to be completely different, even though you are verifying the account and storage proofs of each account and slot there is no way to verify that you are looking up the correct pre-state before the transaction is executed.

You might be able to solve this problem by building up the access list yourself by executing the in memory EVM multiple times, each time collecting more of the access list keys and state until eventually executing the transaction against the full state which will give you the result for eth_call.

Here is the algorithm:

- Fetch code to be executed
- while the evm execution is not successful or the touched account,code,slots have changed since the last execution:
    - execute the code using in memory EVM
    - collect the touched account,code,slots from the evm state
    - fetch all touched account,code,slots concurrently and pass into a new evm state

The last execution will be successful and should be using the correct keys.

You can use the makeMultiKeys function in ledger.nim to collect the touched account keys and storage slots from the EVM state after executing the transaction. You might also be able to use the accessList functions for this.

nimbus_verified_proxy/rpc/rpc_eth_api.nim

kdeme · 2025-05-06T10:55:51Z

I think it would be a good plan to already put this PR in a cleaned-up state that would be ready to review / ready to merge.

Your changes into using the json-rpc API instead of the libp2p enabled block cache right now is already a good improvement that can go into the code base. No need to wait for other improvements/features, those can go in other PRs (this avoids to-big-to-review PRs and brings quicker improvements to the web3 ~~verifier~~ purifier)

kdeme · 2025-05-13T08:57:26Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

+  # load constants and metadata for the selected chain
+  let metadata = loadEth2Network(config.eth2Network)
+
+  # initialze verified proxy


Suggested change

# initialze verified proxy

# initialize verified proxy

kdeme · 2025-05-13T08:57:35Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

    chainId = getConfiguredChainId(metadata)
+    authHooks = @[httpCors(@[])] # TODO: for now we serve all cross origin requests
+    # TODO: write a comment


kdeme · 2025-05-13T08:57:45Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

  template cfg(): auto =
    metadata.cfg

+  # initiialize beacon node genesis data, beacon clock and forkDigests


Suggested change

# initiialize beacon node genesis data, beacon clock and forkDigests

# initialize beacon node genesis data, beacon clock and forkDigests

kdeme · 2025-05-13T09:01:09Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

-  verifiedProxy.installEthApiHandlers()
-
-  info "Listening to incoming network requests"
+  # find out what this does


This registers the basic protocol that allows for ping/getMetadata/goodbye for a node in libp2p.
I think it should remain available for basic communication with nodes on the network.

Eventually when you move to add the REST API sync version, this can be either be removed if we are to only support REST. Or if we want to support both modes, this gets disabled in case of REST mode.

I think I forgot the comment in there. Thank you for the explanation

kdeme · 2025-05-13T09:05:06Z

nimbus_verified_proxy/types.nim

+#   * MIT license (license terms in the root directory or at https://opensource.org/licenses/MIT).
+#   * Apache v2 license (license terms in the root directory or at https://www.apache.org/licenses/LICENSE-2.0).
+# at your option. This file may not be copied, modified, or distributed except according to those terms.
+


add {.push raises: [].}

nimbus_verified_proxy/nimbus_verified_proxy.nim

kdeme · 2025-05-13T09:37:45Z

nimbus_verified_proxy/header_store.nim

+  results
+
+type HeaderStore* = ref object
+  headers: LruCache[Hash32, Header]


I guess it works the use an LruCache because of the way you have implemented it (using peek for the getters and scanning over the cache before add), but to me it does feel like a non-intuitive way to use an LruCache.

It is basically being used/selected mostly for its "automatic" pruning of the oldest headers here, but not at all for keeping its most "touched" content, hence the use of only peek and the requirement to add only new data.

The required full scan on add also makes the efficient add of the lru cache undone, but with the set max value of 64 that's probably not really an issue currently.

This implementation is also easy to make mistakes against in future changes, so it should at least have a big warning on how to use (already has this warning on the add currently).

FYI: I do realize that the original BlockCache was implemented in a similar way, and that you improved it somewhat by using peek (peek was not part of the API at creation of BlockCache I believe). But as you are reworking this I mostly wanted to point out that I do not think it is something we necessarily need to stick with as solution for this cache.

Also, as @bhartnett pointed out, there is an issue with pruning the number-to-hashes-mapping table, which will be more difficult to solve cleanly as the LRU prunes behind the scenes (without hook into it). But I guess you could delete numbers lower than n - max size on the add of a new header.

bhartnett · 2025-05-14T05:25:07Z

We have existing users of the Nimbus Verified Proxy and so we need to be careful not to break any of the functionality, so in my opinion this PR is too big to be merged, especially because we don't have a staging branch etc. I also noticed that there are no tests added.

I would suggest treating this as a draft PR to get feedback and then create smaller PRs with smaller subsets of the features and have some unit tests covering the changes in each PR. Just my thoughts, curious to hear what others in the team think. @kdeme @tersec

Another option I guess would be to create a staging branch where the changes can be tested a bit longer before being merged into master but this would not be my preferred approach.

bhartnett · 2025-05-14T05:41:34Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

    chainId = getConfiguredChainId(metadata)
+    authHooks = @[httpCors(@[])] # TODO: for now we serve all cross origin requests


Why did you need to introduce this CORS change when it previously wasn't needed?

So I rearranged the boot code for the verified proxy to have a proper initialization flow. I did that to understand the verified proxy's workings and the interplay of the light client with it.

The CORS change is just a rearrangement diff i.e it existed before.

cc: @kdeme

bhartnett · 2025-05-14T05:45:20Z

fluffy/evm/async_evm.nim

@@ -108,6 +108,9 @@ proc init*(

  AsyncEvm(com: com, backend: backend)

+proc init*(T: type AsyncEvm, backend: AsyncEvmStateBackend, com: CommonRef): T =
+  AsyncEvm(com: com, backend: backend)


You shouldn't need to pass in a CommonRef directly. I'm not sure if this could lead to problems if the in memory database used by the evm isn't empty. I did provide the NetworkId parameter in the above init proc which is the one you should use.

I did not want to initialize two instances CommonRef. More so because the other instance is merely used for gas price APIs (which internally use CommonRef for fork calculation, I assume)

We probably don't need an instance of CommonRef in the NVP just for determining a fork. After looking at that area of code, it appears that you should be able to do something like this to get the fork without creating an instance of CommonRef:

let forkTransitionTable = config.toForkTransitionTable() # store this somewhere precomputed ToEVMFork[toHardFork(forkTransitionTable, forkDeterminationInfo(header))]

I haven't tried this by it should be possible even with some small changes to Nimbus code if needed.

bhartnett · 2025-05-14T05:53:47Z

nimbus_verified_proxy/header_store.nim

+
+type HeaderStore* = ref object
+  headers: LruCache[Hash32, Header]
+  hashes: Table[base.BlockNumber, Hash32]


It appears that the hashes table only grows and the entries never get deleted. Maybe you should use a LRUCache here as well.

In retrospect, that makes a lot of sense, i'll add it in. Initially, it did not hurt to let it grow because even a million blocks would amount to only ~32MB but since we are probably also aiming for resource constrained embedded devices and not just smartphone wallets it makes sense to add a limit. Regardless of the resource constraints, it still does make sense to add a limit and just set it to a high number

Yeah it can hold more items but it should still be bounded.

bhartnett · 2025-05-14T06:00:36Z

nimbus_verified_proxy/header_store.nim

+  for h in self.headers.keys:
+    hash = h
+
+  self.headers.peek(hash)


Why not just store the earliest and latest block number (or hash) and then use them to return the earliest and latest headers.

bhartnett · 2025-05-14T06:03:04Z

nimbus_verified_proxy/nimbus_verified_proxy.nim

+  let com = CommonRef.new(
+    DefaultDbMemory.newCoreDbRef(),
+    taskpool = nil,
+    config = chainConfigForNetwork(chainId),


Note that chainId is not always the same as networkId. This chainConfigForNetwork function expects a networkId so I'm not sure if there might be an issue here.

bhartnett · 2025-05-14T06:05:55Z

nimbus_verified_proxy/rpc/receipts.nim

+export results, stint, hashes_rlp, accounts_rlp, eth_api_types
+
+template rpcClient(vp: VerifiedRpcProxy): RpcClient =
+  vp.proxy.getClient()


This rpcClient template is defined in each file. Would be better to define this in just one place and then import.

bhartnett · 2025-05-14T06:13:04Z

nimbus_verified_proxy/rpc/accounts.nim

+template rpcClient(vp: VerifiedRpcProxy): RpcClient =
+  vp.proxy.getClient()
+
+proc getAccountFromProof(


It appears that this function is now defined in two places. The other one in validate_proof.nim

bhartnett · 2025-05-14T06:36:31Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

+      raise newException(ValueError, "to address is required")
+
+    if blockTag.kind == bidAlias:
+      raise newException(ValueError, "tag not yet implemented")


Why is tag not implemented? Looks like this was just copied from Fluffy.

Indeed most of the code for evm execution was taken directly from fluffy. I'm fairly sure I removed this bit of code, but I think I lost the diff while stashing some changes for the Pr on AsyncEVM. Will add it back in(i.e. remove the code)

bhartnett · 2025-05-14T06:37:06Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

+      raise newException(ValueError, "to address is required")
+
+    if blockTag.kind == bidAlias:
+      raise newException(ValueError, "tag not yet implemented")


Same here. Tag should be supported.

bhartnett · 2025-05-14T06:42:03Z

nimbus_verified_proxy/rpc/evm.nim

+  ../../fluffy/evm/async_evm, ../../fluffy/evm/async_evm_backend, ./accounts, ../types
+
+export async_evm, async_evm_backend
+


In order to get reasonable performance you should create a cache for the most recently fetched accounts, slots and code. This should be populated after validation when fetching this data from the downstream RPC provider.

bhartnett · 2025-05-14T06:45:50Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

+      optimisticStateFetch = optimisticStateFetch.valueOr:
+        true
+
+    let callResult = (await vp.evm.call(header, tx, optimisticStateFetch)).valueOr:


As a performance optimization, before executing call on the evm, I would suggest calling eth_createAccessList on the downstream RPC provider and then using the returned keys to concurrently fetch the accounts and storage slots to populate a cache which should be used by the AsyncEvm backend.

tersec · 2025-05-14T08:34:38Z

We have existing users of the Nimbus Verified Proxy and so we need to be careful not to break any of the functionality, so in my opinion this PR is too big to be merged, especially because we don't have a staging branch etc. I also noticed that there are no tests added.

I would suggest treating this as a draft PR to get feedback and then create smaller PRs with smaller subsets of the features and have some unit tests covering the changes in each PR. Just my thoughts, curious to hear what others in the team think. @kdeme @tersec

Another option I guess would be to create a staging branch where the changes can be tested a bit longer before being merged into master but this would not be my preferred approach.

I'd echo this. Also, yes, it's possible to keep it in staging longer, unless it needs to be a larger PR, it would better to split out individual changes from this to review and merge, then this branch can be updated to reflect those being merged.

kdeme · 2025-05-14T08:35:26Z

Just my thoughts, curious to hear what others in the team think.

Verified proxy is definitely lacking a good set of tests which currently makes it difficult making changes without breaking things. In the past the only testing done was basically running it with a wallet (e.g. Metamask) and testing some end-to-end functionality there.

Now as going forward and moving this "PoC" into a properly maintained and developed product, and adding more functionality to it, we should definitely have more unit testing (and especially for this newly added functionality). To be fair, the current version does not really have much functionality, libp2p consensus light client, which is already tested, some basic proving and this block cache.
In terms of testing the JSON-RPC functionality (end-to-end tests), I think we can have tests with some static response data (mock the response basically) and load that in the tests just to verify the underlying logic of our JSON-RPC methods.

Regarding smaller PRs: Yes, I always prefer to have features split up (easier to review, to test, to revert, etc.). Practically for this PR I think the block cache changes could be a PR that gets merged (once comments are addressed), and the addition and usage of async EVM could be another (or even several depending on which API method starts to use them already). I noticed that there is currently an issue that causes a lot of the JSON-RPC code to be commented. Separating PRs would allow us to already get some of this functionality merged without that commented code holding it back.

A staging branch I personally do not prefer, in general, but also in this case specifically because we do not have a way to really verify/test that it works for users. Aside from using it ourselves for some time.
Note that this sort of also applies to the big vs smaller PRs comment, but with that difference that with smaller PRs we can potentially diagnose issues quicker (git bisect, etc.) and revert them cleaner.
And in a way, our master is basically the staging branch, as we are not doing releases yet :)

tersec · 2025-05-14T08:35:40Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

-    let
-      blk = lcProxy.getBlockByTagOrThrow(quantityTag)
-      blockNumber = blk.number.uint64
+  #  vp.proxy.rpc("eth_getUncleCountByBlockNumber") do(


Large chunks of commented-out code shouldn't usually be merged

tersec · 2025-05-14T08:38:21Z

nimbus_verified_proxy/rpc/blocks.nim

+  else:
+    return ok(base.BlockNumber(distinctBase(blockTag.number)))
+
+proc convHeader(blk: eth_api_types.BlockObject): Header =


https://status-im.github.io/nim-style-guide/language.proc.html

Prefer func - use proc when side effects cannot conveniently be avoided.

But there appear to be no side effects here to begin with.

tersec · 2025-05-14T08:39:14Z

nimbus_verified_proxy/header_store.nim

@@ -0,0 +1,141 @@
+# nimbus_verified_proxy
+# Copyright (c) 2022-2025 Status Research & Development GmbH


How is this file from 2022-2024? It pops into existence in this PR.

tersec · 2025-05-14T08:39:43Z

nimbus_verified_proxy/rpc/accounts.nim

@@ -0,0 +1,173 @@
+# nimbus_verified_proxy
+# Copyright (c) 2022-2025 Status Research & Development GmbH


This file appears to have no pre-2025 existence.

tersec · 2025-05-14T08:45:15Z

nimbus_verified_proxy/header_store.nim

+      let h = Header()
+    return h
+
+proc new*(T: type HeaderStore, max: int): T =


https://status-im.github.io/nim-style-guide/language.proc.html

Prefer func - use proc when side effects cannot conveniently be avoided.

Also for add (probably, I don't see the side effects at least), latest, etc.

kdeme · 2025-05-14T08:50:22Z

nimbus_verified_proxy/rpc/blocks.nim

+    return err("syncing")
+
+  # walk blocks backwards(time) from source to target
+  let isLinked = await self.walkBlocks(


I think we should have some limit here of how much back in time you can request a block.

tersec · 2025-05-14T09:09:00Z

nimbus_verified_proxy/rpc/receipts.nim

+template toLog(lg: LogObject): Log =
+  Log(address: lg.address, topics: lg.topics, data: lg.data)
+
+proc toLogs(logs: openArray[LogObject]): seq[Log] =


func (etc)

also https://status-im.github.io/nim-style-guide/language.result.html

Avoid using result for returning values.

tersec · 2025-05-14T09:10:13Z

nimbus_verified_proxy/rpc/receipts.nim

+  )
+
+proc toReceipts(recs: openArray[ReceiptObject]): seq[Receipt] =
+  for r in recs:


func + avoid result + probably can be mapIt(toReceipt(it))

tersec · 2025-05-14T09:11:04Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

+
+    raise newException(ValueError, "receipt couldn't be verified")
+
+  #  # eth_blockNumber - get latest tag from header store


More commented-out code. This is fine as an implementation tracking thing in a draft PR, but preferably not to merge.

tersec · 2025-05-14T09:26:11Z

nimbus_verified_proxy/rpc/rpc_eth_api.nim

+    return
+      AccessListResult(accessList: accessList, error: error, gasUsed: gasUsed.Quantity)
+
+  #  vp.proxy.rpc("eth_estimateGas") do(


Large chunk of commented code (as before, reasonable as progress-tracking stand-in, not reasonable to merge)

tersec · 2025-05-14T09:29:39Z

nimbus_verified_proxy/rpc/accounts.nim

+
+  case proofResult.kind
+  of MissingKey:
+    return ok(EMPTY_ACCOUNT)


Don't need any of these three returns explicitly, that way it becomes a case expression rather than a case statement and Nim effectively statically checks that this is either the last thing in a block/function or is discarded.

chirag-parmar · 2025-05-14T09:32:38Z

Separating PRs would allow us to already get some of this functionality merged without that commented code holding it back.

@bhartnett @tersec @kdeme the reasoning is very sound. I'll create smaller PRs which addresses the reviews and also seperates out code. I'll link the new PRs in your reviews here. Thanks a ton for these reviews, learning a lot

kdeme reviewed Mar 7, 2025

View reviewed changes

nimbus_verified_proxy/rpc/rpc_eth_api.nim Outdated Show resolved Hide resolved

chirag-parmar force-pushed the eth-call-vp branch from 6128e25 to d1f000e Compare March 31, 2025 09:29

chirag-parmar changed the title ~~eth call support~~ Nimbus Verified Proxy May 5, 2025

This was referenced May 5, 2025

Make nimbus_verified_proxy even lighter #1567

Closed

Nimbus Verified Proxy - improvements #1266

Closed

chirag-parmar force-pushed the eth-call-vp branch from c146a47 to 4c998e6 Compare May 6, 2025 08:39

chirag-parmar added 14 commits May 9, 2025 15:20

eth call support

91c9839

rebase fix

55b3c15

use cache of headers instead of blocks

9f68995

fixes

cf579fe

download headers on the fly

5f696a0

verify headers

d8584d8

reorg

1d5a061

transactions support

06f9abc

receipt fix

8f62f39

rebase fix

e61f2e3

fixes

1604592

solve c compilation error

1d4adde

async evm

1502a10

nph

d873e21

chirag-parmar force-pushed the eth-call-vp branch from a1e2e57 to d873e21 Compare May 10, 2025 06:23

chirag-parmar added 2 commits May 10, 2025 12:00

copyright year

03f5c34

evm execution

e0fab22

chirag-parmar marked this pull request as ready for review May 13, 2025 08:38

nph format

c139f21

chirag-parmar requested review from kdeme and bhartnett May 13, 2025 08:59

remove unused headers

2c62fc8

kdeme reviewed May 13, 2025

View reviewed changes

bhartnett reviewed May 14, 2025

View reviewed changes

tersec reviewed May 14, 2025

View reviewed changes

kdeme reviewed May 14, 2025

View reviewed changes

tersec reviewed May 14, 2025

View reviewed changes

chirag-parmar marked this pull request as draft May 20, 2025 06:58

chirag-parmar mentioned this pull request May 20, 2025

proxy: swap block cache for header store #3288

Merged

3 tasks

	# initiialize beacon node genesis data, beacon clock and forkDigests
	# initialize beacon node genesis data, beacon clock and forkDigests

		chainId = getConfiguredChainId(metadata)
		authHooks = @[httpCors(@[])] # TODO: for now we serve all cross origin requests

		../../fluffy/evm/async_evm, ../../fluffy/evm/async_evm_backend, ./accounts, ../types

		export async_evm, async_evm_backend

		@@ -0,0 +1,141 @@
		# nimbus_verified_proxy
		# Copyright (c) 2022-2025 Status Research & Development GmbH

		@@ -0,0 +1,173 @@
		# nimbus_verified_proxy
		# Copyright (c) 2022-2025 Status Research & Development GmbH


		raise newException(ValueError, "receipt couldn't be verified")

		# # eth_blockNumber - get latest tag from header store

Nimbus Verified Proxy #3100

Are you sure you want to change the base?

Nimbus Verified Proxy #3100

Conversation

chirag-parmar commented Feb 21, 2025

bhartnett commented Feb 21, 2025

bhartnett commented Feb 21, 2025

kdeme commented May 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdeme May 14, 2025 • edited Loading

Choose a reason for hiding this comment

bhartnett commented May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tersec commented May 14, 2025

kdeme commented May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chirag-parmar commented May 14, 2025

kdeme May 14, 2025 •

edited

Loading

bhartnett commented May 14, 2025 •

edited

Loading

kdeme commented May 14, 2025 •

edited

Loading