Several clean-ups in nimbus_verified_proxy (#1324)

- clean-up config a bit and remove deprecated log-file
- style fixes
- a bit of error msg & logging clean-up and add missing log
- ...

nimbus_verified_proxy/block_cache.nim

@@ -5,7 +5,10 @@
 #   * 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.
 
-{.push raises: [Defect].}
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
 
 import
   std/tables,

nimbus_verified_proxy/nimbus_verified_proxy.nim

@@ -5,7 +5,10 @@
 #   * 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.
 
-{.push raises: [Defect].}
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
 
 import
   std/[os, strutils],
@@ -50,7 +53,7 @@ proc run() {.raises: [Exception, Defect].} =
   # Required as both Eth2Node and LightClient requires correct config type
   var lcConfig = config.asLightClientConf()
 
-  setupLogging(config.logLevel, config.logStdout, config.logFile)
+  setupLogging(config.logLevel, config.logStdout, none(OutFile))
 
   notice "Launching Nimbus verified proxy",
     version = fullVersionStr, cmdParams = commandLineParams(), config

nimbus_verified_proxy/nimbus_verified_proxy_conf.nim

@@ -19,7 +19,7 @@ import
 
 export net, conf
 
-proc defaultLCPDataDir*(): string =
+proc defaultVerifiedProxyDataDir*(): string =
   let dataDir = when defined(windows):
     "AppData" / "Roaming" / "NimbusVerifiedProxy"
   elif defined(macosx):
@@ -30,7 +30,7 @@ proc defaultLCPDataDir*(): string =
   getHomeDir() / dataDir
 
 const
-  defaultDataLCPDirDesc* = defaultLCPDataDir()
+  defaultDataVerifiedProxyDirDesc* = defaultVerifiedProxyDataDir()
 
 type
   Web3UrlKind* = enum
@@ -59,15 +59,11 @@ type VerifiedProxyConf* = object
     defaultValue: StdoutLogKind.Auto
     name: "log-format" .}: StdoutLogKind
 
-  logFile* {.
-    desc: "Specifies a path for the written Json log file (deprecated)"
-    name: "log-file" .}: Option[OutFile]
-
   # Storage
   dataDir* {.
-    desc: "The directory where nimbus will store all blockchain data"
+    desc: "The directory where nimbus_verified_proxy will store all data"
-    defaultValue: defaultLCPDataDir()
+    defaultValue: defaultVerifiedProxyDataDir()
-    defaultValueDesc: $defaultDataLCPDirDesc
+    defaultValueDesc: $defaultDataVerifiedProxyDirDesc
     abbr: "d"
     name: "data-dir" .}: OutDir
 
@@ -77,6 +73,30 @@ type VerifiedProxyConf* = object
     defaultValueDesc: "mainnet"
     name: "network" .}: Option[string]
 
+  # Consensus light sync
+  # No default - Needs to be provided by the user
+  trustedBlockRoot* {.
+    desc: "Recent trusted finalized block root to initialize the consensus light client from"
+    name: "trusted-block-root" .}: Eth2Digest
+
+  # (Untrusted) web3 provider
+  # No default - Needs to be provided by the user
+  web3url* {.
+    desc: "URL of the web3 data provider"
+    name: "web3-url" .}: ValidatedWeb3Url
+
+  # Local JSON-RPC server
+  rpcAddress* {.
+    desc: "Listening address of the JSON-RPC server"
+    defaultValue: defaultAdminListenAddress
+    defaultValueDesc: $defaultAdminListenAddressDesc
+    name: "rpc-address" .}: ValidIpAddress
+
+  rpcPort* {.
+    desc: "Listening port of the JSON-RPC server"
+    defaultValue: 8545
+    name: "rpc-port" .}: Port
+
   # Libp2p
   bootstrapNodes* {.
     desc: "Specifies one or more bootstrap nodes to use when connecting to the network"
@@ -106,9 +126,10 @@ type VerifiedProxyConf* = object
     defaultValueDesc: $defaultEth2TcpPortDesc
     name: "udp-port" .}: Port
 
