Get rid of HexQuantityStr and HexDataStr usage

Both types are not safe and require validation/conversion from rpc implementer. This PR change it to safer types and delegate the conversion and validation to the rpc library.
status-im · Dec 27, 2023 · c0d52ba · c0d52ba
1 parent 5e95df6
commit c0d52ba
Show file tree

Hide file tree

Showing 8 changed files with 137 additions and 157 deletions.
diff --git a/fluffy/rpc/rpc_eth_api.nim b/fluffy/rpc/rpc_eth_api.nim
@@ -10,7 +10,7 @@
 import
   std/[times, sequtils, strutils, typetraits],
   json_rpc/[rpcproxy, rpcserver], stew/byteutils,
-  web3/[conversions, ethhexstrings], # sigh, for FixedBytes marshalling
+  web3/[conversions], # sigh, for FixedBytes marshalling
   eth/[common/eth_types, rlp],
   beacon_chain/spec/forks,
   ../../nimbus/rpc/[rpc_types, filters],
@@ -218,55 +218,56 @@ proc installEthApiHandlers*(
     return some(BlockObject.init(header, body, fullTransactions))
 
   rpcServerWithProxy.rpc("eth_getBlockByNumber") do(
-      quantityTag: string, fullTransactions: bool) -> Option[BlockObject]:
-    let tag = quantityTag.toLowerAscii
-    case tag
-    of "latest":
-      # TODO:
-      # I assume this would refer to the content in the latest optimistic update
-      # in case the majority treshold is not met. And if it is met it is the
-      # same as the safe version?
-      raise newException(ValueError, "Latest tag not yet implemented")
-    of "earliest":
-      raise newException(ValueError, "Earliest tag not yet implemented")
-    of "safe":
-      if beaconLightClient.isNone():
-        raise newException(ValueError, "Safe tag not yet implemented")
-
-      withForkyStore(beaconLightClient.value().store[]):
-        when lcDataFork > LightClientDataFork.Altair:
-          let
-            blockHash = forkyStore.optimistic_header.execution.block_hash
-            (header, body) = (await historyNetwork.getBlock(blockHash)).valueOr:
-              return none(BlockObject)
-
-          return some(BlockObject.init(header, body, fullTransactions))
-        else:
-          raise newException(
-            ValueError, "Not available before Capella - not synced?")
-    of "finalized":
-      if beaconLightClient.isNone():
-        raise newException(ValueError, "Finalized tag not yet implemented")
-
-      withForkyStore(beaconLightClient.value().store[]):
-        when lcDataFork > LightClientDataFork.Altair:
-          let
-            blockHash = forkyStore.finalized_header.execution.block_hash
-            (header, body) = (await historyNetwork.getBlock(blockHash)).valueOr:
-              return none(BlockObject)
-
-          return some(BlockObject.init(header, body, fullTransactions))
-        else:
-          raise newException(
-            ValueError, "Not available before Capella - not synced?")
-    of "pending":
-      raise newException(ValueError, "Pending tag not yet implemented")
+      quantityTag: BlockTag, fullTransactions: bool) -> Option[BlockObject]:
+
+    if quantityTag.kind == bidAlias:
+      let tag = quantityTag.alias.toLowerAscii
+      case tag
+      of "latest":
+        # TODO:
+        # I assume this would refer to the content in the latest optimistic update
+        # in case the majority treshold is not met. And if it is met it is the
+        # same as the safe version?
+        raise newException(ValueError, "Latest tag not yet implemented")
+      of "earliest":
+        raise newException(ValueError, "Earliest tag not yet implemented")
+      of "safe":
+        if beaconLightClient.isNone():
+          raise newException(ValueError, "Safe tag not yet implemented")
+
+        withForkyStore(beaconLightClient.value().store[]):
+          when lcDataFork > LightClientDataFork.Altair:
+            let
+              blockHash = forkyStore.optimistic_header.execution.block_hash
+              (header, body) = (await historyNetwork.getBlock(blockHash)).valueOr:
+                return none(BlockObject)
+
+            return some(BlockObject.init(header, body, fullTransactions))
+          else:
+            raise newException(
+              ValueError, "Not available before Capella - not synced?")
+      of "finalized":
+        if beaconLightClient.isNone():
+          raise newException(ValueError, "Finalized tag not yet implemented")
+
+        withForkyStore(beaconLightClient.value().store[]):
+          when lcDataFork > LightClientDataFork.Altair:
+            let
+              blockHash = forkyStore.finalized_header.execution.block_hash
+              (header, body) = (await historyNetwork.getBlock(blockHash)).valueOr:
+                return none(BlockObject)
+
+            return some(BlockObject.init(header, body, fullTransactions))
+          else:
+            raise newException(
+              ValueError, "Not available before Capella - not synced?")
+      of "pending":
+        raise newException(ValueError, "Pending tag not yet implemented")
+      else:
+        raise newException(ValueError, "Unsupported block tag " & tag)
     else:
