Check index out of bound also for tx inputs not only for psbt inputs

[RCasatta]

Description

A malformed PSBT, with more inputs in psbt.inputs than in psbt.global.unsigned_tx.inputs or viceversa can cause an out of bound panic

Notes to the reviewers

Note this malformed PSBT is not deserialized from rust-bitcoin, failing with ParseFailed("data not consumed entirely when explicitly deserializing")' so it's probably impossible to raise the panic in normal usage condition, it's sane to have the check though.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

Bugfixes:

• I've added tests to reproduce the issue which are now passing

[tcharding]

Small comment on the testing, feel free to ignore me :)

[tcharding]

Relying on this unwrap to indicate that the test passes while having unwrap calls further up the test can give a false positive i.e., the test can pass due to one of the earlier unwraps panicking.

Using an assertion on is_err and removing the should_panic makes the test more robust.

assert!(wallet.sign(&mut psbt, options).is_err());

But looses the error type check, I came up with the rather ugly:

        match wallet.sign(&mut psbt, options) {
            Err(Error::Signer(e)) => assert_eq!(e, SignerError::InputIndexOutOfRange),
            Err(e) => panic!("Wrong error type: {:?}", e),
            Ok(_) => panic!("sign() should have failed"),
        }

[RCasatta]

Yes, you are right, but it's a lot of code...

If we want to be more precise, I think we should consider using https://docs.rs/crate/assert_matches/1.5.0 as a dev-dep which will also be standardized one day https://doc.rust-lang.org/nightly/std/macro.assert_matches.html

it would become something like:
assert_matches!(wallet.sign(&mut psbt, options).unwrap_err(), SignerError::InputIndexOutOfRange)

@afilini ?

[afilini]

I think we have a lot of tests that use the should_panic, so I wouldn't bother doing anything different here.

It makes sense to add assert_matches, but that should be a different PR that also updates all the other tests.

Also ideally we should avoid unwrapping completely if we use it, so the check could be against the Result as well instead of just the error variant:

assert_matches!(wallet.sign(&mut psbt, options), Err(Error::Signer(SignerError::InputIndexOutOfRange)))

[afilini]

I'm gonna merge this to master and the current release branch as well