+  # TODO: Select a lower amount of peers.
   maxPeers* {.
     desc: "The target number of peers to connect to"
-    defaultValue: 160 # 5 (fanout) * 64 (subnets) / 2 (subs) for a heathy mesh
+    defaultValue: 160 # 5 (fanout) * 64 (subnets) / 2 (subs) for a healthy mesh
     name: "max-peers" .}: int
 
   hardMaxPeers* {.
@@ -131,7 +152,7 @@ type VerifiedProxyConf* = object
 
   agentString* {.
     defaultValue: "nimbus",
-    desc: "Node agent string which is used as identifier in network"
+    desc: "Node agent string which is used as identifier in the LibP2P network"
     name: "agent-string" .}: string
 
   discv5Enabled* {.
@@ -144,29 +165,10 @@ type VerifiedProxyConf* = object
           "maintain the connection to, this requires a not random netkey-file." &
           "In the complete multiaddress format like:" &
           "/ip4/<address>/tcp/<port>/p2p/<peerId-public-key>." &
-          "Peering agreements are established out of band and must be reciprocal."
+          "Peering agreements are established out of band and must be reciprocal"
     name: "direct-peer" .}: seq[string]
 
-  rpcPort* {.
-    desc: "HTTP port for the JSON-RPC server"
-    defaultValue: 8545
-    name: "rpc-port" .}: Port
 
-  rpcAddress* {.
-    desc: "Listening address of the RPC server"
-    defaultValue: defaultAdminListenAddress
-    defaultValueDesc: $defaultAdminListenAddressDesc
-    name: "rpc-address" .}: ValidIpAddress
-
-  # No default - Needs to be provided by the user
-  trustedBlockRoot* {.
-    desc: "Recent trusted finalized block root to initialize the consensus light client from"
-    name: "trusted-block-root" .}: Eth2Digest
-
-  # No default - Needs to be provided by the user
-  web3url* {.
-    desc: "url of web3 data provider"
-    name: "web3-url" .}: ValidatedWeb3Url
 
 proc parseCmdArg*(T: type ValidatedWeb3Url, p: TaintedString): T
       {.raises: [Defect, ConfigurationError].} =
@@ -189,7 +191,7 @@ func asLightClientConf*(pc: VerifiedProxyConf): LightClientConf =
     configFile: pc.configFile,
     logLevel: pc.logLevel,
     logStdout: pc.logStdout,
-    logFile: pc.logFile,
+    logFile: none(OutFile),
     dataDir: pc.dataDir,
     eth2Network: pc.eth2Network,
     bootstrapNodes: pc.bootstrapNodes,

nimbus_verified_proxy/rpc/rpc_eth_api.nim

@@ -5,7 +5,10 @@
 #   * 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.
 
-{.push raises: [Defect].}
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
 
 import
   std/strutils,
@@ -70,7 +73,7 @@ func parseHexQuantity(tag: string): Result[Quantity, string] =
     let parsed = ? parseHexIntResult(tag)
     return ok(Quantity(parsed))
   else:
-    return err("Invalid Etheruem Hex quantity.")
+    return err("Invalid hex quantity.")
 
 func parseQuantityTag(blockTag: string): Result[QuantityTag, string] =
   let tag = blockTag.toLowerAscii
@@ -85,11 +88,13 @@ template checkPreconditions(proxy: VerifiedRpcProxy) =
   if proxy.blockCache.isEmpty():
     raise newException(ValueError, "Syncing")
 
-template rpcClient(lcProxy: VerifiedRpcProxy): RpcClient = lcProxy.proxy.getClient()
+template rpcClient(lcProxy: VerifiedRpcProxy): RpcClient =
+  lcProxy.proxy.getClient()
 
 proc getPayloadByTag(
     proxy: VerifiedRpcProxy,
-    quantityTag: string): results.Opt[ExecutionPayloadV1] {.raises: [ValueError, Defect].} =
+    quantityTag: string):
+    results.Opt[ExecutionPayloadV1] {.raises: [ValueError, Defect].} =
   checkPreconditions(proxy)
 
   let tagResult = parseQuantityTag(quantityTag)