-      if not validate(quantityTag.HexQuantityStr):
-        raise newException(ValueError, "Provided block number is not a hex number")
-
       let
-        blockNumber = fromHex(UInt256, quantityTag)
+        blockNumber = quantityTag.number.toBlockNumber
         maybeBlock = (await historyNetwork.getBlock(blockNumber)).valueOr:
           raise newException(ValueError, error)
 

diff --git a/nimbus/evm/async/data_sources/json_rpc_data_source.nim b/nimbus/evm/async/data_sources/json_rpc_data_source.nim
@@ -136,7 +136,6 @@ type
 proc fetchAccountAndSlots*(rpcClient: RpcClient, address: EthAddress, slots: seq[UInt256], blockNumber: common.BlockNumber): Future[(Account, AccountProof, seq[StorageProof])] {.async.} =
   let t0 = now()
   debug "Got to fetchAccountAndSlots", address=address, slots=slots, blockNumber=blockNumber
-  #let blockNumberHexStr: HexQuantityStr = encodeQuantity(blockNumber)
   let blockNumberUint64 = blockNumber.truncate(uint64)
   let a = web3.Address(address)
   let bid = blockId(blockNumber.truncate(uint64))

diff --git a/nimbus/rpc/debug.nim b/nimbus/rpc/debug.nim
@@ -9,7 +9,9 @@
 
 import
   std/json,
-  json_rpc/rpcserver, rpc_utils,
+  json_rpc/rpcserver, 
+  ./rpc_utils,
+  ./rpc_types,
   ../tracer, ../vm_types,
   ../common/common,
   ../beacon/web3_eth_conv,
@@ -63,7 +65,7 @@ proc setupDebugRpc*(com: CommonRef, rpcsrv: RpcServer) =
 
     result = traceTransaction(com, blockHeader, blockBody, txDetails.index, flags)
 
-  rpcsrv.rpc("debug_dumpBlockStateByNumber") do(quantityTag: string) -> JsonNode:
+  rpcsrv.rpc("debug_dumpBlockStateByNumber") do(quantityTag: BlockTag) -> JsonNode:
     ## Retrieves the state that corresponds to the block number and returns
     ## a list of accounts (including storage and code).
     ##
@@ -89,7 +91,7 @@ proc setupDebugRpc*(com: CommonRef, rpcsrv: RpcServer) =
 
     result = dumpBlockState(com, header, body)
 
-  rpcsrv.rpc("debug_traceBlockByNumber") do(quantityTag: string, options: Option[TraceOptions]) -> JsonNode:
+  rpcsrv.rpc("debug_traceBlockByNumber") do(quantityTag: BlockTag, options: Option[TraceOptions]) -> JsonNode:
     ## The traceBlock method will return a full stack trace of all invoked opcodes of all transaction
     ## that were included included in this block.
     ##
@@ -119,7 +121,7 @@ proc setupDebugRpc*(com: CommonRef, rpcsrv: RpcServer) =
 
     result = traceBlock(com, header, body, flags)
 
