spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.45k stars 3.09k forks source link

Showing unsigned transaction as signed #4814

Closed RHavar closed 4 years ago

RHavar commented 6 years ago

Given the raw transaction:

01000000000102f68f5010515184f69ac5f5ecb4a35494718bdf1c9cfa7e0188b9e7409d129c1c0100000017160014dcf43453327b8851747472233da0918b47e977f5fdffffffc8c8d5fa9022dc6ce4aaf75695946b13e4929c6506e050318f9019d25ee711370100000017160014e0f37d3f8c82b750b598bb1c816cc20190fd40d3fdffffff02ceb403000000000017a914d54adab927c9292b88573de000d73807078211e4872c871be90100000017a914e804c5068415d43f12fa01716d3b25f1f8373d118702473044022009ae33b3763964621c318962630705653970cefe79b63d8383aca4d1c05df64e02205a30c3a9711318f04b5388ba11aca0171a47dff8933b239585b9bb8229af94510121035babe56073cfe90cecd9fc1c3103f33002c087f2b85a2feb22cfe768d7c66bfa000c5c0800

Electrum is showing it as signed:

electrum

When in fact, the 2nd input is not (and thus not allowing it to be signed)

RHavar commented 6 years ago

Based on my reading of the source-code of electrums transaction deserialization, I would think I could convert it to electrums format by appending: 0x45505446ff00 to the start of the transaction, along with 0xFF00 to the start of the unsigned input's scriptSig, but I guess I'm missing something -- as that doesn't work :(

SomberNight commented 6 years ago

Electrum is showing it as signed

This is because since 3.2, Electrum only tries to "fully parse" the inputs if the transaction is using the custom partial serialisation format. (but with older versions, you would have ran into other troubles; I'm guessing the parser would have raised when fed with your example) related: https://github.com/spesmilo/electrum/issues/4514

E.g. if the tx is in network serialisation format, it is considered "complete": https://github.com/spesmilo/electrum/blob/f819e9b6f4b8670329bf293bbcd9b37c57004a0e/electrum/transaction.py#L1201-L1205

but I guess I'm missing something -- as that doesn't work :(

I, too, have spent some time on this after reading this issue, and it's not trivial to fix. I'll have another look later. Are you looking at this for https://github.com/spesmilo/electrum/issues/4766 ?

To be honest transaction.py is full of hacks :/ I'm hoping much of that code can be cleaned up after adding support for PSBTs (https://github.com/spesmilo/electrum/issues/4615).

RHavar commented 6 years ago

@SomberNight yeah, it's in relation to #4766 -- just trying to see if I can come up with a super hacky way to get it working (even if it's just manually). Basically I want to parse a (pure-segwit) transaction, and then any input that's missing a witness have electrum know it needs signing.

But all I'm really doing is a good job of confusing myself =/

SomberNight commented 6 years ago

Try with this patch:

diff --git a/electrum/transaction.py b/electrum/transaction.py
index 2965d77cf..3b1ead8ad 100644
--- a/electrum/transaction.py
+++ b/electrum/transaction.py
@@ -1199,8 +1199,8 @@ class Transaction:
         return s, r

     def is_complete(self):
-        if not self.is_partial_originally:
-            return True
+        #if not self.is_partial_originally:
+        #    return True
         s, r = self.signature_count()
         return r == s

diff --git a/electrum/wallet.py b/electrum/wallet.py
index 62964486a..29b196698 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -746,8 +746,8 @@ class Abstract_Wallet(AddressSynchronizer):
             self.add_input_sig_info(txin, address)

     def add_input_info_to_all_inputs(self, tx):
-        if tx.is_complete():
-            return
+        #if tx.is_complete():
+        #    return
         for txin in tx.inputs():
             self.add_input_info(txin)

I think the two lines from transaction.py potentially could be removed; still need to think about implications more. The change in wallet.py is only there to illustrate the point.

wallet.add_input_info_to_all_inputs makes the wallet able to sign its own inputs -- but at the same time it destroys signatures currently there (on own inputs) (well, sort of, if the txn is in the custom partial format, existing sigs for multisig inputs should be preserved for example). wallet.add_input_info_to_all_inputs runs, for instance, when you open a Transaction Dialog.

This should be enough to "come up with a super hacky way to get it working" :)

