Repository navigation
Implement verification states, time locks and required consensus rules - #73
Conversation
b3d5c8c to
a186d35
Compare
0f048d5 to
facd4c9
Compare
facd4c9 to
5d13173
Compare
raphjaph
left a comment
raphjaph
left a comment
There was a problem hiding this comment.
It's been a while since I worked on the library and the BIP has been finalized now (which included a bunch of verification changes), so I'm just paging back everything into memory. Your PRs have also been sitting for a while and some of the following things might have been lost in the stuff I merged or are still upcoming in the other PRs you have.
This PR looks correct overall, some things to change/add:
- nVersion upgradeable rule never reports Inconclusive
- sign_pof doesn't set inputs[0].witness_utxo for segwit challenges
- Unused error variant Error::UnknownScriptType, let's remove it, just use Ok(Inconclusive)
Tests:
- verify_full_rejects_noncanonical_to_sign has case(|tx| tx.version = Version(3)); asserting Err(ToSignInvalid) (src/lib.rs:602), this should be Ok(Inconclusive) I think
- add a test for sign a PoF with LockParams and assert Valid { time, age }
- The InputVerification::Inconclusive => return Ok(Verification::Inconclusive) early-return in verify_pof's input loop is untested. Add a PoF whose proof input's script is uninterpretable (e.g. a witness-v2 program, or a non-multisig P2WSH) → Inconclusive. This also covers the parse_multisig failure → Inconclusive behavior change (previously Err), which nothing currently asserts
- add a test for LOW_S rejection
- add a test for MINIMALDATA rejection
- Same-txid non_witness_utxo fallback. The or_else fallback over earlier inputs in verify_pof is entirely untested. Two cases: positive -> two legacy PoF inputs spending different vouts of the same prev transaction, non_witness_utxo only on the first -> Ok; negative -> same setup but the second input's UTXO absent (or vice versa) -> Err(Error::ToSignInvalid).
| #[snafu(display("Cannot interpret script `{script}`"))] | ||
| UnknownScriptType { script: String }, |
There was a problem hiding this comment.
This doesn't seem to be used anywhere
|
|
||
| /// Enforces the LOW_S rule, a valid ECDSA signature must have a low-S value. | ||
| #[allow(clippy::result_large_err)] | ||
| pub fn require_low_s(signature: &bitcoin::secp256k1::ecdsa::Signature) -> Result<()> { |
There was a problem hiding this comment.
Let's add a test for this
| match verify_input(&to_sign, &[challenge_prevout], 0)? { | ||
| InputVerification::Inconclusive => Ok(Verification::Inconclusive), | ||
| InputVerification::Valid => { | ||
| // Upgradeable rule: nVersion must be 0 or 2, else inconclusive. | ||
| if to_sign.version != Version(0) && to_sign.version != Version(2) { | ||
| return Ok(Verification::Inconclusive); | ||
| } | ||
| Ok(Verification::Valid { | ||
| time: to_sign.lock_time, | ||
| age: to_sign.input[0].sequence, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This is good but we have the check_to_sign call before so this code never gets executed and therefore Ok(Inconclusive) never returned. Up to you how to structure this, we could combine it into check_to_sign or change the check_to_sign. Whatever is more ergonomic
| /// Verifies a BIP-322 full proof of funds from a spec-compliant string encoding. | ||
| /// Verifies a BIP-322 full proof of funds. | ||
| /// | ||
| /// See [`verify_pof`] for the expected contents of `prevouts`. |
There was a problem hiding this comment.
nit: given that prevouts was removed from this function maybe we can remove this comment too.
MINIMALIF is N/A (we never execute scripts; non-template scripts are Inconclusive). CLEANSTACK was enforced everywhere except P2TR — now fixed; duplicate outpoints in PoF are also rejected. Stale doc line removed. |
3 participants
Summary
Implement the spec's three-state verifier output, time lock support for full signatures, explicit enforcement of the required consensus rules, and spec-compliant prevout resolution for proof of funds.
Rebased on #71
Changes
Verificationenum (Valid { time, age }/Inconclusive).InputVerificationfor per-input outcomes.LockParamsto setnLockTimeandnSequenceonto_sign.verify_*functions now returnResult<Verification>, with the invalid state reported asErr.Notes for reviewers
Inconclusiverather than erroring.LockParamsversion is derived — 2 when any lock is set, else 0, so an inconsistent version can't be constructed. Threaded throughsign_full,sign_full_encoded,sign_pof,sign_pof_encodedandcreate_to_sign. The simple variant keeps all-zero locks per the spec.Closes #72