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

Showing unsigned transaction as signed #4814

Closed
RHavar opened this issue Oct 29, 2018 · 15 comments
Closed

Showing unsigned transaction as signed #4814

RHavar opened this issue Oct 29, 2018 · 15 comments
Labels
topic-transactions 📑 related to logic in transaction.py

Comments

@RHavar
Copy link

RHavar commented Oct 29, 2018

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
Copy link
Author

RHavar commented Oct 29, 2018

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 SomberNight added the topic-transactions 📑 related to logic in transaction.py label Oct 29, 2018
@SomberNight
Copy link
Member

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: #4514

E.g. if the tx is in network serialisation format, it is considered "complete":

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

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 #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 (#4615).

SomberNight added a commit that referenced this issue Oct 29, 2018
tangentially related: #4814

also recognise that input is_mine if tx was not fully parsed
but we have the prevout UTXO
@RHavar
Copy link
Author

RHavar commented Oct 30, 2018

@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
Copy link
Member

SomberNight commented Oct 30, 2018

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 f53b480 for this to work, so make sure to pull

@RHavar
Copy link
Author

RHavar commented Oct 30, 2018

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 added a commit that referenced this issue Oct 30, 2018
see #4814 (issuecomment-434392195)
@SomberNight
Copy link
Member

okay, that's unrelated. try with 5b4fada

The transaction itself was marked as fully-signed

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

@RHavar
Copy link
Author

RHavar commented Oct 30, 2018

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
Copy link
Member

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
Copy link

kot-begemot commented Mar 5, 2019

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 pooler#209

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

@andronoob
Copy link

andronoob commented Mar 21, 2019

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
Copy link
Member

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

@andronoob
Copy link

andronoob commented Mar 22, 2019

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

@andronoob
Copy link

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

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

XCP

@SomberNight
Copy link
Member

@andronoob right. #4814 (comment)

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
Copy link
Member

Closing, as since implementing PSBT support (#5721), this code has changed substantially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-transactions 📑 related to logic in transaction.py
Projects
None yet
Development

No branches or pull requests

4 participants