Skip to content

feat/verification enum - #84

Open
aagbotemi wants to merge 3 commits into
rust-bitcoin:masterfrom
aagbotemi:feat/verification-enum
Open

aagbotemi wants to merge 3 commits into
rust-bitcoin:masterfrom
aagbotemi:feat/verification-enum

Conversation

@aagbotemi

Copy link
Copy Markdown
Contributor

Summary

BIP-322 defines three validator outputs: valid at time T and age S, inconclusive, and invalid. Two of those lived in Verification while the third was reported as Err, so the spec's own state machine was split across a Rust idiom. Verification now carries all three.

Changes

  • Added Invalid(Error) variant to Verification.
  • Added a Box around TransactionExtract's source.
  • Removed #[allow(clippy::result_large_err)] from every function.

Notes for reviewers

Invalid carries the full Error, which means Verification can no longer derive Clone, Copy, PartialEq or Eq. Error implements none of them, and bitcoin::io::Error can't.

Closes #83
Closes #58

@aagbotemi
aagbotemi force-pushed the feat/verification-enum branch from 9d90e6b to 9447c48 Compare September 29, 2026 17:14
@raphjaph

raphjaph commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator

This looks good overall. I just merged the changes from #77. A couple thoughts for you since you are a consumer of this library:

  1. create_to_sign and extract_tx are more internal integrity failures rather than Verifiction::Invalid so maybe they keep their error variants? I don't feel strongly about this and the way it is right now seems okay to me.

  2. check_to_sign only ever produces Error::ToSignInvalid so maybe we don't need the Option there?

  3. This is an existing footgun I can see being problematic: calling .is_ok() doesn't necessarily mean the proof is valid, just that the input decoded valid. It can be Ok(Verification::Invalid(..)). We could either flatten everything to just return Verification or add some sort of #[must_use] + a helper .is_valid() + a note in the Changelog about this.

Let me know your thoughts.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Verification return value should be just the enum Verification Remove the #[allow(clippy::result_large_err)]

2 participants