greenaddress / WalletCrx

GreenAddress open source client
https://greenaddress.it/
GNU Lesser General Public License v2.1
79 stars 39 forks source link

Using Chrome extention + keepkey; can't spend #28

Closed RyanSinger closed 7 years ago

RyanSinger commented 7 years ago

image

RyanSinger commented 7 years ago

I uninstalled and reinstalled the chrome extention, and I'm still getting the error. I'm regretting sending funds into the keepkey+GA wallet before testing with a small amount. 👎

greenaddress commented 7 years ago

Hi Ryan

What version of firmware for the keep key?

Did you set it up with GreenAddress mnemonics or had it initialized with a mnemonic/seed before use with GreenAddress?

You should be able to use your mnemonic directly if you don't want to wait for a fix, of course meanwhile we'll try to reproduce and fix as soon as possible.

Off the top of my head I think this issue is related to a setup/firmware mismatch, maybe a new version we don't support well yet or something alike.

Can you send an email to us (info@greenaddress.it) with a couple of tx ids (received or sent) to see if we can find any issue around them?

Thanks

RyanSinger commented 7 years ago

It updated firmware and initialized with a seed before using with GreenAddress. How do I check the firmware version?

It's only received one transaction, and never spent. That transaction is 9595828fedce7c490da33f18bf9c6ed0ffbcbeaa8777a5bb240a38247959cdfa

On Fri, Dec 23, 2016, 2:06 PM GreenAddress notifications@github.com wrote:

Hi Ryan

What version of firmware for the keep key?

Did you set it up with GreenAddress mnemonics or had it initialized with a mnemonic/seed before use with GreenAddress?

You should be able to use your mnemonic directly if you don't want to wait for a fix, of course meanwhile we'll try to reproduce and fix as soon as possible.

Off the top of my head I think this issue is related to a setup/firmware mismatch, maybe a new version we don't support well yet or something alike.

Can you send an email to us (info@greenaddress.it) with a couple of tx ids (received or sent) to see if we can find any issue around them?

Thanks

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/greenaddress/WalletCrx/issues/28#issuecomment-269050592, or mute the thread https://github.com/notifications/unsubscribe-auth/ACirqiupoKvqdgvbZ9G38vKE1kH2GcQyks5rLEXHgaJpZM4LTfl7 .

greenaddress commented 7 years ago

There are multiple ways to check the firmware version, I would probably check in the app with a debugger or with the python tools from https://github.com/keepkey/python-keepkey but maybe @pkhx02 (from Keep Key) can suggest an easier way of doing it and maybe even has some insight into the problem at hand.

RyanSinger commented 7 years ago

I'm installed the python-keepkey tools. What's the command to get the firmware version?

RyanSinger commented 7 years ago

Firmware version 3.0. @greenaddress

RyanSinger commented 7 years ago

Ping.

bgok commented 7 years ago

@RyanSinger first off, we will get your coins off the device/greenaddress. It might take a little finessing or a bug fix, but I'm confident it will happen.

This issue seems to be an incompatibility between the message format used by KeepKey vs the format used by GreenAddresses. I notice that GreenAddress uses the Trezor protobuf layout for communicating with KeepKey. This has worked well in the past, but the two have diverged a bit. I have tried to keep an eye on the trezor repository that contains the proto files and point out any incompatibilities to the appropriate people at KeepKey and Trezor, but unfortunately, I missed one recently. Field number 18 of the Features message is different between Trezor and KeepKey. I have it on my todo list to contact Trezor this week to arrange for a work around and negotiate ranges of field numbers so that we don't add more conflicts in the future.

That said, I'm not sure that this is problem here. It would be interesting to look at the log from the error. Is there a way to get logs from GreenAddress? Do you log the messages sent to and received from the device?

@RyanSinger, if you can get the logs, feel free to send them to the support ticket that you opened with KeepKey. Or if the folks at GreenAddress want to take the lead, send the logs to them through a private channel and have them contact me.

