test: refactor rpc_signrawtransaction and add logging #18545

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:refactor-rpc_signrawtransaction changing 1 files +13 −22

jonatack commented at 7:13 PM on April 6, 2020: member

As a follow-up to #18484, the new tests are good but bury the one non-duplicate line in each test that sets the witness script, and there is no logging in the testfile. This PR makes it easy to see what is unique to each of the new tests and adds logging.
DrahtBot added the label Needs rebase on Apr 6, 2020
jonatack force-pushed on Apr 6, 2020
DrahtBot removed the label Needs rebase on Apr 6, 2020
fanquake added the label Tests on Apr 6, 2020

in test/functional/rpc_signrawtransaction.py:173 in 269b664ced outdated

 186 | -        # Check the signing completed successfully
 187 | -        assert 'complete' in spending_tx_signed
 188 | -        assert_equal(spending_tx_signed['complete'], True)
 189 | -        self.nodes[0].sendrawtransaction(spending_tx_signed['hex'])
 190 | +        # Now test with P2PKH and P2PK scripts as the witnessScript
 191 | +        for tx_type in ['P2PKH', 'P2PK']:  # these tests are order-independant

theStack commented at 5:24 PM on April 10, 2020:

nit: s/independant/independent

jonatack commented at 10:13 AM on April 12, 2020:

good eye -- done

in test/functional/rpc_signrawtransaction.py:53 in 5390ee10a9 outdated
```
  50 | @@ -50,6 +51,7 @@ def successful_signing_test(self):
  51 |  
  52 |      def test_with_lock_outputs(self):
  53 |          """Test correct error reporting when trying to sign a locked output"""
```
theStack commented at 5:30 PM on April 10, 2020:

This comment is redudant to the introduced logging message and could be removed? (Same as for witness_script_test() below).

jonatack commented at 10:13 AM on April 12, 2020:

done
theStack commented at 5:37 PM on April 10, 2020: member

ACK modulo nits Deduplication and better logging are always good :+1:

For other reviewers, I suggest looking at the refactor diff with a larger number of lines of context via the -U option (at a first glance it looks like the signing part has been removed, as one doesn't see the whole newly introduced function verify_txn_with_witness_script() to the end), e.g. git show -U20 269b664ced
test: refactor rpc_signrawtransaction witness script tests
to see what is distinct in each test.
4d6cde38ce
test: add rpc_signrawtransaction logging 9cdddae3b4
jonatack force-pushed on Apr 12, 2020
jonatack commented at 10:16 AM on April 12, 2020: member

Thanks for reviewing, @theStack. I've updated with both of your suggestions.
theStack approved
theStack commented at 1:42 PM on April 12, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18545/commits/9cdddae3b4efee071d71ba3b6629a53017332f6f :egg: :rabbit:
MarcoFalke merged this on Apr 13, 2020
MarcoFalke closed this on Apr 13, 2020
jonatack deleted the branch on Apr 13, 2020
Fabcien referenced this in commit b36ecbb6b1 on Jan 15, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

jonatack

theStack

Labels

Tests

Linked (view graph)

#18484 rpc: Correctly compute redeemScript from witnessScript for signrawtransaction