test: add test for decoding PSBT with per-input preimage types #25625

theStack commented at 2:15 PM on July 16, 2022: contributor

This PR adds missing test coverage for the decodepsbt RPC in the case that a PSBT with on of the per-input preimage types (PSBT_IN_RIPEMD160, PSBT_IN_SHA256, PSBT_IN_HASH160, PSBT_IN_HASH256; see BIP 174) is passed. As preparation, the first four commits move the already existing helpers for (de)serialization of PSBTs and PSBTMaps from the signet miner to the test framework (in a new module psbt.py), which should be quite useful for further tests to easily create PSBTs.

theStack force-pushed on Jul 16, 2022

DrahtBot commented at 3:26 PM on July 16, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23127 (tests: Use test framework utils where possible by vincenzopalazzo)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jul 16, 2022

brunoerg commented at 1:48 PM on July 18, 2022: contributor

Concept ACK

achow101 commented at 8:19 PM on July 18, 2022: member

ACK 4dc573e78976dc2728464714a589f4bc2a07e37d

fanquake requested review from instagibbs on Jul 18, 2022

brunoerg approved

brunoerg commented at 12:49 PM on July 19, 2022: contributor

crACK 4dc573e78976dc2728464714a589f4bc2a07e37d

DrahtBot added the label Needs rebase on Jul 19, 2022

instagibbs commented at 1:06 PM on July 19, 2022: member

concept ACK, moving the PSBT utility functions into the testing framework proper is something I wanted regardless

scripted-diff: rename `FromBinary` helper to `from_binary` (signet miner)

-BEGIN VERIFY SCRIPT-
sed -i s/FromBinary/from_binary/g ./contrib/signet/miner
-END VERIFY SCRIPT-

597a4b35f6

refactor: move `from_binary` helper from signet miner to test framework

Can be easily reviewed with `--color-moved=dimmed-zebra`.

7c0dfec2dd

refactor: move PSBT(Map) helpers from signet miner to test framework

Can be easily reviewed with `--color-moved=dimmed-zebra`.

1b035c03f9

test: add constants for PSBT key types (BIP 174)

Also take use of the constants in the signet miner to get rid of
magic numbers and increase readability and maintainability.

fdc1ca3896

theStack force-pushed on Jul 19, 2022

theStack commented at 1:45 PM on July 19, 2022: contributor

Rebased on master (due to trivial merge conflict in rpc_psbt.py after commit d2ed97656bba050051cfc677f1fa7eb3fc633f7d, #25590), should be easy to re-review.

in test/functional/rpc_psbt.py:793 in 94dbcdc5c7 outdated

 787 | @@ -775,5 +788,37 @@ def test_psbt_input_keys(psbt_input, keys):
 788 |              self.nodes[0].sendrawtransaction(rawtx)
 789 |              self.generate(self.nodes[0], 1)
 790 |  
 791 | +        self.log.info("Test decoding PSBT with per-input preimage types")
 792 | +        # note that the decodepsbt RPC doesn't check whether preimages and hashes match
 793 | +        hash_ripemd160, preimage_ripemd160 = os.urandom(20), os.urandom(50)

instagibbs commented at 1:59 PM on July 19, 2022:

this is the first instance of urandom in the test framework. Can we make it seed-determined instead, so that failures can be diagnosed easier?

If we don't expect failures due to randomness, we probably shouldn't do it at all then.

theStack commented at 3:50 PM on July 19, 2022:

Good point, I agree that seed-determined randomness is preferred here. Moved the random_bytes helper (which uses random.getrandbits) from the taproot test to the utils library and used that instead. Note that Python 3.9 offers random.randbytes(), but unfortunately we have Python 3.6 as minimum requirement.

instagibbs commented at 2:01 PM on July 19, 2022: member

looks good aside from one question of mine

DrahtBot removed the label Needs rebase on Jul 19, 2022

refactor: move helper `random_bytes` to util library

Can be easily reviewed with `--color-moved=dimmed-zebra`.

faf43378e2

test: add test for decoding PSBT with per-input preimage types 71a751f6c3

theStack force-pushed on Jul 19, 2022

theStack commented at 3:55 PM on July 19, 2022: contributor

Force-pushed with feedback taken from instagibb's review comment, i.e. the random bytes are now generated with the random module and hence could be reproduced by passing a PRNG seed (which is printed out in the test framework log).

achow101 commented at 8:12 PM on July 20, 2022: member

ACK 71a751f6c3e8912e1b1cfe388e593309d210e576

achow101 merged this on Jul 20, 2022

achow101 closed this on Jul 20, 2022

theStack deleted the branch on Jul 20, 2022

sidhujag referenced this in commit a0b9c4f356 on Jul 21, 2022

bitcoin locked this on Jul 20, 2023