RyanSinger commented 7 years ago

Thanks @bgok

I'm not sure how to get logs out of @GreenAddress

greenaddress commented 7 years ago

thanks @bgok and @RyanSinger

I recently heard the protocol changed but I assumed it was backwards compatible. I don't know if an acceptable workaround while this gets fixed is to downgrade the firmware but generally that's not advisable as updates bring a number of bug and security fixes.

We don't have access to logs ourselves but we can try to reproduce by upgrading our Keep Key and use those logs - I assume the issue would repeat, if not we can help @RyanSinger with getting out some logs and see what's happening.

@bgok how much work do you think it would take to support both trezor and keep key at the same time? Or do you think we should wait for you to merge back if possible?

Thanks

bgok commented 7 years ago

@greenaddress I think I found the problem. I haven't done any testing, so this may not be the issues, but I think it's worth looking at.

In July, Trezor added a field to GetAddress:

{
    "rule": "optional",
    "type": "InputScriptType",
    "name": "script_type",
    "id": 5,
    "options": {
        "default": "SPENDADDRESS"
     }
}

I believe that this will make KeepKey fail every time GetAddress is called.

A quick fix would be to delete the default value so that the field won't be added. I'm not 100% sure, but I don't think Trezor uses this field yet. I think it there to support segwit addresses in a future release.

Another option is to leave it as is and KeepKey can add the field in our next firmware release.

bgok commented 7 years ago

If my theory is right, a firmware downgrade won't fix the problem. The optional field with a default value is the problem. I think that breaks backward compatibility. If my theory is right, old firmware from both trezor and keepkey would not work because of this change.

I'll do some testing later today to see if my theory is correct.

bgok commented 7 years ago

I'm going to contact the folks at TREZOR to set up something that avoids conflicts in the future. As one of the maintainers of Multibit, I face the same issues as @greenaddress in maintaining compatibility with both devices. @greenaddress can I include you on the thread when I contact TREZOR?

greenaddress commented 7 years ago

Sure thing @bgok thank you for all your help!

bgok commented 7 years ago

Sorry for the delay, @RyanSinger. KeepKey was hacked on Christmas day and it has been a bit chaotic. I was able to reproduce your issue. I need to look a little deeper to find the cause. I'll try to get back to you later today.

@greenaddress Is there a way for me to see the console in the chrome application?

bgok commented 7 years ago

I figured out how to get the console.

@greenaddress There is definitely something wrong. The SignTx message is requesting a signature of a TESTNET transaction:

[trezor] Sending: SignTx {"inputs_count":1,"outputs_count":1,"coin_name":"Testnet","lock_time":445586}

This output stems from this comparison:

coin_name: _this.network === bitcoin.networks.bitcoin ? 'Bitcoin' : 'Testnet'

_this.network =\= bitcoin.networks.bitcoin

Maybe you intended a deep comparison?

BTW, I also tried it with a TREZOR (firmware v1.4.0) and had the same issue.

I will try installing old firmware to see if that fixes the issue.

bgok commented 7 years ago

I tried KeepKey firmware 1.1.0, still failed. @greenaddress Let me know if you need me to try anything else.

RyanSinger commented 7 years ago

@greenaddress any updates?

RyanSinger commented 7 years ago

@greenaddress C'mon guys. It's been two weeks. How can I get my money out of this wallet?

greenaddress commented 7 years ago

thanks @bgok!

@RyanSinger version 0.0.94 (just released) fixes the issue. Sorry for the waiting and trouble.

Keep in mind that the bug didn't affect the GreenBits app for Android (supports KeepKey) and that in general recovery was always possible with the backed up mnemonics

RyanSinger commented 7 years ago

Problem solved. Works great now. I really appreciate you guys working together on this. I'll be recommending the combination of keepkey+greenaddress to all of my friends as an ideal multi-sig + hardware wallet solution.

greenaddress commented 7 years ago

Great, thanks @RyanSinger