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.

Sign up for GitHub

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

Jump to bottom

[chain, wallet] Add verify_tx for TxGraph #1339

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[chain, wallet] Add verify_tx for TxGraph #1339

wants to merge 4 commits into from

Conversation

ValuedMammal
Copy link
Contributor

@ValuedMammal ValuedMammal commented Feb 12, 2024 •
edited

Reintroduce a way to verify a transaction.

This adds a new feature to bdk_chain that uses feature bitcoin/bitcoinconsensus, and exposes the same through bdk wallet as bdk_chain/bitcoinconsensus.

closes #1180

Notes to the reviewers

Changelog notice

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

@ValuedMammal
feat(tx_graph): Add method verify_tx for TxGraph
e9aa82b 
* Add feature `bitcoinconsensus` to Cargo.toml
@ValuedMammal
chain: add hex macro to common test utils
bb4b19e 
For getting bytes from a hex str.
@ValuedMammal
test(chain): Add test_verify_transaction in test_tx_graph
e81fd4d 
That checks we can verify a transaction when TxGraph has
knowledge of the coins being spent. Additionally, check these
error cases:

- verification fails due to missing outputs
- script verification fails
@ValuedMammal
feat(wallet): expose TxGraph::verify_tx via Wallet
bc589b6 
* Add feature `bitcoinconsensus` to Cargo.toml
@ValuedMammal ValuedMammal changed the title [chain] Add verify_tx for TxGraph [chain, wallet] Add verify_tx for TxGraph Feb 16, 2024
@evanlinjin
Copy link
Member

I can't decide whether we need this in bdk_chain or even in bdk_wallet. On one hand, the bitcoinconsensus API is not difficult so the caller can just import it directly and use it without much difficulty...

Would love some input on this.

@ValuedMammal
Copy link
Contributor Author

Right, I'm curious if anyone has specifically asked for this functionality in bdk.

@nondiremanuel nondiremanuel added the discussion There's still a discussion ongoing label Mar 12, 2024
@nondiremanuel nondiremanuel added this to the 1.1.0 milestone Mar 12, 2024
@notmandatory
Copy link
Member

How about leaving it as a draft until we are ready to scope out a 1.1 release. Maybe by then we'll have a use case, if not we can close it. There is a scenario described in #352 where this verify functionality could be used.

@notmandatory notmandatory added module-wallet api A breaking API change labels Mar 16, 2024
@notmandatory
Copy link
Member

Ok per @LLFourn comments in #1180 I think this should stay in the 1.0.0-alpha milestone.

@LLFourn
Copy link
Contributor

LLFourn commented Mar 17, 2024

@notmandatory I don't really have an opinion on this being in any milestone. My opinion is that the main issue is fixing RBF design. Being able to verify a transaction is also a nice to have but if no one needs it then it doesn't have to be in v1.0.0.

@nondiremanuel nondiremanuel removed this from the 1.1.0 milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ValuedMammal ValuedMammal

Labels
api A breaking API change discussion There's still a discussion ongoing module-wallet
Projects
BDK
Status: Discussion
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reintroduce a way verify a transaction
5 participants
@ValuedMammal @evanlinjin @notmandatory @LLFourn @nondiremanuel