[0.20] wallet: Simplify and fix CWallet::SignTransaction #19740

1. 
   achow101 commented at 1:39 AM on August 17, 2020: member

   Backport CWallet::SignTransaction from master which is simpler and not broken.

   Previously CWallet::SignTransaction would return false for a fully signed transaction. This is a regression and obviously incorrect - a fully signed transaction is always complete. This occurs because CWallet::SignTransaction would iterate through each input and skip any further checks if the input was already signed. It would then end up falling through to the return false catch-all thus erroneously saying a fully signed transaction is incomplete. The change to attempting to use all ScriptPubKeyMans fixes this problem since the LegacyScriptPubKeyMan (the only spkm implemented in 0.20) will verify inputs during its signing attempt and correctly return that it is complete when the inputs verify. Thus a fully signed transaction will be correctly identified as complete, LegacyScriptPubKeyMan::SignTranaction will return true, and so CWallet::Transaction will return true too.

   Note that this is not a backport of any specific commit. Rather it is the end result of the changes we have made to this function in master.

   Fixes #19737

2. Simplify and fix CWallet::SignTransaction

   Backport CWallet::SignTransaction from master which is simpler and not
   broken.

   2d48d7dcfb
3. fanquake added the label Wallet on Aug 17, 2020
4. fanquake added this to the milestone 0.20.2 on Aug 17, 2020
5. fanquake deleted a comment on Aug 17, 2020
6. DrahtBot added the label Backport on Aug 23, 2020
7. MarcoFalke removed the label Wallet on Aug 23, 2020
8. 
   fjahr commented at 1:32 PM on September 1, 2020: member

   utACK 2d48d7dcfb93eeec36dd6f5cbad289b89ec69324

9. 
   MarcoFalke commented at 10:54 AM on October 19, 2020: member

   I'd feel more comfortable if all the individual commits were backported instead of a effectively squashed version without reference to the original commits

10. 
    achow101 commented at 3:26 PM on October 19, 2020: member

    I'd feel more comfortable if all the individual commits were backported instead of a effectively squashed version without reference to the original commits

    There aren't really any individual commits to backport. This end result is essentially an accident and a product of the refactors and changes made for descriptor wallets. To backport those commits would be nuts and bring in a bunch of things that aren't needed.

11. fanquake requested review from meshcollider on Nov 5, 2020
12. 
    MarcoFalke commented at 9:08 AM on November 18, 2020: member

    Concept ACK

13. 
    achow101 commented at 7:53 PM on December 3, 2020: member

    Updated the OP and cherry-picked #20562 for the test for this.

14. 
    MarcoFalke commented at 2:58 PM on December 4, 2020: member

    Checked that the test fails without the code changes:

    ./test/functional/rpc_signrawtransaction.py 
    2020-12-04T14:55:24.933000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_q8rf0stk
    2020-12-04T14:55:26.373000Z TestFramework (INFO): Try with a P2PKH script as the witnessScript
    2020-12-04T14:55:26.389000Z TestFramework (INFO): Try with a P2PK script as the witnessScript
    2020-12-04T14:55:26.741000Z TestFramework (INFO): Test signing a fully signed transaction does nothing
    2020-12-04T14:55:26.946000Z TestFramework (ERROR): Assertion failed
    Traceback (most recent call last):
      File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/test_framework.py", line 112, in main
        self.run_test()
      File "/home/marco/workspace/btc_bitcoin_core/./test/functional/rpc_signrawtransaction.py", line 227, in run_test
        self.test_fully_signed_tx()
      File "/home/marco/workspace/btc_bitcoin_core/./test/functional/rpc_signrawtransaction.py", line 158, in test_fully_signed_tx
        assert_equal(signedtx2["complete"], True)
      File "/home/marco/workspace/btc_bitcoin_core/test/functional/test_framework/util.py", line 46, in assert_equal
        raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    AssertionError: not(False == True)
    

    Approach ACK a4b56df42e42ad304571ed82648893484accfa5e 🥔

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    Approach ACK a4b56df42e42ad304571ed82648893484accfa5e 🥔
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUho6AwAy4I+vrdOq7pk7ML0Dehz3qSStFH7iRuDJgJ8Y51p1BiD5KyVeT0gh0vj
    ZJ3Cidqnw5BF6oDhFRGWCu1SZyy3yiNFBUqR/HhqSprpSenYcm2BXvCHuKl2BjNh
    DqDMsER379Joc3vdIqFBDspyQceUS+4gvkqsgn9tct1kz2Fpzw0ZXo5FPKpj+7Pd
    hs7lvWb8UUWRcmk7++UBRB2lxSdlIIs/avgZzsdvQxk8sEQ5qLkqp2Z/LBhMuVtt
    10DEhmAoX73zfCKYV5zTj+XXMRWVlZzxZoeslxco60CmwuMRZcMTrolvQJP0oduE
    YxlL5G5l4dANoSO9jnb068ANALLK7Zzl3Ku/OM4d6ag4Yzt6EU1RkdVLSXZ66hwu
    ACyqsAWlioxfCYDpXmDpjoCvobWap4Ot9Tg+1m4AwItzbRaEdVzPlCRmesvqGnUI
    i3xuPwTPaFSBDdvzZHjSyuNYCsSwDokiWRNANwgY0e75eY4fYxUs3LJEpcs8QlgJ
    arovBtKa
    =W9jc
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash c79fcd31a097ea698807dc5bea3f3e1c7255db211721590c8c645e427a0c180a -

    </details>

