Skip to content
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

ethapi: add personal.signTransaction #15971

Merged
merged 3 commits into from
Jan 26, 2018

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jan 25, 2018

This pr adds personal.signTransaction( <tx> , <pw>) , which is a deficiency at the moment.

In order to create offline transactions, there's currently no great way to accomplish that:

  1. Use personal.unlock and then eth.signTransaction
    • unlockis dangerous for various reasons
  2. Start geth in --nodiscover --maxpeers=0, and use personal.sendTransaction(<tx>,<pw>).
    • This is also dangerous, since locally submitted transactions are stored in transactions.rlp, and geth will broadcast these on the next startup unless the file is deleted.

This pr resolves #15953

return nil, err
}
return &SignTransactionResult{data, tx}, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an awfully lot of duplicated code. Can't we make SendTransaction call SignTransaction and submit the transaction at the end (instead of having the same thing copy-pasted twice)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or if we want to avoid the RLP encoding, then perhaps make a common step that just returns the signer transaction and then SendTransaction can submit it, whilst SignTransaction can RLP encode and return it.

@karalabe karalabe added this to the 1.8.0 milestone Jan 26, 2018
@holiman
Copy link
Contributor Author

holiman commented Jan 26, 2018

Fixed, PTAL

@karalabe
Copy link
Member

Should we allow passing in a nil nonce/gaslimit/gasprice here? The only meaningful reason to not submit a transaction is if the node is not in sync (not yet or offline). If the node is not in sync however, we don't know the nonce, the gas limit cannot be estimated without the required contracts/states and the gasprice will be dangerous as it's some past one.

@karalabe
Copy link
Member

karalabe commented Jan 26, 2018

I think it would be safer to explicitly disallow the nil fields, instead of deal with the slew of bugreports from people "using it wrong".

@holiman
Copy link
Contributor Author

holiman commented Jan 26, 2018

So, my primary usecase is offline transactions, and in those cases the user should supply all values himself, rather than rely on defaults. So I'm ok with that..
So, should we explicitly check that these fields are not nil:

	args.Gas
        args.GasPrice
	args.Nonce

(and still call SetDefault after that check has passed)

?

@holiman
Copy link
Contributor Author

holiman commented Jan 26, 2018

Hold on though. We already have eth.signTransaction. Don't we want those two to behave identically? If so, old or new behaviour?

@holiman
Copy link
Contributor Author

holiman commented Jan 26, 2018

PTAL, changed the behaviour of personal/eth.signTransaction to fail on missing nonce/gas/gasPrice

@holiman
Copy link
Contributor Author

holiman commented Jan 26, 2018

Testing:

> personal.signTransaction({from: eth.accounts[1], to:eth.accounts[1]},"test")
Error: gas not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> personal.signTransaction({from: eth.accounts[1], to:eth.accounts[1], gas:1},"test")
Error: gasPrice not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> personal.signTransaction({from: eth.accounts[1], to:eth.accounts[1],gas:1, gasPrice:1},"test")
Error: nonce not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> personal.signTransaction({from: eth.accounts[1], to:eth.accounts[1],gas:1, gasPrice:1, nonce:1},"test")
{
  raw: "0xf85d01010194f3b3138e5eb1c75b43994d1bb760e2f9f735789680801ca08f8e7cb8d999250a30662a7b11d44afe808f7f9103bc80447599de46251bdcf7a00bc1ddff1fdd1d2c2496807c17cd1d33842fd394a066fb2a51b354a0c2b8d731",
  tx: {
    gas: "0x1",
    gasPrice: "0x1",
    hash: "0x6cb39e49763bc56a10016c8ea94aa9b8585b3037bc93c219ccb5d314cd3363e0",
    input: "0x",
    nonce: "0x1",
    r: "0x8f8e7cb8d999250a30662a7b11d44afe808f7f9103bc80447599de46251bdcf7",
    s: "0xbc1ddff1fdd1d2c2496807c17cd1d33842fd394a066fb2a51b354a0c2b8d731",
    to: "0xf3b3138e5eb1c75b43994d1bb760e2f9f7357896",
    v: "0x1c",
    value: "0x0"
  }
}
> personal.unlockAccount(eth.accounts[1],"test")
true

> eth.signTransaction({ from: eth.accounts[1], to: eth.accounts[1], value:0})
Error: gas not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> eth.signTransaction({ from: eth.accounts[1], to: eth.accounts[1], gas:0})
Error: gasPrice not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> eth.signTransaction({ from: eth.accounts[1], to: eth.accounts[1], gas:0, gasPrice:1})
Error: nonce not specified
    at web3.js:3143:20
    at web3.js:6347:15
    at web3.js:5081:36
    at <anonymous>:1:1

> eth.signTransaction({ from: eth.accounts[1], to: eth.accounts[1], gas:0, gasPrice:1, nonce:1})
{
  raw: "0xf85d01018094f3b3138e5eb1c75b43994d1bb760e2f9f735789680801ca06484d00575e961a7db35ebe5badaaca5cb7ee65d1f2f22f22da87c238b99d30da07a85d65797e4b555c1d3f64beebb2cb6f16a6fbd40c43cc48451eaf85305f66e",
  tx: {
    gas: "0x0",
    gasPrice: "0x1",
    hash: "0x0a32fb4e18bc6f7266a164579237b1b5c74271d453c04eab70444ca367d38418",
    input: "0x",
    nonce: "0x1",
    r: "0x6484d00575e961a7db35ebe5badaaca5cb7ee65d1f2f22f22da87c238b99d30d",
    s: "0x7a85d65797e4b555c1d3f64beebb2cb6f16a6fbd40c43cc48451eaf85305f66e",
    to: "0xf3b3138e5eb1c75b43994d1bb760e2f9f7357896",
    v: "0x1c",
    value: "0x0"
  }
}

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe merged commit 722bac8 into ethereum:master Jan 26, 2018
prestonvanloon pushed a commit to prestonvanloon/go-ethereum that referenced this pull request Apr 2, 2018
* ethapi: add personal.signTransaction

* ethapi: refactor to minimize duplicate code

* ethapi: make nonce,gas,gasPrice obligatory in signTransaction
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
* ethapi: add personal.signTransaction

* ethapi: refactor to minimize duplicate code

* ethapi: make nonce,gas,gasPrice obligatory in signTransaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC: add personal_signTransaction: [tx, pw]
2 participants