PSBT(bip174) implementation #4615

[asfin]

Support for PSBT serialization format.
Unittests and integration tests provided.
Manually tested(bitcoin testnet) with UI, send to p2sh, p2pkh, p2wsh, p2wpkh. Export to file/qr/buffer works ok.
Resulting PSBT valid for bitcoind walletprocesspsbt API, and can be processed and finalized by it.
Fixes at least this issues #4615 #4514 #4471

About code:
I think package bip174.py should be renamed, because it doesn't contain BIP174 class.
@SomberNight bip32.py refactoring was conflicted with mine xpubkey_utils.py, so i removed xpubkey_utils.py and just moved rest of it to bip32.py
HW plugins wasn't tested, so if you saw an error, write about it(or just fix).
Some parts of code wasn't properly finalized, because i think that review and some time after merge should find some problems.

[SomberNight]

Thanks for this. The diff is huge...
I've added some comments (there may be more coming later).
In general, there seems to be a non-negligible amount of code duplication. Maybe that could be reduced?

[SomberNight]

this is overridden in trustedcoin.py and is_sweep is used there

electrum/electrum/plugins/trustedcoin/trustedcoin.py

Lines 288 to 289 in db89286

	def make_unsigned_transaction(self, coins, outputs, config, fixed_fee=None,
	change_addr=None, is_sweep=False):

[SomberNight]

can't we get rid of make_unsigned_transaction in favour of make_psbt?

[asfin]

@SomberNight removed together with mktx

[asfin]

Removed all mktx related stuff.
Added support for PSBT to Old Keystore. This is covered by test_sending_between_p2sh_2of3_and_uncompressed_p2pkh, test_old_keystore_provides_valid_bip32_keys
Offline tests are broken, trying to figure out possibility to fix it.

[asfin]

Offline tests was broken because offline wallet don't know anything about txin, so i've added non_witness_utxo from input to wallet history, and it works fine. But is it good behavior to populate history from partial transactions?(3f9d9df#diff-6d9743b8dd833bae9dc6e06816142fa2R1059)

@SomberNight Any new issues?

[SomberNight]

But is it good behavior to populate history from partial transactions?

These txns show up in the History tab, right? It might be a bit confusing... but not sure how to solve it otherwise atm.

Where in the code do you specifically need the input/parent transactions? I wonder what side effects of add_transaction (called by receive_tx_callback) are needed. Would this be enough: self.transactions[tx_hash] = tx instead?

[asfin]

Main purpose - is_mine wallet check, which triggers populate and process logic(check amounts, get associated keys for utxo, set txin['type']). And simply using self.transactions[tx_hash] = tx doesn't work.
In case of populating history with this transactions, may it broke transaction synchronization? Because if synchronization performs only on last timestamps/block_count, it may shadow transactions which was made before previous transaction and current.

[SomberNight]

some more comments

[SomberNight]

watch-only wallets will get here as _can_sign seems not to be finished.
so e.g. xpub only wallet can click "sign" button in tx dialog

[asfin]

it's ok now?

[SomberNight]

note: in History tab, right-click>Details on a transaction fails

trace

Traceback (most recent call last):
  File "...\electrum\electrum\gui\qt\history_list.py", line 499, in 
    menu.addAction(_("Details"), lambda: self.show_transaction(tx_hash))
  File "...\electrum\electrum\gui\qt\history_list.py", line 446, in show_transaction
    self.parent.show_transaction(tx, label)
  File "...\electrum\electrum\gui\qt\main_window.py", line 814, in show_transaction
    show_transaction(tx, self, tx_desc)
  File "...\electrum\electrum\gui\qt\transaction_dialog.py", line 52, in show_transaction
    d = TxDialog(tx, parent, desc, prompt_if_unsaved)
  File "...\electrum\electrum\gui\qt\transaction_dialog.py", line 177, in __init__
    self.update()
  File "...\electrum\electrum\gui\qt\transaction_dialog.py", line 271, in update
    self.wallet.add_info_to_psbt(self.psbt)
  File "...\electrum\electrum\wallet.py", line 1060, in add_info_to_psbt
    for inp in psbt.input_sections:
AttributeError: 'NoneType' object has no attribute 'input_sections'

[SomberNight]

Offline signing with an old electrum seed (OldKeystore; discussed recently on IRC) does not work.
The produced transaction fails validation.

I wrote a test for this in e8a8a17; try to see why that fails.

[asfin]

Offline signing with an old electrum seed (OldKeystore; discussed recently on IRC) does not work.
The produced transaction fails validation.

Only difference, is compressed public key. I thought that pushing full pubkey shouldn't work. So, now there are full pubkeys, and test works.

History tab fixed

[asfin]

FYI, trezor test fails because import in trezorlib seems to be broken(at least in my environment)
https://github.com/trezor/python-trezor/blob/master/trezorlib/client.py#L21

[SomberNight]

FYI, trezor test fails because import in trezorlib seems to be broken(at least in my environment)
https://github.com/trezor/python-trezor/blob/master/trezorlib/client.py#L21

is it importing mnemonic.py from electrum instead of the global mnemonic package? I've seen that before. pycharm is mangling the PYTHONPATH. maybe see #4907 (comment)

[jonathancross]

Looks like there is some serious merge conflicts to resolve here.
@asfin ?

[SomberNight]

@asfin Thank you for your work. Master now has PSBT support with #5721

In the end we decided to take a different implementation approach, that IMHO makes the code easier to maintain going forward, mainly that

• there is only two Transaction classes (Transaction and PartialTransaction(Transaction)), where PartialTransaction fully conforms to the API of Transaction
  • further, Transactions have inputs: TxInput, and outputs: TxOutput; and PartialTransactions have inputs: PartialTxInput(TxInput), and outputs: PartialTxOutput(TxOutput); instead of the old ugly input dicts
• and we ripped the old electrum custom partial tx format out of the code

Given that the lightning branch was now merged into master, and mostly due to that this PR had lots of hard to fix conflicts, we decided not to try to rebase this and fix conflicts. This is in large part of course our fault, as this PR should have been reviewed and progressed earlier, before it had developed huge conflicts, but we were focusing on other things, mostly lightning. This PR had seemed a huge task to review and get merged, even at the beginning; and now that I have worked on #5721 for more than a month, I can only tell that indeed it's a lot of work even just to understand the implications and get this kind of change merged. See the design decisions section in the other PR; all of which was not actually discussed here.

Rest assured, your work was not wasted, as I had spent several full days just thinking about code architecture, and this PR was a good reference for some of the decisions.