client: allow past block numbers in RPC queries #1598
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Have given this a quick glance, will give it a more thorough review later, but this looks good - and also much more simple than I expected 😄 |
|
Could we wait merging this until #1590 is merged? Then we can also include these changes for the getProof RPC call :) |
* already have array validator (usage: `validators.array(validators.hex)`) * add typedoc for getProof (thanks @gabrocheleau) * update getProof.spec.ts to match future test setup from PR #1598 (this slightly modifies the returned accountProof)
* vm: start on getProof * vm: stateManager: add getProof EIP-1186 * vm: stateManager: add verifyProof * vm: add getProof tests * vm: start on getProof * vm: stateManager: add getProof EIP-1186 * vm: stateManager: add verifyProof * vm: move changes of old state manager into new one * vm: fix proof test * vm: make getProof better readable * vm: stateManager cleanup verifyProof * vm: make proof/verifyproof optional * stateManager: add ropsten stateManager tests stateManager: fix getProof EIP1178 field * stateManager: more getProof tests / ensure geth compatibility * stateManager: partially fix verifyProof * stateManager: fix empty account proofs stateManager: fix storage slot proofs * stateManager: add tests for tampered proofs * stateManager: use Proof type of stateManage not MPT * client: add getProof endpoint client: add tests for getProof * vm: state: update interface * review - vm: * bolster invalid proof error messages * extract ProofStateManager tests to own file * move testdata files to own folder and use import syntax for improved type info * review - client: * already have array validator (usage: `validators.array(validators.hex)`) * add typedoc for getProof (thanks @gabrocheleau) * update getProof.spec.ts to match future test setup from PR #1598 (this slightly modifies the returned accountProof) Co-authored-by: Ryan Ghods <ryan@ryanio.com>
ryanio
force-pushed
the
client-rpc-allow-block-number-arg
branch
from
328f22a to
4af298d
Compare
| @@ -10,48 +11,65 @@ import { checkError } from '../util' | |||
| const method = 'eth_call' | |||
|
|
|||
| tape(`${method}: call with valid arguments`, async (t) => { | |||
| const blockchain = await Blockchain.create() | |||
| const blockchain = await Blockchain.create({ validateBlocks: false, validateConsensus: false }) | |||
There was a problem hiding this comment.
How come this passed before? It should throw on invalid PoW blocks right?
There was a problem hiding this comment.
before we were just using vm.runTx, but now are creating blocks and adding them to the blockchain (so our source code can setStateRoot properly), so needed these additional overrides
|
|
||
| // call with height that exceeds chain height | ||
| req = params(method, [address.toString(), '0x1']) | ||
| expectRes = checkError(t, INVALID_PARAMS, 'specified block greater than current height') |
There was a problem hiding this comment.
This test in general looks great, but it does not explicitly check that when we pass a specific block, it actually uses that block. Probably just create 2 blocks (dont have to be valid) so we have blocks 0 (earliest), 1 and 2 (latest). Now you can check explicitly for earliest/latest (these are two different blocks), explicit 0x0, explicit 0x2, explicit 0x3 (throws) and explicit 0x1. Just check the balance of coinbase of all blocks should be fine.
Also 2 questions: (1) what happens if we pass a negative number over RPC? and (2) in general what happens if the RPC throws? Does it then still return a request with an error code, or does it fail to reply to the request?
There was a problem hiding this comment.
good idea, will add another block, thanks.
- numbers are only passed as hex encoded over the RPC layer, so something like
-0x10?
Trying with toBuffer which we mostly use internally to format, I get:
Error: Cannot convert string to buffer. toBuffer only supports 0x-prefixed hex strings and this string was given: -0x10
- the RPC needs to throw an error of shape
{ code, message }to properly transmit as an JSON-RPC formatted error object. otherwise if something throws deeper, sometimes it may return asInternal Errorwith no error object/msg displayed, which is annoying and challenging to debug :) so you can see in some delicate places in the engine API I do:
} catch (error: any) {
throw {
code: INTERNAL_ERROR,
message: error.message.toString(),
}
}There was a problem hiding this comment.
Ah great thanks.
For (2) I think we should log the entire error message in the console? In Geth, for instance, if you do an RPC call and there is some internal error (e.g. trying to mine without etherbase/coinbase set) then it puts [WARN] <error message> in the console.
There was a problem hiding this comment.
(This is not in the scope of this PR though)
There was a problem hiding this comment.
yeah I agree with you, haven't found a straightforward way to forward all errors like this through jayson yet, definitely worth more investigation to see if it is possible (i assume it should be, we have to place a catch at a higher level and throw with the right obj shape)
|
BTW, this PR also shows that we should really start creating a standalone package for StateManager soon. It does not make a lot of sense to use the VM interface when we have "simple" state lookups such as getCode/getBalance and alike. It only makes sense when we actually simulate txs, such as |
|
Will merge here. |
This PR enables the RPC layer to use past block numbers in
call,getBalance,getCode,getStorageAt,getTransactionCount, andestimateGas.This is possible by setting the vm's stateManager's stateRoot to the block's stateRoot. Thanks @jochem-brouwer for the tip :) The logic is fully tested in
getBalance.spec.tsby comparing a past block to the latest.The second commit (unrelated) simplifies some of the tape result comparisons.