EDIT: note that you also need https://github.com/spesmilo/electrum/commit/f53b480f1cd1b572e4c39b249b5c1ff2a0737466 for this to work, so make sure to pull

RHavar commented 6 years ago

Thanks for the help. I pulled the latest commit, applied that patch and tried. The transaction itself was marked as fully-signed, so I tried adding the prefix 0x45505446ff00 which made the gui recognize the the transaction as partially signed (yay!). However hitting the "sign" (with trezor HW wallet) gave:

Traceback (most recent call last):
  File "~/electrum/electrum/gui/qt/util.py", line 666, in run
    result = task.task()
  File "~/electrum/electrum/wallet.py", line 805, in sign_transaction
    self.add_hw_info(tx)
  File "~/electrum/electrum/wallet.py", line 784, in add_hw_info
    txin['prev_tx'] = self.get_input_tx(tx_hash, ignore_timeout)
  File "~/electrum/electrum/wallet.py", line 771, in get_input_tx
    tx = Transaction(self.network.get_transaction(tx_hash))
  File "~electrum/electrum/transaction.py", line 681, in __init__
    raise Exception("cannot initialize transaction", raw)
Exception: ('cannot initialize transaction', <coroutine object Network.best_effort_reliable.<locals>.make_reliable_wrapper at 0x112b31448>)
~/electrum/electrum/gui/qt/__init__.py:314: RuntimeWarning: coroutine 'Network.best_effort_reliable.<locals>.make_reliable_wrapper' was never awaited
SomberNight commented 6 years ago

okay, that's unrelated. try with https://github.com/spesmilo/electrum/commit/5b4fada2a03f1ebfb156d7c4c7e5d468669ae8d6

The transaction itself was marked as fully-signed

hmmm, not sure. It seemed to work for me without adding the prefix

RHavar commented 6 years ago

The problem here might actually be with the trezor. It appear trezor firmware doesn't actually implement support signing a subset of the inputs (even though the interface supports yet). Will need to do further digging.

Nothing is easy :(

SomberNight commented 6 years ago

I would suggest trying to get it to work on testnet, with a standard software wallet first. And yes, I am fairly certain python-trezor will want to sign all inputs.

kot-begemot commented 5 years ago

Any news on this issue? I faced same thing with electrum-ltc (which I assume is a clone of electrum). For some version of electrum (not sure which version was that) I was able to sign multisig transaction, but I can't any more, as I upgradeed my client.

Here is a link to an issue I opened for electrum-ltc https://github.com/pooler/electrum-ltc/issues/209

If needed I can copy here thing from that issue that applies to BTC

andronoob commented 5 years ago

Recently I came across a problem similar to this, too. I created an unsinged RBF transaction (1-input, 1-output) using electrum-3.3.4-portable.exe , then I tried to sign it with Electrum 3.3.4 for Android - the transaction was recoginised as "signed", I was unable to sign it.

SomberNight commented 5 years ago

@andronoob that should not happen. what kind of wallet is this with (what kind of output scripts; multisig/segwit)?

andronoob commented 5 years ago

@SomberNight It's P2SH-P2WPKH to native P2WPKH, 1-in-1-out. I was still using a problematic wallet ( #5156 ) on Android.

andronoob commented 5 years ago

I reproduced this problem with Counterparty's Proof-of-Burn address:

createrawtransaction "[{\"txid\":\"9bdf7995e49129dc660d64b660bd9d1876eed76a1fb4bd4ea4f41418ae8c3b7f\",\"vout\":0}]" "[{\"1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa\":0.0001}]"
02000000017f3b8cae1814f4a44ebdb41f6ad7ee76189dbd60b6640d66dc2991e49579df9b0000000000ffffffff0110270000000000001976a91462e907b15cbf27d5425399ebf6f0fb50ebb88f1888ac00000000

XCP

SomberNight commented 5 years ago

@andronoob right. https://github.com/spesmilo/electrum/issues/4814#issuecomment-434068393

This is because since 3.2, Electrum only tries to "fully parse" the inputs if the transaction is using the custom partial serialisation format.

So this is expected if you create the unsigned transaction with something other than Electrum, for now. Previously you said that you had managed to reproduce this by creating the transaction in a new version of Electrum.

SomberNight commented 4 years ago

Closing, as since implementing PSBT support (https://github.com/spesmilo/electrum/pull/5721), this code has changed substantially.