Upstream bdk sighash calculation code #946
Conversation
In order to sign a utxo that does a p2wpkh spend we need to create the script that can be used to create a sighash. In the libbitcoin docs this is referred to as the 'script code' [0]. The script is the same as a p2pkh script but the pubkey_hash is found in the scriptPubkey. Add a `Script` conversion method that checks if `self` is a v0 p2wpkh script and if so extracts the pubkey_hash and returns the required script. [0] https://github.com/libbitcoin/libbitcoin-system/wiki/P2WPKH-Transactions#spending-a-p2wpkh-output
Add methods to get the sighash from a PSBT prior to signing, one for legacy inputs and one for segwit v0 inputs. This code is copied directly from bdk, attribution and licensing needs seeing to.
|
Maybe some/all of this code belong to rust-miniscript? @apoelstra ? If it's ok here I think an attribution naming bdk is due, I think naming the whole project instead of individual author is fine. @afilini do you agree? |
|
You definitely deserve attribution, this code is non-trivial to work out! Would you guys like your license on the file too? bdk uses Apache and rust-bicoin uses CC0? |
| @@ -427,6 +427,23 @@ impl Script { | |||
| Script::new_p2sh(&self.script_hash()) | |||
| } | |||
|
|
|||
| /// Returns the script code used for spending a P2WPKH output if this script is a script pubkey | |||
| /// for a P2WPKH output. | |||
| pub fn p2wpkh_script_code(&self) -> Option<Script> { | |||
There was a problem hiding this comment.
Yes, please. I had to do this downstream in an ugly way
// Get a script from witness script pubkey hash
fn script_code_wpkh(script: &Script) -> Script {
assert!(script.is_v0_p2wpkh());
// ugly segwit stuff https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#specification
let mut script_code = vec![0x76u8, 0xa9, 0x14];
script_code.extend(&script.as_bytes()[2..]);
script_code.push(0x88);
script_code.push(0xac);
Script::from(script_code)
}There was a problem hiding this comment.
Moved to a separate PR since its a discreet change: #954
| @@ -427,6 +427,23 @@ impl Script { | |||
| Script::new_p2sh(&self.script_hash()) | |||
| } | |||
|
|
|||
| /// Returns the script code used for spending a P2WPKH output if this script is a script pubkey | |||
| /// for a P2WPKH output. | |||
| pub fn p2wpkh_script_code(&self) -> Option<Script> { | |||
There was a problem hiding this comment.
This should also link to BIP143.
There was a problem hiding this comment.
Added link in #954 as suggested.
| @@ -183,6 +185,103 @@ impl PartiallySignedTransaction { | |||
|
|
|||
| Ok(()) | |||
| } | |||
|
|
|||
| // TODO: Work out attribution/license stuff, copied from `impl ComputeSighash for Legacy` in `bdk`. | |||
There was a problem hiding this comment.
I think we can ask bdk for relicensing stuff here. They are super-friendly people and (I think) they would not mind doing it. But we should have attribution here.
There was a problem hiding this comment.
I've raised an issue on bdk repo to get input: bitcoindevkit/bdk#586
| // TODO: Work out attribution/license stuff, copied from `impl ComputeSighash for Legacy` in `bdk`. | ||
| /// Returns the sighash (and type) for input at `input_index`, caller is | ||
| /// responsible for ensuring `input_index` is a legacy input. | ||
| pub fn legacy_sighash(&self, input_index: usize) -> Result<(Sighash, EcdsaSighashType), Error> { |
There was a problem hiding this comment.
I think we can actually do better :) . See https://docs.rs/miniscript/7.0.0-rc.2/miniscript/psbt/trait.PsbtExt.html#tymethod.sighash_msg . This figures out what the type of spent input is and applies the corresponding signature hash algorithm.
I was planning to upstream this but forgot about it.
|
Please also see the discussion rust-bitcoin/rust-miniscript#305 (comment) and the following comments. I think we should have signer support like @JeremyRubin suggested there. I did not review deeply his suggestion, but I do think we need some form of API to allow the signers to sign the psbt and insert the signature. /// Note that this should handle all types of spends (legacy, segwitv0, taproot) and both types of signature algorithms(schnorr/ecdsa)
pub fn sign_psbt(&mut self, sk: bitcoin::PrivateKey); |
|
Thanks for the links @sanket1729, I'm not watching
|
|
+1 for having sighash computation code for PSBTs in rust-bitcoin. This already exists in rust-miniscript though. Have you seen this: https://docs.rs/miniscript/7.0.0-rc.2/miniscript/psbt/trait.PsbtExt.html#tymethod.sighash_msg Any reason this couldn't be plucked from rust-miniscript into rust-bitcoin @sanket1729? [EDIT] NVM it can be: #946 (comment) Note for signing in case I forget: we also made an xkey signer which does on the fly derivation from the master and then signs in (this can be copied without attribution). |
|
Cheers @LLFourn, I'm on it. |
I think we should attribute it in a comment. But I don't think we should be copying around license files everywhere. |
Feel free to praise me for my fabulous code. just pointing out the BSD0 LICENSE doesn't require attribution. |
bitcoindevkit's bdk has some code in it for computing the sighash of PSBT inputs during signing (and some nice signing code too). I first came across this code a few years back and wondered why it was not upstream. This last week @DanGould and myself have been working on some PSBT examples/tests and for them I shamelessly stole the code out of
bdkfor signing.This PR is an exploration to see if we want this code in
rust-bitcoin. I'm no where near fully understanding what goes where in PSBTs, or sighashes even, so this will need a fair bit of review.I have added a
TODOto add attribution and/or the bdk license, not sure if this is enough code to warrant that. @RCasatta can you speak for bdk on this matter?So far this is just the sighash code, I rekon we could use the PSBT signing code as well possibly but I'll wait to see how this is received.
Thanks