-
Notifications
You must be signed in to change notification settings - Fork 20.6k
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
eth_sendRawTransaction JSON RPC #1267
Conversation
👍 handy |
I definitely would like to see this get merged. Once concern I have is that with this in place, it seem like eth_sendTransaction should then be using the same underlying XEth signing and pushing functions. For example, we have XEth.Sign() and XEth.PushTx(), but neither of these are used by XEth.SendTransaction(), which seems a bit strange. Maybe we leave refactoring for a later date, since there's probably quite a bit of cleanup that can be done, but I wanted to document this concern at a minimum. |
@@ -170,6 +170,17 @@ func (api *EthereumApi) GetRequestReply(req *RpcRequest, reply *interface{}) err | |||
} | |||
*reply = v | |||
|
|||
case "eth_pushTx": |
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.
Please rename this to eth_sendRawTransaction
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 felt it in my bones that you wanted this name :)
@tgerring Yeah, I wasn't to sure how this fits in with the broader plan for geth's sendTransaction system/xEth SIgn and pushTX. But my main point here would be eth_sendRawTransaction should remain an open method outside the account lock mechanism that allows for valid and complete signed TX's to stream into RPC. I see there being many use cases for this, I need it for several projects myself. In general browser-based account management. I'm working with Martin and Aaron on this. Cool stuff ahead! |
Definitely want |
+1 for this but please add test/s too (wherever they are and maybe in https://github.com/ethereum/rpc-tests) |
@@ -51,6 +51,7 @@ var ( | |||
"getData", | |||
"getCode", | |||
"sign", | |||
"eth_sendRawTransaction", |
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 one looks different than all the others.
Not quite groking what feedback the errors provide. What if there's an error? Does this PR check the nonce, verify transaction will run, etc? Would be great to see this go in, and even better if the feedback is as informative as submitting a transaction without a signature. |
Just tested eth_sendRawTransaction with ethereumjs-tx, and curl I get successful results back. All invalid TX data reported back successfully through RPC errors, i.e. low gas, invalid nonce, invalid from data. It acted much like Transaction or eth_sendTransaction. Although more tests should be run to fully vet PushTx. My curl sample is like this: Where unfortunately, the first param used here, must be a "from" address. @tgerring this is unnecessary, but I feel uncomfortable manipulating core to remove this required "from", at this time. And seeing as the "from" param is required in both new and old style transactions, leaving it in seems entirely adequate. The address used here is: The private key used here is: The raw signed transaction used here is: The library used to generate this transaction data can be found here: https://github.com/ethereum/ethereumjs-tx Cheers :) |
This really should have tests in code which will help reviewers and future refactoring and maintenance of this code. Anyway, I won't mention again and will let the reviewers decide between the balance of quickly merging code/functionality vs time to write tests (sometimes the tests could take much longer to write than the code).
Is it possible to have the first param still be an object with a
|
@ethers with the latest commit, thanks to the help of @tgerring you no longer need to add the "from" param inside the RPC "params", example:
The address used:
The private key used:
I don't feel comfortable writing tests for this in go at this point, however, you can do a run through with CURL using the CURL example I provided above. Set the address above as your --etherbase="0f6cf08300b7bdc6d83d1898321a19770cb25578" mine ether to it, then use the CURL sample above to run eth_sendRawTransaction. There is definitely more testing to do here. |
We also need to catch invalid gas, gasPrice, nonce and to much like eth_sendTransaction does. Both with the RPC and with xeth. |
eth_sendRawTransaction JSON RPC
The tests were never merged... I'll create a separate PR with them |
p2p: cherry-pick commits from geth for peering issues
I'm still testing this, but this should allow signed transactions to be sent to JSON RPC. It uses the pushTx method, which I believe is the right approach for sending signed transaction packets to the chain. Thoughts?
Closes #1071