Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a utility method to psbt to compute find sighash type of a given input #838

Closed
sanket1729 opened this issue Feb 17, 2022 · 8 comments · Fixed by #847
Closed

Add a utility method to psbt to compute find sighash type of a given input #838

sanket1729 opened this issue Feb 17, 2022 · 8 comments · Fixed by #847

Comments

@sanket1729
Copy link
Member

For each psbt input, we have a field called sighash_ty.

pub sighash_type: Option<PsbtSigHashType>,

We should add a two new APIs to input

  • ecdsa_sighash_ty: Outputs Result<EcdsaSigHashType, Error>. If the sighash_ty field is none, output Ok(EcdsaSigHashType::All). Return err is the sighash type is non-standard.
  • schnorr_sighash_ty: Same as above but for schnorr signatures.
@rish-singhal
Copy link
Contributor

rish-singhal commented Feb 22, 2022

Hi @sanket1729, I am new to contributing to this repository and a participant in Summer of Bitcoin. I would like to solve this issue. Also, am I going in the correct direction? just wanted to confirm

impl Input {
    /// Returns the ['EcdsaSigHashType'] if the sighash_type is None else
    /// NonStandardSigHashType Error
    pub fn ecdsa_hash_ty(&self) -> Result<EcdsaSigHashType, psbt::Error> {
        if let Some(sighash_type_value) = self.sighash_type {
            Err(Error::NonStandardSigHashType(sighash_type_value.inner))
        } else {
            Ok(EcdsaSigHashType::All)
        }
    }

    /// Returns the ['SchnorrSigHashType'] if the sighash_type is None else
    /// NonStandardSigHashType Error
    pub fn schnorr_hash_ty(&self) -> Result<SchnorrSigHashType, psbt::Error> {
        if let Some(sighash_type_value) = self.sighash_type {
            Err(Error::NonStandardSigHashType(sighash_type_value.inner))
        } else {
            Ok(SchnorrSigHashType::All)
        }
    }
....

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 22, 2022

This doesn't look correct. I think it should be something like Ok(self.sighash_type.map(|sighash_type| sighash_type.ecdsa_hash_ty()).transpose()?.unwrap_or(EcdsaSigHashType::All))

@sanket1729
Copy link
Member Author

Hi @rish-singhal , thanks for taking the time to look at this. The algorithm should be

  • If sighash_ty is Some, call .ecdsa_sighash_ty on the inner returning the result
  • If it is None, return EcdsaSigHashtype::All.

SImilarly for Schnorr. Also, note that the None case of schnorr should be SchnorrSignatureHashType::Default. Feel to ask more questions here or you can approach me on the summer of bitcoin discord for more assistance on this.

@rish-singhal
Copy link
Contributor

Hi @rish-singhal , thanks for taking the time to look at this. The algorithm should be

  • If sighash_ty is Some, call .ecdsa_sighash_ty on the inner returning the result
  • If it is None, return EcdsaSigHashtype::All.

SImilarly for Schnorr. Also, note that the None case of schnorr should be SchnorrSignatureHashType::Default. Feel to ask more questions here or you can approach me on the summer of bitcoin discord for more assistance on this.

Hi thanks for your reply, does the following work?

impl Input {
    /// Returns sighash_type.ecdsa_hash_ty() is sighash_type exists else 
    /// ['EcdsaSigHashType::All']
    pub fn ecdsa_hash_ty(&self) -> Result<EcdsaSigHashType, NonStandardSigHashType> {
        if let Some(sighash_type) = self.sighash_type {
            sighash_type.ecdsa_hash_ty()
        } else {
            Ok(EcdsaSigHashType::All)
        }
    }

    /// Returns sighash_type.schnorr_hash_ty() is sighash_type exists else 
    /// ['SchnorrSigHashType::Defaul']
    pub fn schnorr_hash_ty(&self) -> Result<SchnorrSigHashType, sighash::Error> {
        if let Some(sighash_type) = self.sighash_type {
            sighash_type.schnorr_hash_ty()
        } else {
            Ok(SchnorrSigHashType::Default)
        }
    }
...

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 24, 2022

That looks good. I like match a bit more when None is handled but not very important.

@rish-singhal
Copy link
Contributor

rish-singhal commented Feb 24, 2022

That looks good. I like match a bit more when None is handled but not very important.

Thanks! yes you are right, it makes it more explicit that it is a None case, would update the code with that. Also, just a question is it ok to use closures and more of a functional programming style of code for the PRs, I am asking as I am new to contributing to this repository? I really liked your suggestion above.

@Kixunil
Copy link
Collaborator

Kixunil commented Feb 24, 2022

I don't think there are hard rules. I personally prefer functional style but there are cases when match looks better. I suspect this is one of them. For iterators, functional has better performance.

Also my suggestion above is a bit complex, this is probably better:

self.sighash_type
    .map(|sighash_type| sighash_type.ecdsa_hash_ty())
    .unwrap_or(Ok(EcdsaSigHashType::All))

@sanket1729
Copy link
Member Author

@rish-singhal, I too prefer functional styles. Feel free to open a PR to start an implementation discussion there.

rish-singhal pushed a commit to rish-singhal/rust-bitcoin that referenced this issue Feb 25, 2022
Fixes rust-bitcoin#838: Add a utility method to psbt to compute find sighash
type of a given input.
rish-singhal pushed a commit to rish-singhal/rust-bitcoin that referenced this issue Feb 25, 2022
Fixes rust-bitcoin#838: Add a utility method to psbt to compute find sighash
type of a given input.
rish-singhal pushed a commit to rish-singhal/rust-bitcoin that referenced this issue Feb 25, 2022
Fixes rust-bitcoin#838: Add a utility method to psbt to compute find sighash
type of a given input.
rish-singhal pushed a commit to rish-singhal/rust-bitcoin that referenced this issue Feb 25, 2022
Fixes rust-bitcoin#838: Add a utility method to psbt to compute find sighash
type of a given input.
apoelstra added a commit that referenced this issue Feb 27, 2022
fb04cab Add a method to psbt to compute find sighash type (Rishabh Singhal)

Pull request description:

  Fixes #838: Add a utility method to psbt to compute find sighash
  type of a given input.

  For now, I have changed my previous implementation as discussed in #838 to functional style code as suggested by @Kixunil.

ACKs for top commit:
  apoelstra:
    ACK fb04cab
  Kixunil:
    ACK fb04cab

Tree-SHA512: 86184649e7a309348cb217347b82bf39c9997ae259fe7881322038a88bd04deab927bede1dd71d17496bac420353a3fd07e7d191ff4671a07754c02a38dd1319
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants