-
Notifications
You must be signed in to change notification settings - Fork 773
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
Implement debug_setHead #3811
Implement debug_setHead #3811
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
Co-authored-by: acolytec3 <[email protected]>
…mjs/ethereumjs-monorepo into debug-setHead-rpc-implementation
@@ -0,0 +1,96 @@ | |||
import { Block, createBlock, createBlockHeader } from '@ethereumjs/block' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied these helpers from the blockchain package test helpers in packages/blockchain/test/util.ts
. Any thoughts on extracting these out to the util
package so that they can be imported for testing and maybe production use cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking more into this, I realized this is not possible since it would introduce circular dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 comments.
I would have assumed we have some blockchain generators for tests in the client, but it appears not? 🤔 I'm not sure if we want to export those from the blockchain package for production use-cases though (for testing - absolutely! 👍 )
const oldHead = headHash ? bytesToHex(headHash!) : undefined | ||
const block = await getBlockByOption(blockOpt, this.chain) | ||
try { | ||
await this.service.skeleton?.setHead(block, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sets the skeleton head. Should the vm execution part also not be updated? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated it to set the execution head as well. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, can you also add this check in the tests? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the tests to include it.
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like the docs from geth, are these also applicable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is implemented with force=true
, I'd say it can be destructive, so it would be good to know in the docs.
headHash = await this.service.skeleton?.headHash() | ||
const newHead = headHash ? bytesToHex(headHash!) : undefined | ||
|
||
return `oldHead: ${oldHead} - newHead: ${newHead}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thought I had here is it would be worth making our response value match Geth's as well. Strings like above are pretty non-standard for RPC responses and you can't parse them programmatically without having to do fancy string parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think you're right will remove. It was a convenient way to develop, but will change it to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change provides an implementation for
debug_setHead
as a part of the work being done in #3781 in order to integrate, develop, and provide an ethereumjs repl console. This work also relates to #1114 and increasing JSON-RPC API coverage.