fix converttopsbt permitsigdata arg, add basic test #14356

1. 
   instagibbs commented at 2:10 AM on September 30, 2018: member

   The final check for extraneous sigdata has a flipped boolean, resulting in incorrect behavior.

   Resolves https://github.com/bitcoin/bitcoin/issues/14355

2. fix converttopsbt permitsigdata arg, add basic test 88a79cb436
3. fanquake added the label RPC/REST/ZMQ on Sep 30, 2018
4. in test/functional/rpc_psbt.py:112 in 88a79cb436

    106 | @@ -107,6 +107,9 @@ def run_test(self):
    107 |          # Make sure that a psbt with signatures cannot be converted
    108 |          signedtx = self.nodes[0].signrawtransactionwithwallet(rawtx['hex'])
    109 |          assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].converttopsbt, signedtx['hex'])
    110 | +        assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].converttopsbt, signedtx['hex'], False)
    111 | +        # Unless we allow it to convert and strip signatures
    112 | +        self.nodes[0].converttopsbt(signedtx['hex'], True)
   

   ---

   ---

   

   MarcoFalke commented at 1:03 AM on October 1, 2018:

   Nit: could use named args permitsigdata=True/False?

5. 
   MarcoFalke commented at 1:03 AM on October 1, 2018: member

   Is this for backport?

6. 
   instagibbs commented at 1:51 AM on October 1, 2018: member

   Backport would be appreciated.

   On Sun, Sep 30, 2018, 9:06 PM MarcoFalke notifications@github.com wrote:

   @MarcoFalke commented on this pull request.

   Is this for backport?

   In test/functional/rpc_psbt.py https://github.com/bitcoin/bitcoin/pull/14356#discussion_r221480702:

   @@ -107,6 +107,9 @@ def run_test(self): # Make sure that a psbt with signatures cannot be converted signedtx = self.nodes[0].signrawtransactionwithwallet(rawtx['hex']) assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].converttopsbt, signedtx['hex'])

   •    assert_raises_rpc_error(-22, "TX decode failed", self.nodes[0].converttopsbt, signedtx['hex'], False)

   •    # Unless we allow it to convert and strip signatures

   •    self.nodes[0].converttopsbt(signedtx['hex'], True)

   Nit: could use named args permitsigdata=True/False?

   — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/14356#pullrequestreview-160136423, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC00wnXt81XHgEIjyE4tn73YFdXSBgks5ugWqRgaJpZM4XAxPO .

7. MarcoFalke added this to the milestone 0.17.1 on Oct 1, 2018
8. MarcoFalke added the label Needs backport on Oct 1, 2018
9. 
   achow101 commented at 3:49 PM on October 1, 2018: member

   utACK 88a79cb436b30b39d37d139da723f5a31e9d161b

10. 
    promag commented at 6:20 PM on October 1, 2018: member

    utACK 88a79cb.

11. in src/rpc/rawtransaction.cpp:1672 in 88a79cb436

    1668 | @@ -1669,7 +1669,7 @@ UniValue converttopsbt(const JSONRPCRequest& request)
    1669 |  
    1670 |      // Remove all scriptSigs and scriptWitnesses from inputs
    1671 |      for (CTxIn& input : tx.vin) {
    1672 | -        if ((!input.scriptSig.empty() || !input.scriptWitness.IsNull()) && (request.params[1].isNull() || (!request.params[1].isNull() && request.params[1].get_bool()))) {
    1673 | +        if ((!input.scriptSig.empty() || !input.scriptWitness.IsNull()) && !permitsigdata) {
    

    ---

    ---

    

    jimmysong commented at 10:15 PM on October 1, 2018:

    Would it make sense to have the !request.params[1].isNull() in line 1661 above?

    ---

    

    promag commented at 12:07 PM on October 10, 2018:

    Why? permitsigdata is enough?

12. 
    meshcollider commented at 4:01 AM on November 9, 2018: contributor

    utACK https://github.com/bitcoin/bitcoin/pull/14356/commits/88a79cb436b30b39d37d139da723f5a31e9d161b

13. 
    sipa commented at 6:10 PM on November 12, 2018: member

    utACK 88a79cb436b30b39d37d139da723f5a31e9d161b

14. laanwj merged this on Nov 12, 2018
15. laanwj closed this on Nov 12, 2018

    ---

16. laanwj referenced this in commit 47ed24cf8a on Nov 12, 2018
17. MarcoFalke referenced this in commit 7a590d8390 on Dec 5, 2018
18. MarcoFalke removed the label Needs backport on Dec 5, 2018
19. deadalnix referenced this in commit 5517f469f9 on May 11, 2020
20. UdjinM6 referenced this in commit 59b51a86e7 on Jul 15, 2021
21. UdjinM6 referenced this in commit 7d66e6647b on Jul 15, 2021
22. UdjinM6 referenced this in commit 47317e8fd0 on Jul 16, 2021
23. MarcoFalke locked this on Sep 8, 2021

Contributors

instagibbsMarcoFalkeachow101promagjimmysongmeshcollidersipa

Milestone
0.17.1