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.

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

Reintroduce a way verify a transaction #1180

Open
notmandatory opened this issue Oct 23, 2023 · 7 comments · May be fixed by #1339
Open

Reintroduce a way verify a transaction #1180

notmandatory opened this issue Oct 23, 2023 · 7 comments · May be fixed by #1339
Assignees
Labels
api A breaking API change enhancement New feature or request module-wallet

Comments

@notmandatory
Copy link
Member

Describe the enhancement

In pre-1.0 there was a function to verify a transaction against consensus rules and there should also be a convenient way to do this in BDK 1.0.

See original function bdk::wallet::verify::verify_tx.

Use case

This function is useful for verifying a transaction is valid before broadcasting it.

Additional context

https://docs.rs/bdk/latest/src/bdk/wallet/verify.rs.html#34-73

@notmandatory notmandatory added the enhancement New feature or request label Oct 23, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Oct 23, 2023
@DanGould
Copy link

DanGould commented Oct 23, 2023

Automated payjoin receivers must check that the "original psbt" transaction to be augmented can be broadcast as fallback in case the sender doesn't broadcast the payjoin they propose so that they can still get paid in the failure case. This function is useful in that case, and I'd hope to see a reference to testmempoolaccept bitcoind rpc because that's where I looked for this functionality at first.

That RPC also tests against the UTXO set which is important for this use case.

@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@ValuedMammal
Copy link
Contributor

Should this be a new build feature in bdk, or should we just include the bitcoinconsensus feature in the dependency for rust-bitcoin?

rust bitcoin offers what looks like a fairly convenient API as a method on Transaction.

If we want something more granular, we would likely want to pull in rust-bitcoinconsensus as a new dep.

As a first step, I like the idea of making verify_tx a method on Wallet that takes a &Transaction as param.

    pub fn verify_tx(&self, tx: &Transaction) -> Result<(), VerifyTxError> {
        tx.verify(|op: &OutPoint| -> Option<TxOut> { self.tx_graph().get_txout(*op).cloned() })
            .map_err(VerifyTxError::BitcoinConsensus)
    }

@LLFourn
Copy link
Contributor

LLFourn commented Feb 8, 2024

This verify method belongs on TxGraph in bdk_chain feature gated on the bitcoinconsensus feature. Wallet can just call it internally.

@ValuedMammal ValuedMammal linked a pull request Feb 12, 2024 that will close this issue
5 tasks
@notmandatory
Copy link
Member Author

There's some more discussion for the original feature in #352.

@LLFourn
Copy link
Contributor

LLFourn commented Mar 15, 2024

@notmandatory this is very relevant to #1374. The problem we should solve here is the RBF feature being mis-designed. You shouldn't just RBF a transaction by looking at the inputs and outputs without understanding the semantics of it. How do we know which outputs and inputs have to be there and which can we change. We guess and provide awkward APIs to let the developer hint us (allow_shrinking). Applications should store the purpose of a transaction at the application layer i.e. "paying john x BTC" and recreate a transaction serving the same purpose from scratch with the constraint that it has to spend one of the existing transaction's inputs. There are much fewer ways to screw this approach up and it is more powerful (you can replace multiple txs at once).

@notmandatory
Copy link
Member Author

I see how this is valuable especially since we had it in the pre-1.0.0 wallet API, so I agree this should stay in the 1.0.0-alpha milestone.

@notmandatory notmandatory added the api A breaking API change label Mar 16, 2024
@notmandatory
Copy link
Member Author

Since this can be enabled with a new feature flag without an api change I propose we move this to a post 1.0 milestone.

@nondiremanuel nondiremanuel removed this from the 1.0.0-alpha milestone Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change enhancement New feature or request module-wallet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

5 participants