Sighash method signatures could return no error or io::Error if it wasn't for input index argument

[stevenroose]

This is just a thought that annoys me, but I don't really have a proposed solution.

Currently methods like

    pub fn segwit_encode_signing_data_to<Write: io::Write>(
        &mut self,
        mut writer: Write,
        input_index: usize,
        script_code: &Script,
        value: u64,
        sighash_type: EcdsaSighashType,
    ) -> Result<(), Error> {

    pub fn segwit_signature_hash(
        &mut self,
        input_index: usize,
        script_code: &Script,
        value: u64,
        sighash_type: EcdsaSighashType,
    ) -> Result<SegwitV0Sighash, Error> {

Return this super generic sighash::Error type but in all cases it's either io::Error or this one special case

            let txin = &self.tx.borrow().input.get(input_index).ok_or(Error::IndexOutOfInputsBounds {
                index: input_index,
                inputs_size: self.tx.borrow().input.len(),
            })?;

If it wasn't for this case, the segwit_signature_hash method would not need to return an error at all the the first one could just return io::Error forwarded from the writer argument.

That's very unfortunate.

Oh going into the taproot variant, I see another exception to that rule that actually is a bug in the segwit variant:

            self.tx.borrow().output[input_index].consensus_encode(&mut single_enc)?;

That should also do a range check like it does in the taproot variant:

            self.tx
                .borrow()
                .output
                .get(input_index)
                .ok_or(Error::SingleWithoutCorrespondingOutput {
                    index: input_index,
                    outputs_size: self.tx.borrow().output.len(),
                })?
                .consensus_encode(&mut enc)?;

I can fix that.

Still, it seems that taproot has more error variants. Maybe we should have a SegwitError and TaprootError so that users of segwit can explicitly unwrap if indices are in range or something.

[stevenroose]

This MR fixes that one missing error for segwit: #1966

[apoelstra]

We could create an IndexedInput type which contained a reference to a specific vin as well a its index. Then constructing this may have a Result but it could be used in other methods that would become infallible.

[sanket1729]

Even more annoying part is that if we are writing to HashEngines as we do in the most common case, there is no possibility of io::Error. We can use the infaillable engine.input() methods.

impl io::Write for sha256::HashEngine {
    fn flush(&mut self) -> io::Result<()> { Ok(()) }

    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        self.input(buf);
        Ok(buf.len())
    }
}

The only reason segwit_signature_hash should fail is if the index is out of bounds. There is some work on trying to return only the error types that are reachable instead of giant parent enum. If that experiment is successful, we should adopt it for this sighash module.

See related:
Suggestion to improve ergonomics: rust-bitcoin/rust-secp256k1#633
Attempt of this at address module. #1950

cc @tcharding

[tcharding]

Thanks @sanket1729 , yeah read this yesterday and did some more work on the error stuff you link to. I rekon we need to get on and work this error stuff out.

[tcharding]

#2329 splits the general error up to a state that may be acceptable enough to close this.

[apoelstra]

I think we should close it regardless. Ultimately this is a "I wish Rust had dependent types" issue, but it doesn't, so there's nothing we can do. There is a trivial error variant that could in-principle be eliminated by extremely simple static analysis, but Rust can't do it, so we have to keep it.

[Kixunil]

Oh, only noticed this now. I had some ideas around this but forgot what they were. :D But one I have now is to use lgio which makes error generic so we can statically prove the IO case never happens in case of hashers.

[tcharding]

Cool, what is your hoped time frame on being able to use lgio?

[Kixunil]

Crazy idea: use both lgio and bitcoin-io temporarily? Though I originally thought lgio can be skipped entirely because of push_decode, then until I started writing this comment I thought it can't but now I think it actually probably can if we can change the code to return an encoder. (So instead of encode_signing_data_to we would have signing_data_encoder.)

[Kixunil]

Oh, I remembered one of the ideas: have a method that signs all inputs and sets the witnesses so at least in the case when one is signing all inputs the out of bounds errors can be removed.