15. tests: Test that a fully signed tx given to signrawtx is unchanged

    Tests that a fully signed transaction given to
    signrawtransactionwithwallet is both unchanged and marked as complete.
    This tests for a regression in 0.20 where the transaction would not be
    marked as complete.
    
    Github-Pull: #20562
    Rebased-From: 773c42b265fb2212b5cb8785b7226a206d063543

    6a326cf66f
16. achow101 force-pushed on Dec 5, 2020
17. 
    achow101 commented at 6:51 PM on December 5, 2020: member

    Updated to backport #20562

18. 
    fjahr commented at 9:14 PM on December 5, 2020: member

    Code review ACK 6a326cf66f04948ffe729e5540a69e159a7840ad

    Also verified that the test fails without the fix.

19. 
    MarcoFalke commented at 9:06 AM on December 6, 2020: member

    re-ACK 6a326cf66f 👓

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA512
    
    re-ACK 6a326cf66f 👓
    -----BEGIN PGP SIGNATURE-----
    
    iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    pUhTyAwAnFnihLBRb563G4JDJxQh4cCfRgpRXR3ue1ethB9Rz7Nj8LwlM83+pjCT
    E2W0YwFdqo+qnQ2IbqJ5KwA5v0HxUow0dG7/f6CY33JP26YTHCfchg2a88WflcB+
    PHdffVINZoEUavW4yAc6bQeF6qmlEWLTTbGaeRgZBpKvNWJcLkh1FlvS/7upg3kM
    Qiyk4u7/ORYlGzhg5FvYqbVBbaDAZINIwACli1TwdSg0tbgNdojHFFMYdy6b7o6/
    TMELRZy0HRfwDJBSwaVg1RuLQQnfOL1qmafXdSkfTR9RKLc2kPofR5hpxGBCHQKR
    HS3AQ8vsETMDyoBaWfC1lAcDdhOdk/iFOLJFP2GSY9MHArZdF1uea/nOD7AhmMF5
    mCR7/XahkSyviwwVwJidNN2cFaz/k2Yac8/8AAyMMx9wEnkodzifAo7M88pmcEay
    bt9Tg7TrHfvXMEFuoF3Ofy4iF+9KNZrKeLbIzW5iviS6YEppWgRlF059/Ffw7vny
    INa4mZw9
    =uNHT
    -----END PGP SIGNATURE-----
    

    Timestamp of file with hash 04a77f1e983fe92e78f64282f06846538107aa60448a3aa30dd8a20609188ffb -

    </details>

20. MarcoFalke merged this on Dec 10, 2020
21. MarcoFalke closed this on Dec 10, 2020

    ---

22. DrahtBot locked this on Feb 15, 2022

Review Requested
meshcollider

Milestone
0.20.2