-  rpcsrv.rpc("debug_setHead") do(quantityTag: string) -> bool:
+  rpcsrv.rpc("debug_setHead") do(quantityTag: BlockTag) -> bool:
     ## Sets the current head of the local chain by block number.
     ## Note, this is a destructive action and may severely damage your chain.
     ## Use with extreme caution.

diff --git a/nimbus/rpc/p2p.nim b/nimbus/rpc/p2p.nim
@@ -25,15 +25,6 @@ import
   ../beacon/web3_eth_conv,
   ./filters
 
-#[
-  Note:
-    * Hexstring types (HexQuantitySt, HexDataStr, Web3Address, Web3Hash)
-      are parsed to check format before the RPC blocks are executed and will
-      raise an exception if invalid.
-    * Many of the RPC calls do not validate hex string types when output, only
-      type cast to avoid extra processing.
-]#
-
 type
   BlockHeader = eth_types.BlockHeader
   Hash256 = eth_types.Hash256
@@ -49,9 +40,9 @@ proc setupEthRpc*(
     let ac = newAccountStateDB(chainDB, header.stateRoot, com.pruneTrie)
     result = ReadOnlyStateDB(ac)
 
-  proc stateDBFromTag(tag: string, readOnly = true): ReadOnlyStateDB
+  proc stateDBFromTag(quantityTag: BlockTag, readOnly = true): ReadOnlyStateDB
       {.gcsafe, raises: [CatchableError].} =
-    result = getStateDB(chainDB.headerFromTag(tag))
+    result = getStateDB(chainDB.headerFromTag(quantityTag))
 
   server.rpc("eth_protocolVersion") do() -> Option[string]:
     # Old Ethereum wiki documents this as returning a decimal string.
@@ -112,7 +103,7 @@ proc setupEthRpc*(
     ## Returns integer of the current block number the client is on.
     result = w3Qty(chainDB.getCanonicalHead().blockNumber)
 
-  server.rpc("eth_getBalance") do(data: Web3Address, quantityTag: string) -> UInt256:
+  server.rpc("eth_getBalance") do(data: Web3Address, quantityTag: BlockTag) -> UInt256:
     ## Returns the balance of the account of given address.
     ##
     ## data: address to check for balance.
@@ -123,7 +114,7 @@ proc setupEthRpc*(
       address = data.ethAddr
     result = accDB.getBalance(address)
 
-  server.rpc("eth_getStorageAt") do(data: Web3Address, slot: UInt256, quantityTag: string) -> UInt256:
+  server.rpc("eth_getStorageAt") do(data: Web3Address, slot: UInt256, quantityTag: BlockTag) -> UInt256:
     ## Returns the value from a storage position at a given address.
     ##
     ## data: address of the storage.
@@ -135,7 +126,7 @@ proc setupEthRpc*(
       address = data.ethAddr
     result = accDB.getStorage(address, slot)[0]
 
-  server.rpc("eth_getTransactionCount") do(data: Web3Address, quantityTag: string) -> Web3Quantity:
+  server.rpc("eth_getTransactionCount") do(data: Web3Address, quantityTag: BlockTag) -> Web3Quantity:
     ## Returns the number of transactions sent from an address.
     ##
     ## data: address.
@@ -157,7 +148,7 @@ proc setupEthRpc*(
       txCount   = chainDB.getTransactionCount(header.txRoot)
     result = w3Qty(txCount)
 
-  server.rpc("eth_getBlockTransactionCountByNumber") do(quantityTag: string) -> Web3Quantity:
+  server.rpc("eth_getBlockTransactionCountByNumber") do(quantityTag: BlockTag) -> Web3Quantity:
     ## Returns the number of transactions in a block matching the given block number.
     ##
     ## data: integer of a block number, or the string "earliest", "latest" or "pending", as in the default block parameter.
@@ -178,7 +169,7 @@ proc setupEthRpc*(
       unclesCount = chainDB.getUnclesCount(header.ommersHash)
     result = w3Qty(unclesCount)
 
-  server.rpc("eth_getUncleCountByBlockNumber") do(quantityTag: string) -> Web3Quantity:
+  server.rpc("eth_getUncleCountByBlockNumber") do(quantityTag: BlockTag) -> Web3Quantity:
     ## Returns the number of uncles in a block from a block matching the given block number.
     ##
     ## quantityTag: integer of a block number, or the string "latest", "earliest" or "pending", see the default block parameter.
@@ -188,7 +179,7 @@ proc setupEthRpc*(
       unclesCount = chainDB.getUnclesCount(header.ommersHash)
     result = w3Qty(unclesCount.uint)
 
-  server.rpc("eth_getCode") do(data: Web3Address, quantityTag: string) -> seq[byte]:
+  server.rpc("eth_getCode") do(data: Web3Address, quantityTag: BlockTag) -> seq[byte]:
     ## Returns code at a given address.
     ##
     ## data: address
@@ -232,7 +223,7 @@ proc setupEthRpc*(
       raise newException(ValueError, "Account locked, please unlock it first")
 
     let
-      accDB    = stateDBFromTag("latest")
+      accDB    = stateDBFromTag(blockId("latest"))
       tx       = unsignedTx(data, chainDB, accDB.getNonce(address) + 1)
       eip155   = com.isEIP155(com.syncCurrent)
       signedTx = signTransaction(tx, acc.privateKey, com.chainId, eip155)
@@ -252,7 +243,7 @@ proc setupEthRpc*(
       raise newException(ValueError, "Account locked, please unlock it first")
 
     let
-      accDB    = stateDBFromTag("latest")
+      accDB    = stateDBFromTag(blockId("latest"))
       tx       = unsignedTx(data, chainDB, accDB.getNonce(address) + 1)
       eip155   = com.isEIP155(com.syncCurrent)
       signedTx = signTransaction(tx, acc.privateKey, com.chainId, eip155)
@@ -276,7 +267,7 @@ proc setupEthRpc*(
       raise newException(ValueError, res.error)
     result = txHash.w3Hash
 
-  server.rpc("eth_call") do(call: EthCall, quantityTag: string) -> seq[byte]:
+  server.rpc("eth_call") do(call: EthCall, quantityTag: BlockTag) -> seq[byte]:
     ## Executes a new message call immediately without creating a transaction on the block chain.
     ##
     ## call: the transaction call object.
@@ -288,7 +279,7 @@ proc setupEthRpc*(
       res      = rpcCallEvm(callData, header, com)
     result = res.output
 
-  server.rpc("eth_estimateGas") do(call: EthCall, quantityTag: string) -> Web3Quantity:
+  server.rpc("eth_estimateGas") do(call: EthCall, quantityTag: BlockTag) -> Web3Quantity:
     ## Generates and returns an estimate of how much gas is necessary to allow the transaction to complete.
     ## The transaction will not be added to the blockchain. Note that the estimate may be significantly more than
     ## the amount of gas actually used by the transaction, for a variety of reasons including EVM mechanics and node performance.
@@ -318,7 +309,7 @@ proc setupEthRpc*(
     else:
       result = nil
 
-  server.rpc("eth_getBlockByNumber") do(quantityTag: string, fullTransactions: bool) -> BlockObject:
+  server.rpc("eth_getBlockByNumber") do(quantityTag: BlockTag, fullTransactions: bool) -> BlockObject:
     ## Returns information about a block by block number.
     ##
     ## quantityTag: integer of a block number, or the string "earliest", "latest" or "pending", as in the default block parameter.
@@ -366,7 +357,7 @@ proc setupEthRpc*(
     else:
       result = nil
 
-  server.rpc("eth_getTransactionByBlockNumberAndIndex") do(quantityTag: string, quantity: Web3Quantity) -> TransactionObject:
+  server.rpc("eth_getTransactionByBlockNumberAndIndex") do(quantityTag: BlockTag, quantity: Web3Quantity) -> TransactionObject:
     ## Returns information about a transaction by block number and transaction index position.
     ##
     ## quantityTag: a block number, or the string "earliest", "latest" or "pending", as in the default block parameter.
@@ -425,7 +416,7 @@ proc setupEthRpc*(
     result = populateBlockObject(uncles[index], chainDB, false, true)
     result.totalDifficulty = chainDB.getScore(header.hash)
 
-  server.rpc("eth_getUncleByBlockNumberAndIndex") do(quantityTag: string, quantity: Web3Quantity) -> BlockObject:
+  server.rpc("eth_getUncleByBlockNumberAndIndex") do(quantityTag: BlockTag, quantity: Web3Quantity) -> BlockObject:
     # Returns information about a uncle of a block by number and uncle index position.
     ##
     ## quantityTag: a block number, or the string "earliest", "latest" or "pending", as in the default block parameter.

diff --git a/nimbus/rpc/rpc_types.nim b/nimbus/rpc/rpc_types.nim
@@ -18,3 +18,7 @@ export
 
 type
   FilterLog* = eth_api_types.LogObject
+
+  # BlockTag instead of BlockId:
+  # prevent type clash with eth2 BlockId in fluffy/verified_proxy
+  BlockTag* = eth_api_types.RtBlockIdentifier
diff --git a/nimbus/rpc/rpc_utils.nim b/nimbus/rpc/rpc_utils.nim
@@ -13,7 +13,6 @@ import
   std/[strutils, algorithm, options],
   ./rpc_types,
   eth/[common, keys],
-  web3/ethhexstrings,
   ../db/core_db,
   ../constants, stint,
   ../utils/utils,
@@ -23,38 +22,34 @@ import
   ../beacon/web3_eth_conv
 
 const
-  defaultTag = "latest"
+  defaultTag = blockId("latest")
 
 type
   BlockHeader = common.BlockHeader
 
-proc headerFromTag*(chain: CoreDbRef, blockTag: string): BlockHeader
+proc headerFromTag*(chain: CoreDbRef, blockId: BlockTag): BlockHeader
     {.gcsafe, raises: [CatchableError].} =
-  let tag = blockTag.toLowerAscii
-  case tag
-  of "latest": result = chain.getCanonicalHead()
-  of "earliest": result = chain.getBlockHeader(GENESIS_BLOCK_NUMBER)
-  of "safe": result = chain.safeHeader()
-  of "finalized": result = chain.finalizedHeader()
-  of "pending":
-    #TODO: Implement get pending block
-    raise newException(ValueError, "Pending tag not yet implemented")
-  else:
-    if not validate(tag.HexQuantityStr):
-      raise newException(ValueError, "Invalid hex of blockTag")
-    let blockNum = stint.fromHex(UInt256, tag)
-    result = chain.getBlockHeader(blockNum.toBlockNumber)
 
-proc headerFromTag*(chain: CoreDbRef, blockTag: Option[RtBlockIdentifier]): BlockHeader
-    {.gcsafe, raises: [CatchableError].} =
-  if blockTag.isSome():
-    let blockId = blockTag.get
-    if blockId.kind == bidAlias:
-      return chain.headerFromTag(blockId.alias)
+  if blockId.kind == bidAlias:
+    let tag = blockId.alias.toLowerAscii
+    case tag
+    of "latest": result = chain.getCanonicalHead()
+    of "earliest": result = chain.getBlockHeader(GENESIS_BLOCK_NUMBER)
+    of "safe": result = chain.safeHeader()
+    of "finalized": result = chain.finalizedHeader()
+    of "pending":
+      #TODO: Implement get pending block
+      raise newException(ValueError, "Pending tag not yet implemented")
     else:
-      return chain.getBlockHeader(blockId.number.toBlockNumber)
+      raise newException(ValueError, "Unsupported block tag " & tag)
   else:
-    return chain.headerFromTag(defaultTag)
+    let blockNum = blockId.number.toBlockNumber
+    result = chain.getBlockHeader(blockNum)
+
+proc headerFromTag*(chain: CoreDbRef, blockTag: Option[BlockTag]): BlockHeader
+    {.gcsafe, raises: [CatchableError].} =
+  let blockId = blockTag.get(defaultTag)
+  chain.headerFromTag(blockId)
 
 proc calculateMedianGasPrice*(chain: CoreDbRef): GasInt
     {.gcsafe, raises: [CatchableError].} =