@@ -122,12 +127,13 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
     return encodeQuantity(lcProxy.chainId)
 
   lcProxy.proxy.rpc("eth_blockNumber") do() -> HexQuantityStr:
-    ## Returns the number of most recent block.
+    ## Returns the number of the most recent block.
     checkPreconditions(lcProxy)
 
     return encodeQuantity(lcProxy.blockCache.latest.get.blockNumber)
 
-  lcProxy.proxy.rpc("eth_getBalance") do(address: Address, quantityTag: string) -> HexQuantityStr:
+  lcProxy.proxy.rpc("eth_getBalance") do(
+      address: Address, quantityTag: string) -> HexQuantityStr:
     # When requesting state for `latest` block number, we need to translate
     # `latest` to actual block number as `latest` on proxy and on data provider
     # can mean different blocks and ultimatly piece received piece of state
@@ -136,9 +142,10 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
       executionPayload = lcProxy.getPayloadByTagOrThrow(quantityTag)
       blockNumber = executionPayload.blockNumber.uint64
 
-    info "Forwarding get_Balance", executionBn = blockNumber
+    info "Forwarding eth_getBalance call", blockNumber
 
-    let proof = await lcProxy.rpcClient.eth_getProof(address, @[], blockId(blockNumber))
+    let proof = await lcProxy.rpcClient.eth_getProof(
+      address, @[], blockId(blockNumber))
 
     let accountResult = getAccountFromProof(
       executionPayload.stateRoot,
@@ -155,15 +162,17 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
     else:
       raise newException(ValueError, accountResult.error)
 
-  lcProxy.proxy.rpc("eth_getStorageAt") do(address: Address, slot: HexDataStr, quantityTag: string) -> HexDataStr:
+  lcProxy.proxy.rpc("eth_getStorageAt") do(
+      address: Address, slot: HexDataStr, quantityTag: string) -> HexDataStr:
     let
       executionPayload = lcProxy.getPayloadByTagOrThrow(quantityTag)
       uslot = UInt256.fromHex(slot.string)
       blockNumber = executionPayload.blockNumber.uint64
 
-    info "Forwarding eth_getStorageAt", executionBn = blockNumber
+    info "Forwarding eth_getStorageAt", blockNumber
 
-    let proof = await lcProxy.rpcClient.eth_getProof(address, @[uslot], blockId(blockNumber))
+    let proof = await lcProxy.rpcClient.eth_getProof(
+      address, @[uslot], blockId(blockNumber))
 
     let dataResult = getStorageData(executionPayload.stateRoot, uslot, proof)
 
@@ -173,14 +182,16 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
     else:
       raise newException(ValueError, dataResult.error)
 
-  lcProxy.proxy.rpc("eth_getTransactionCount") do(address: Address, quantityTag: string) -> HexQuantityStr:
+  lcProxy.proxy.rpc("eth_getTransactionCount") do(
+      address: Address, quantityTag: string) -> HexQuantityStr:
     let
       executionPayload = lcProxy.getPayloadByTagOrThrow(quantityTag)
       blockNumber = executionPayload.blockNumber.uint64
 
-    info "Forwarding eth_getTransactionCount", executionBn = blockNumber
+    info "Forwarding eth_getTransactionCount", blockNumber
 
-    let proof = await lcProxy.rpcClient.eth_getProof(address, @[], blockId(blockNumber))
+    let proof = await lcProxy.rpcClient.eth_getProof(
+      address, @[], blockId(blockNumber))
 
     let accountResult = getAccountFromProof(
       executionPayload.stateRoot,
@@ -197,13 +208,15 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
     else:
       raise newException(ValueError, accountResult.error)
 
-  lcProxy.proxy.rpc("eth_getCode") do(address: Address, quantityTag: string) -> HexDataStr:
+  lcProxy.proxy.rpc("eth_getCode") do(
+      address: Address, quantityTag: string) -> HexDataStr:
     let
       executionPayload = lcProxy.getPayloadByTagOrThrow(quantityTag)
       blockNumber = executionPayload.blockNumber.uint64
 
     let
-      proof = await lcProxy.rpcClient.eth_getProof(address, @[], blockId(blockNumber))
+      proof = await lcProxy.rpcClient.eth_getProof(
+        address, @[], blockId(blockNumber))
       accountResult = getAccountFromProof(
         executionPayload.stateRoot,
         proof.address,
@@ -223,6 +236,8 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
       # account does not have any code, return empty hex data
       return hexDataStr("0x")
 
+    info "Forwarding eth_getCode", blockNumber
+
     let code = await lcProxy.rpcClient.eth_getCode(
       address,
       blockId(blockNumber)
@@ -231,10 +246,12 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
     if isValidCode(account, code):
       return bytesToHex(code)
     else:
-      raise newException(ValueError, "received code which does not match account code hash")
+      raise newException(ValueError,
+        "Received code which does not match the account code hash")
 
-  # TODO This methods are forwarded directly to provider therefore thay are not
+  # TODO:
-  # validated in any way
+  # Following methods are forwarded directly to the web3 provider and therefore
+  # are not validated in any way.
   lcProxy.proxy.registerProxyMethod("net_version")
   lcProxy.proxy.registerProxyMethod("eth_call")
   lcProxy.proxy.registerProxyMethod("eth_sendRawTransaction")
@@ -242,7 +259,8 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
 
   # TODO currently we do not handle fullTransactions flag. It require updates on
   # nim-web3 side
-  lcProxy.proxy.rpc("eth_getBlockByNumber") do(quantityTag: string, fullTransactions: bool) -> Option[BlockObject]:
+  lcProxy.proxy.rpc("eth_getBlockByNumber") do(
+      quantityTag: string, fullTransactions: bool) -> Option[BlockObject]:
     let executionPayload = lcProxy.getPayloadByTag(quantityTag)
 
     if executionPayload.isErr:
@@ -250,7 +268,8 @@ proc installEthApiHandlers*(lcProxy: VerifiedRpcProxy) =
 
     return some(asBlockObject(executionPayload.get()))
 
-  lcProxy.proxy.rpc("eth_getBlockByHash") do(blockHash: BlockHash, fullTransactions: bool) -> Option[BlockObject]:
+  lcProxy.proxy.rpc("eth_getBlockByHash") do(
+      blockHash: BlockHash, fullTransactions: bool) -> Option[BlockObject]:
     let executionPayload = lcProxy.blockCache.getPayloadByHash(blockHash)
 
     if executionPayload.isErr:
@@ -263,17 +282,15 @@ proc new*(
     proxy: RpcProxy,
     blockCache: BlockCache,
     chainId: Quantity): T =
-
+  VerifiedRpcProxy(
-  return VerifiedRpcProxy(
     proxy: proxy,
     blockCache: blockCache,
-    chainId: chainId
+    chainId: chainId)
-  )
 
 proc verifyChaindId*(p: VerifiedRpcProxy): Future[void] {.async.} =
   let localId = p.chainId
 
-  # retry 2 times, if the data provider will fail despite re-tries, propagate
+  # retry 2 times, if the data provider fails despite the re-tries, propagate
   # exception to the caller.
   let providerId = awaitWithRetries(
     p.rpcClient.eth_chainId(),
@@ -281,8 +298,9 @@ proc verifyChaindId*(p: VerifiedRpcProxy): Future[void] {.async.} =
     timeout = seconds(30)
   )
 
-  # this configuration error, in theory we could allow proxy to chung on, but
+  # This is a chain/network mismatch error between the Nimbus verified proxy and
-  # it would only mislead the user. It is better to fail fast here.
+  # the application using it. Fail fast to avoid misusage. The user must fix
+  # the configuration.
   if localId != providerId:
     fatal "The specified data provider serves data for a different chain",
       expectedChain = distinctBase(localId),

nimbus_verified_proxy/rpc/rpc_utils.nim

@@ -5,6 +5,11 @@
 #   * 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.
 
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
+
 import
   std/typetraits,
   eth/common/eth_types as etypes,
@@ -22,7 +27,8 @@ template asEthHash(hash: BlockHash): Hash256 =
   Hash256(data: distinctBase(hash))
 
 proc calculateTransactionData(
-  items: openArray[TypedTransaction]): (etypes.Hash256, seq[TxHash], uint64) =
+    items: openArray[TypedTransaction]):
+    (etypes.Hash256, seq[TxHash], uint64) {.raises: [Defect, RlpError].} =
   ## returns tuple composed of
   ## - root of transactions trie
   ## - list of transactions hashes
@@ -37,7 +43,8 @@ proc calculateTransactionData(
     txHashes.add(toFixedBytes(keccakHash(tx)))
   return (tr.rootHash(), txHashes, txSize)
 
-func blockHeaderSize(payload: ExecutionPayloadV1, txRoot: etypes.Hash256): uint64 =
+func blockHeaderSize(
+    payload: ExecutionPayloadV1, txRoot: etypes.Hash256): uint64 =
   let bh = etypes.BlockHeader(
     parentHash    : payload.parentHash.asEthHash,
     ommersHash    : etypes.EMPTY_UNCLE_HASH,
@@ -58,8 +65,9 @@ func blockHeaderSize(payload: ExecutionPayloadV1, txRoot: etypes.Hash256): uint6
   )
   return uint64(len(rlp.encode(bh)))
 
-proc asBlockObject*(p: ExecutionPayloadV1): BlockObject =
+proc asBlockObject*(
-  # TODO currently we always calculate txHashes as BlockObject does not have
+    p: ExecutionPayloadV1): BlockObject {.raises: [Defect, RlpError].} =
+  # TODO: currently we always calculate txHashes as BlockObject does not have
   # option of returning full transactions. It needs fixing at nim-web3 library
   # level
   let (txRoot, txHashes, txSize) = calculateTransactionData(p.transactions)
@@ -82,7 +90,7 @@ proc asBlockObject*(p: ExecutionPayloadV1): BlockObject =
     timestamp: p.timestamp,
     nonce: some(default(FixedBytes[8])),
     size: Quantity(blockSize),
-    # TODO It does not matter what we put here in after merge blocks.
+    # TODO: It does not matter what we put here after merge blocks.
     # Other projects like `helios` return `0`, data providers like alchemy return
     # transition difficulty. For now retruning `0` as this is a bit easier to do.
     totalDifficulty: UInt256.zero,

nimbus_verified_proxy/validate_proof.nim

@@ -5,7 +5,10 @@
 #   * 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.
 
-{.push raises: [Defect].}
+when (NimMajor, NimMinor) < (1, 4):
+  {.push raises: [Defect].}
+else:
+  {.push raises: [].}
 
 import
   std/[sequtils, typetraits, options],
@@ -36,8 +39,8 @@ proc isValidProof(
     key, value: seq[byte]): bool =
   try:
     # TODO: Investigate if this handles proof of non-existence.
-    # Probably not as bool is not expressive enough to say if proof is valid, but
+    # Probably not as bool is not expressive enough to say if proof is valid,
-    # key actually does not exists in MPT
+    # but key actually does not exists in MPT
     return isValidBranch(branch, rootHash, key, value)
   except RlpError:
     return false
@@ -85,7 +88,8 @@ proc getStorageData(
     storageMptNodes = storageProof.proof.mapIt(distinctBase(it))
     key = toSeq(keccakHash(toBytesBE(storageProof.key)).data)
     encodedValue = rlp.encode(storageProof.value)
-    proofResult = verifyMptProof(storageMptNodes, account.storageRoot, key, encodedValue)
+    proofResult = verifyMptProof(
+      storageMptNodes, account.storageRoot, key, encodedValue)
 
   case proofResult.kind
   of MissingKey: