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

Ethereum: support SignMessage/Verify Message #163

Closed
prusnak opened this Issue May 3, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@prusnak
Copy link
Member

prusnak commented May 3, 2017

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

jhoenicke commented Jul 1, 2017

I'd like to start implementing this.

Do you think it is better to make three new API calls (EthereumSignMessage/EthereumVerifyMessage/EthereumMessageSignature) or reuse the current ones and allow coinname='Ethereum' there. Alternatively, one could reuse MessageSignature and only add Sign/Verify.

I'd go for three new messages.

The signing code is similar but uses keccak instead of sha2

@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 1, 2017

Let's use new messages like we do for EthereumSignTx and EthereumGetAddress.

@jhoenicke jhoenicke self-assigned this Jul 2, 2017

jhoenicke added a commit to jhoenicke/trezor-mcu that referenced this issue Jul 2, 2017

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

jhoenicke commented Jul 2, 2017

There seems to be confusion, whether there should be the "\x19Ethereum Signed Message:\n${messagelen}" header. I decided to not include it, so that the signatures matches what MyEtherWallet or etherscan.io do.

@saleemrashid

This comment has been minimized.

Copy link
Contributor

saleemrashid commented Jul 2, 2017

@jhoenicke Doesn't that allow you to sign Ethereum transactions?

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

jhoenicke commented Jul 3, 2017

@saleemrashid Yes, the situtation is kind of confusing. It looks like the eth_sign functionality in geth/parity is the library and the user tools should call it with the prefix added to the message. However, neither myetherwallet nor etherscan.io do this at the moment. So you can sign a transaction using the sign message feature.

Also the proposed prefix is really strange. You have to sign "\x19Ethereum Signed Message:\n12Some Message". Yes the 12 is in ascii in decimal without any separator added after it.

@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 12, 2017

@jhoenicke What is the state of this feature? I'd like to release new firmware soon and this would be a nice addition. Is there anything I could help with? (Research? Testing? etc.)

@prusnak prusnak added the nextrelease label Jul 12, 2017

@jhoenicke

This comment has been minimized.

Copy link
Collaborator

jhoenicke commented Jul 12, 2017

The code is in my ethersign branches in trezor-common, trezor-mcu, and python-trezor. The python-trezor code needs to be updated for the new trezorctl style though.

The code is finished, it signs the raw message without any prefix. Maybe we should at least have a flag that allows to use the Geth prefix, that Etherscan.io allows when verifying messages.

unit test is still missing.

@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 12, 2017

According to https://github.com/ethereum/go-ethereum/blob/9e5f03b6c487175cc5aa1224e5e12fd573f483a7/internal/ethapi/api.go#L361 it seems they are indeed using ASCII for length, sigh.

I think we should sign WITH prefix and use the correct prefix (=varint one from Bitcoin) and educate others to do the same.

I started an issue here: ethereum/go-ethereum#14794

prusnak added a commit that referenced this issue Jul 12, 2017

Ethereum Sign/Verify Message
Implements issue #163.
@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 12, 2017

Merged your changes in c5e927f

Added prefix (+other maintenance changes) in b0ac3a2

@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 12, 2017

I am going to close this issue, let's see how the discussion in the other thread evolves.

@prusnak prusnak closed this Jul 12, 2017

@prusnak

This comment has been minimized.

Copy link
Member

prusnak commented Jul 12, 2017

Adding commits where this is implemented in trezor-common/python-trezor for reference:

trezor/trezor-common@b29b98d
trezor/python-trezor@23ab43d

@Ivshti

This comment has been minimized.

Copy link

Ivshti commented Feb 26, 2018

For anyone wondering how to do this in solidity: AdExNetwork/adex-core@814bb91#diff-a395ac9d4c11715998c09afc5f4ae246R248

Because of the way bitcoin variant length integer works, any number under 254 resolves to a single byte, which you can put in the string with "\x" and then the hex representation of the byte. For example, for a 32, you would do "\x20" (20 being the hex representation of 32)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment