Some minor cleanup #31
Conversation
Signature hex was obtained by running the previous version of the test,
but with `println!("signature: {signature}"); panic!();` inserted before
the `Witness::new()` line. The output was collected by running the test
as follows `cargo test invalid_signature -- --show-output`.
ECDSA signature generation appears to be deterministic on my machine so
you ought to be able to verify this new constant yourself.
This makes the `network` parameter of `crate::reserves::verify_proof()` unused, it should probably be removed at the next opportunity to break the API.
Checking the challenge input is able to spend a fabricated OP_TRUE input doesn't verify anything useful. The challenge input is intentionally unspendable to begin with. The witness_utxo in the psbt for the challenge input is there for signers, not validators. The check that the challenge input txid is properly constructed is the important one.
|
Actually I think the CI failure might be caused by the release of crossbeam-utils 0.8.17 yesterday. |
Yes, that is very likely. Adding a line similar to https://github.com/bitcoindevkit/bdk-reserves/blob/main/.github/workflows/cont_integration.yml#L47 will likely fix it. I just checked. BDK 0.29 stuck with MSRV 1.57 which becomes funny to maintain. BDK 1 has MSRV 1.63 which should be easier. |
| @@ -192,7 +187,7 @@ pub fn verify_proof( | |||
| psbt: &PSBT, | |||
| message: &str, | |||
| outpoints: Vec<(OutPoint, TxOut)>, | |||
| network: Network, | |||
| _network: Network, | |||
There was a problem hiding this comment.
If the next release is for 0.29 I think it's ok to break the interface here by removing that parameter.
There was a problem hiding this comment.
I wasn't sure if/when you wanted to bump to the next BDK version, I had that in my next patch set which makes a number of more significant changes. I can include it here instead though.
There was a problem hiding this comment.
If it's in the next one, that's also fine. You mentioned before that you had something prepared with v0.29.
There was a problem hiding this comment.
Yeah my intention was to only make backwards compatible changes in this patch set
| if tx.output[0].script_pubkey != out_script_unspendable { | ||
| return Err(ProofError::InvalidOutput); | ||
| } | ||
|
|
||
| let serialized_tx = serialize(&tx); | ||
| // Verify the challenge input |
There was a problem hiding this comment.
I was wondering and thinking for a while, if there was a way to cheat in absence of this check. But all I came up with was something like in the tampered_proof_message() test, which shows that this kind of cheating is caught.
There was a problem hiding this comment.
Yeah I was hesitant to remove a test, but I'm pretty certain this is redundant. In bip-0127 verification is done over transactions, where the synthetic witness_utxo from the psbt wouldn't be available anyway.
|
It looked like you fixed the CI issues in your branch, do you have any outstanding reservations about this change set? |
Raising the MSRV from 0.57 to 0.63. Lower rust versions became increasingly difficult to maintain.
Happy to discuss any of the changes, I think they're pretty straight forward and uncontroversial, tests pass.
CI is broken for me, but main is also broken for me so I'm not certain that's relevant, or if I need to configure something in my fork repo.