Add support for P2WSH P2WPKH transaction

[QuincySx]

Add support for P2SH P2WPKH transaction
Did not complete the isolation testimony tx_id generation

[karask]

Hi @QuincySx and thank you for the contribution!

I was actually in the middle of implementing segwit support but only worked/tested the SIGHASH_ALL type. Yours seems to be more complete so I will be happy to include it.

However, I need to have the appropriate tests implemented. I typically create a tx (e.g. p2wpkh) with bitcoin core and get the raw transaction and then create a test that creates the same transaction using python-bitcoin-utils and compare them.

I would create a test_p2wpkh_txs.py that will test single and multi-input segwit transactions using different SIGHASHs (check test_p2pkh_txs.py). Accordingly, for p2wsh.

Another minor point is that I try to be consistent throughout the codebase with regard to syntax, naming and documentation.
e.g. naming: I would have has_segwit instead of hasSegwit, basic_sig_hash_type instead of basicSigHashType, etc.
e.g. documentation: has_segwit would need to be added to the doc string of Transaction class, etc.

This is intended as an educational library so more inline comments are encouraged. For example, get_transaction_segwit_digest() would need some inline comments to explain the different cases (I have a TODO to improve the doc in the current get_transaction_digest() as well).

I also see that you added an "amount" parameter in TxInput. Why would that be needed? I see that is not used anywhere though.

Again, I really appreciate your time and effort on this. But I want to include as tested code as possible before I merge to master. Next time I devote time on this I might work on the above. Until then, if you have time to commit the above changes then it would be great!

Cheers!

[QuincySx]

1. I'm a Java programmer, and my naming style is somewhat java-oriented. I'll take the air conditioner for a while.
2. transactions.py line 460 uses txin amount

[QuincySx]

I have written a test code for various transfers, but the signature type is just SIGHASH_ALL .
segwit demo

[karask]

I was referring to unit tests like those in tests/ dir
e.g. https://github.com/karask/python-bitcoin-utils/blob/master/tests/test_p2pkh_txs.py

[karask]

Regarding amount I see where it is used now but it does not make sense in TxInput class. Maybe you meant to pass it when calling sign_segwit_input() and then get_transaction_segwit_digest()

[QuincySx]

Hi.
The test case has been added, but the isolation testimony get_txid() function still has some problems.

[QuincySx]

I'll add another example of mixing tomorrow, merging.

[QuincySx]

In addition to the comments basically can also

[karask]

Hi @QuincySx and thanks for the update!

I just saw that you also have witnesses in TxInput. Similarly to the amount change that I requested above, the witnesses are not really part of TxInput. Instead, it should go to the Transaction class since they are serialized outside of the txins. i.e. it fits better in:

class Transaction:
    def __init__(self, inputs=[], outputs=[], witnesses=[], locktime=DEFAULT_TX_LOCKTIME,
                 version=DEFAULT_TX_VERSION):

Let me know when you are done updating/adding to the PR and I can then do final testing and cleanup (if needed) before I merge.

I also saw some comments in Chinese... could you please translate to English?

Cheers

[QuincySx]

From the point of view, witnesses should belong to the transaction, I think the witnesses are more reasonable in TxInput.

[karask]

As you say, from a user/dev perspective it might by more intuitive to have them in the TxInput.

Strictly speaking though, when witnesses are serialized they are not part of the inputs... they are serialized as part of the Transaction (after the outputs).

I am not insisting on this though. Let me know when you are done with the changes and I can test things further.

Thanks

[karask]

Thanks!

For some reason the tx-related tests are failing. I will check it out later.

[QuincySx]

Sorry, there was a parameter change in the test.

[QuincySx]

Hi.
I am having a problem now, the test_p2wpkh_txs.py and test_p2wsh_txs.py overall tests fail, and one test can pass. I speculate that there may be a problem with the copy() method of Transaction. I don't know much about Python. If you have time, you can help me look at it.

[karask]

The test that passes is the non-segwit one (def test_signed_send_to_p2wpkh).

The Transaction.copy() is correct. I quickly double checked by using a library's copy method. e.g. in class Transaction:

import copy

...

tmp_tx = copy.deepcopy(self)
(instead of tmp_tx = Transaction.copy(self) )

I got the exact same issues using deepcopy above.

Were the tests working before you moved the witnesses to Transaction class?
And were the raw tx results in the tests come from BitcoinCore created transactions?

I will try to look at it further next week.

[QuincySx]

It is normal to test the test before moving the witness to the transaction class.

[QuincySx]

Creating a transaction specifies that witnesses = [] is no problem.
I don't understand the specific reasons. If you have a better solution, you can tell me.

[karask]

Can you revert to a version that the tests were working as expected (e.g. when witness was in TxInput) ? I could then move witness to Transaction myself and understand what is breaking them.

As it is now, I would have to rework all the logic.

[QuincySx]

There is no problem now, you can run the test case directly.