Refactor verification logic #502

rajarshimaitra · 2021-12-16T15:34:23Z

Description

As discussed in #498 and also in team call,

default verification from wallet sync is removed
verify_tx refactored as an wallet API
in sync verification added for electrum and esplora backends, gated by verify flag.
verify flag is removed from TransactionDetails.

Notes to the reviewers

I haven't looked into comapct_filters to see how verification can fit there, but that will probably be required in future.

Checklists

All Submissions:

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

Wallet API change:

I've updated CHANGELOG.md

rajarshimaitra · 2021-12-18T17:30:32Z

Rebased.

LLFourn

utACK 426430d

Thanks for this @rajarshimaitra.
Looks like sqlite will need some extra care to get rid of the "verified" flag.

rajarshimaitra · 2022-01-02T14:28:29Z

Thanks @LLFourn for the look. Sqlite was tricky. But finally all checks seems to be passing.. 🥳

Changes since last review

Added verification tests in CI (closing Add verify feature into blockchain tests #500 )
Fixed verification flags in sqlite
Fixed verification flags in compact filters.

src/blockchain/esplora/reqwest.rs

LLFourn · 2022-01-11T04:33:35Z

src/wallet/verify.rs

+    /// `electrum` and `esplora` support (confed/unconfed) *any* transaction verification,
+    /// `rpc` supports (confed/unconfed) *in-wallet* transaction verification
+    /// `compact_filters` supports (unconfed) *any* transaction verification.
+    pub fn verify_tx(&self, tx: &Transaction) -> Result<(), VerifyError> {


After thinking about this further I really think we should remove this API for now.

If a user wants to verify a transaction it's unlikely they want to verify a transaction that's in the database. If it's in the database they can just check if it's in the database. If it is then it's verified (assuming you compiled with verification on).

If it's not in the database then it's really a blockchain specific thing in which case it should be a trait that is implemented on blockchains.

I think I get what you are saying.

Even if its an inherent Blockchain thing, we don't want to force user with its impl in their custom backends. So one way to do that will be to create a default implementation for any Blockchain. This can then be feature gated and blanket implemented.

And the end result would look like wallet.blockchain.verify(), instead of current wallet.verify(). I am not sure if thats a big enough API win.

My envision of using the verify API is also mostly for non wallet transactions, because its a reasonable assumption that you trust your own DB. And they can enforce it at sync level with --verify feature flag for that purpose.

But there should be an easy way to verify non wallet txs. (Even random message verification at future). So the library needs the API somewhere.

So I think for this PR I will just let it be here. And we can make the move in future PR if we want.

@LLFourn Do you feel satisfied with this reasoning?

@rajarshimaitra I'm not sure.

I've thought about this a lot and I think that transaction verification is a very context specific thing and I'm not sure this function fits any of those contexts. Do you have one in mind?

In what situations do you need to verify non-wallet transactions where you don't care about whether the transaction is actually valid but just if the inputs were to exist it passes the consensus rules. Don't you also want to check if the inputs has have been spent or otherwise exist or if the locktime on the tx has passed? Also note this function fails with backends without a transaction index. How are you meant to deal with this failure in the context where it is useful (which again I can't conceive of).

I can understand wanting to check a tx is valid without broadcasting it. Bitcoin core rpc has a call for this. With esplora it would be a matter of checking whether the inputs have been spent and using libconsensus. So I see this as a blockchain specific check which this function doesn't implement properly. It may fail depending on the backend which is odd because the backend it fails on (rpc) is the only one that has a call to do this thing properly (testmempoolaccept).

I don't think it's worth holding up this PR on this but if we are unsure about this function then it makes sense to me to leave it where it is. Why break the API only to break it again later on?

Hmm, I guess I see what you mean. My initial use case was, if I build a transaction with a custom script P2SH and I create my custom witness to spend it, a easy way to check if the the witness works would be useful. Something like testmempoolaccept of core RPC.

I don't have any strong opinion on where this should be placed, and no solution for the case of non-txindex blockchains.

Currently its not breaking the wallet API. Using the lib with --verify flag will just open this extra function call on wallet, no other existing calls changes.

What do you suggest is the way forward with this PR?

Hmm, I guess I see what you mean. My initial use case was, if I build a transaction with a custom script P2SH and I create my custom witness to spend it, a easy way to check if the the witness works would be useful. Something like testmempoolaccept of core RPC.

Wouldn't using libconsensus for the P2SH output be preferable in this case? testmempool would only be useful if you want to check the outputs actually exist at the same time and the network time allows for it etc. Another option is to use Interpreter from rust-miniscript.

Currently its not breaking the wallet API. Using the lib with --verify flag will just open this extra function call on wallet, no other existing calls changes.

It's breaking the API because it removes the existing function.

What do you suggest is the way forward with this PR?

I'd suggest the easiest way forward is to remove the changes related to the verify function from this PR and then make an issue or follow up PR where we can discuss whether this API is even a good idea or not. iirc @afilini introduced this function so his input is probably more useful than mine here.

rajarshimaitra · 2022-01-21T16:15:24Z

@LLFourn as suggested moved verification logic to script_sync.

@notmandatory updated with rebase.

The default sync verification is removed from wallet module. By default sync time verification only makes sense for `electrum` and `esplora` backend as they are usually untrusted 3rd party services. script verification for transaction is costly, so removing default script verification optimizes performance.

Instead of verifying txs at sync time in every backend, its moved to script_sync to by default be available to any backend.

rajarshimaitra · 2022-02-09T11:52:00Z

@LLFourn updated with suggested changes..

Some weird test failure is happening in ureq compilation.. Not sure why..

notmandatory · 2022-02-10T06:13:13Z

@rajarshimaitra I think the CI was failing due to some problem with the recent rust nightly used by those jobs.. I pinned the failing jobs to nightly-2022-01-25 and now they are passing.

The reason the Blockchain test-electrum and two Blockchain test-esplora tests are waiting is because the name changed with the new verify feature added, I can fix this in the github checks config prior to merging.

rajarshimaitra · 2022-02-10T17:28:26Z

Thanks @notmandatory for looking into it.. Yup they seem like something to do with recent rust.

LLFourn

utACK 98a3b32.

LGTM thanks @rajarshimaitra

notmandatory

Overall looks good but I have a few changelog related comments. Some may be fixed with a rebase.

CHANGELOG.md

rajarshimaitra · 2022-02-11T16:40:28Z

Thanks @notmandatory , I missed the changelog modification in last push.. Updated as suggested.

afilini

I like the concept and I think the code looks good. My only concern are the database changes that were made, I don't think those are backwards-compatible, especially in sqlite.

For sqlite I would just add another migration that removes the verified column.

For sled I don't think we have to do anything, because serde should ignore the extra fields when deserializing, but if you could check it (even manually, no need to add a test for this) it would be great.

LLFourn · 2022-02-21T22:39:01Z

utACK 02f2302 thanks for fixing that up @notmandatory

notmandatory · 2022-02-21T22:44:37Z

In 02f2302 I changed the MIGRATIONS to do a proper migration and remove the transaction_details.verified column rather than altering the original CREATE TABLE transaction_details ... statement. Since sqlite doesn't support ALTER TABLE DROP column I had to recreate the transaction_details table which seems to be the recommended way to do it.

I also used my PR version of bdk-cli to manually test sync and list_transaction before and after this PR change. With our without it works for key-value-db or sqlite. I still fixed the MIGRATIONS so it can be an example for the future changes.

notmandatory · 2022-02-21T22:55:06Z

~~I think all we need now is a rebase to pickup the pinned tokio version and CHANGELOG (:face_exhaling:) changes in master.~~

I removed my cherry-picked commits and added a merge commit from master at the end instead.

86d36e5 Pin tokio version to ~1.14 (Steve Myers) 02f2302 Remove `verify` flag from `TransactionDetails` via new MIGRATIONS (Steve Myers) 662500f Update CHANGELOG (rajarshimaitra) e6be550 [ci] Change test-readme-examples and Codecov jobs to use `nightly-2022-01-25` rust (Steve Myers) 1d7ea89 Refactor sync time verification (rajarshimaitra) b05ee78 Remove verifcation flag from compact_filters (rajarshimaitra) 53c30b0 Add verification tests in CI (rajarshimaitra) 6a09075 Remove verify flag from sqlite DB (rajarshimaitra) 61a95d0 Update changelog (rajarshimaitra) 08f312a Remove `verify` flag from `TransactionDetails` (rajarshimaitra) acbf0ae Add sync verification for `esplora` (rajarshimaitra) 4761155 Add sync verification in `electrum` (rajarshimaitra) 98a3b32 Remove sync verification (rajarshimaitra) Pull request description:  ### Description As discussed in bitcoindevkit#498 and also in team call, - default verification from wallet sync is removed - `verify_tx` refactored as an wallet API - in `sync` verification added for electrum and esplora backends, gated by `verify` flag. - `verify` flag is removed from `TransactionDetails`. ### Notes to the reviewers I haven't looked into `comapct_filters` to see how verification can fit there, but that will probably be required in future. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Wallet API change: * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: 82ac52f77e0fc1a011fc59d1ac70ad24b729e0287adf75fcb78af0f2b38eaeb187b2cabe959b63147ffb16d825711d58b87ad3292cbaa340dc6df2bfdcab5ab3

@notmandatory

0cc4700 Fix typo in CHANGELOG and doc in wallet/mod.rs (Steve Myers) 660faab Fix typo in CHANGELOG (LLFourn) 45767fc Remove max_addresses sync param (LLFourn) fbb50ad apply doc suggestions from @notmandatory (Lloyd Fournier) 035307e [rpc] Filter out unrelated transactions (LLFourn) c0e75fc Split get_tx into its own trait (LLFourn) dcd90f8 Restore but depreciate new_offline (LLFourn) 410a513 Add SyncOptions as the second argument to Wallet::sync (LLFourn) 326bfe8 Remove Blockchain from wallet (LLFourn) Pull request description: While trying to finish bitcoindevkit#490 I thought that it'd be better to try the idea of getting rid of a lot of the async macros and just having tow different traits for sync and async stuff. While trying to do that I felt that this needed to be done first. The goal of this change is to decouple the wallet from the blockchain trait. A wallet is something that keeps track of UTXOs and transactions (and can sign things). The only reason it should need to talk to the blockchain is if doing a `sync`. So we remove all superfluous calls to the blockchain and precisely define the requirements for the blockchain to be used to sync with two new traits: `WalletSync` and `GetHeight`. 1. Stop requesting height when wallet is created 2. `new_offline` is just `new` now. 3. a `WalletSync + GetHeight` is now the first argument to `sync`. 4. `SyncOptions` replaces the existing options to `sync` and allows for less friction when making breaking changes in the fuutre (no more noop_progress!). 5. We `Box` the `Progress` now to avoid type parameters 6. broadcast has been removed from `Wallet`. You just use the blockchain directly to broadcast now. ### Notes to the reviewers This needs bitcoindevkit#502 before it can be merged but can reviewed now. Be on the look up for stale documentation from this change. Our doc build looks broken because of ureq or something. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: a8f3e21e45359be642b1f30d4ac1ba74785439e1b770dbeab0a5a4b8aab1eef4bb6b855aea4382e289257bc890fa713ca832a8b6c9655f7a59e96d412b4da3e6

…irectories 00454be Update CHANGELOG with new db features and wallet data directories (Steve Myers) 14106eb Prevent building with more than one database feature enabled (Steve Myers) fac465e Fix clippy warning (Steve Myers) d7471f6 Add key-value-db and sqlite-db features, separate wallet directories (Steve Myers) Pull request description: ### Description - Add distinct `key-value-db` and `sqlite-db` features, keep default as `key-value-db` - Put cached wallet data in separate directories: `~/.bdk-bitcoin/<wallet_name>` - Put compact filter data in `<wallet_name>/compact_filters` - Depending on the db used put cached wallet data in: `<wallet_name>/wallet.sled/` or `<wallet_name>/wallet.sqlite` ### Notes to the reviewers This change will help test BDK with different databases, in particular for manually testing DB migrations such as in bitcoindevkit/bdk#502. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [ ] I've added tests for the new feature * [x] I've added docs for the new feature * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: a2610448295b39674706ab48f36e4ccb92f7065489bca2b7e0be81a6bbc063844ce7ea3728bd1fffde97938a8ef84234ba5a6cee56aa0deca267bbd671ae692a

1999d97aeb3d97ee24a9b59a2f1453a26943b595 Remove `verify` flag from `TransactionDetails` via new MIGRATIONS (Steve Myers) 0195bc0636d9a58013f0cbc3781d213b0cfc1509 Update CHANGELOG (rajarshimaitra) 4652bb8 Refactor sync time verification (rajarshimaitra) b05ee78c7335f7e3a1593dc4ff7686e6f72ff5c0 Remove verifcation flag from compact_filters (rajarshimaitra) 53c30b0479c74dde17cd27f8eac7f540e492067d Add verification tests in CI (rajarshimaitra) 6a09075d1a87509e9117eab727132efdf8ea6e1d Remove verify flag from sqlite DB (rajarshimaitra) 61a95d0d15b9944e8b5d13db64ef6ffd9a8d6e29 Update changelog (rajarshimaitra) 08f312a82f889f6bf5dfedfaa565ac2cb59683ae Remove `verify` flag from `TransactionDetails` (rajarshimaitra) 6fa29fa Add sync verification for `esplora` (rajarshimaitra) 4761155707a819c4db437e71a36621a027d5302a Add sync verification in `electrum` (rajarshimaitra) 98a3b3282a0ff59bbdf900adc765f6912d1975c1 Remove sync verification (rajarshimaitra) Pull request description:  ### Description As discussed in bitcoindevkit/bdk#498 and also in team call, - default verification from wallet sync is removed - `verify_tx` refactored as an wallet API - in `sync` verification added for electrum and esplora backends, gated by `verify` flag. - `verify` flag is removed from `TransactionDetails`. ### Notes to the reviewers I haven't looked into `comapct_filters` to see how verification can fit there, but that will probably be required in future. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Wallet API change: * [x] I've updated `CHANGELOG.md` Top commit has no ACKs. Tree-SHA512: 72e307008a137468d96d5c2a6ec804b18fa52363606f3c978208ae5dc22973a7f0aa37488e9bb98dde88409a12d59cc5f00c675d2d408e57e661bf6210bee67b

rajarshimaitra force-pushed the verifcation-refactor branch 4 times, most recently from ff24805 to 426430d Compare December 18, 2021 17:30

LLFourn reviewed Dec 27, 2021

View reviewed changes

rajarshimaitra mentioned this pull request Jan 2, 2022

Add verify feature into blockchain tests #500

Closed

3 tasks

rajarshimaitra force-pushed the verifcation-refactor branch 3 times, most recently from 15560d3 to 8f47dfb Compare January 2, 2022 14:14

rajarshimaitra mentioned this pull request Jan 9, 2022

Non-Wallet Tx verification doesn't work for RPC backend #498

Closed

LLFourn requested changes Jan 11, 2022

View reviewed changes

rajarshimaitra force-pushed the verifcation-refactor branch from e5afa46 to c738dd7 Compare January 21, 2022 16:14

notmandatory added this to the Release 0.17.0 Feature Freeze milestone Jan 25, 2022

LLFourn mentioned this pull request Jan 27, 2022

Remove blockchain from wallet #535

Merged

4 tasks

notmandatory assigned rajarshimaitra Feb 1, 2022

rajarshimaitra added 8 commits February 9, 2022 12:29

Add sync verification in electrum

4761155

Add sync verification for esplora

acbf0ae

Remove verify flag from TransactionDetails

08f312a

Update changelog

61a95d0

Remove verify flag from sqlite DB

6a09075

Add verification tests in CI

53c30b0

Remove verifcation flag from compact_filters

b05ee78

rajarshimaitra force-pushed the verifcation-refactor branch from c738dd7 to a184f78 Compare February 9, 2022 11:22

Refactor sync time verification

1d7ea89

Instead of verifying txs at sync time in every backend, its moved to script_sync to by default be available to any backend.

rajarshimaitra force-pushed the verifcation-refactor branch from a184f78 to 1d7ea89 Compare February 9, 2022 11:30

LLFourn approved these changes Feb 10, 2022

View reviewed changes

notmandatory requested changes Feb 11, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

notmandatory mentioned this pull request Feb 11, 2022

refactor wallet address caching into its own public method #537

Merged

5 tasks

notmandatory force-pushed the verifcation-refactor branch from e5856a0 to 662500f Compare February 11, 2022 17:23

afilini reviewed Feb 14, 2022

View reviewed changes

notmandatory mentioned this pull request Feb 20, 2022

Add key-value-db and sqlite-db features, separate wallet directories bitcoindevkit/bdk-cli#71

Merged

6 tasks

notmandatory approved these changes Feb 22, 2022

View reviewed changes

rajarshimaitra and others added 3 commits February 21, 2022 17:31

Update CHANGELOG

0195bc0

Remove verify flag from TransactionDetails via new MIGRATIONS

1999d97

Merge branch 'master' into verifcation-refactor

ad65dd5

notmandatory force-pushed the verifcation-refactor branch from 8cc4d0b to ad65dd5 Compare February 22, 2022 03:15

notmandatory merged commit 19f0287 into bitcoindevkit:master Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor verification logic #502

Refactor verification logic #502

rajarshimaitra commented Dec 16, 2021 •

edited

Loading

rajarshimaitra commented Dec 18, 2021

LLFourn left a comment •

edited

Loading

rajarshimaitra commented Jan 2, 2022

LLFourn Jan 11, 2022

rajarshimaitra Jan 11, 2022 •

edited

Loading

rajarshimaitra Jan 21, 2022

LLFourn Jan 26, 2022 •

edited

Loading

rajarshimaitra Jan 27, 2022

LLFourn Jan 29, 2022

rajarshimaitra commented Jan 21, 2022

rajarshimaitra commented Feb 9, 2022

notmandatory commented Feb 10, 2022

rajarshimaitra commented Feb 10, 2022

LLFourn left a comment •

edited

Loading

notmandatory left a comment •

edited

Loading

rajarshimaitra commented Feb 11, 2022

afilini left a comment

LLFourn commented Feb 21, 2022

notmandatory commented Feb 21, 2022 •

edited

Loading

notmandatory commented Feb 21, 2022 •

edited

Loading

Refactor verification logic #502

Refactor verification logic #502

Conversation

rajarshimaitra commented Dec 16, 2021 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

Wallet API change:

rajarshimaitra commented Dec 18, 2021

LLFourn left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented Jan 2, 2022

LLFourn Jan 11, 2022

Choose a reason for hiding this comment

rajarshimaitra Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

rajarshimaitra Jan 21, 2022

Choose a reason for hiding this comment

LLFourn Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

rajarshimaitra Jan 27, 2022

Choose a reason for hiding this comment

LLFourn Jan 29, 2022

Choose a reason for hiding this comment

rajarshimaitra commented Jan 21, 2022

rajarshimaitra commented Feb 9, 2022

notmandatory commented Feb 10, 2022

rajarshimaitra commented Feb 10, 2022

LLFourn left a comment • edited Loading

Choose a reason for hiding this comment

notmandatory left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra commented Feb 11, 2022

afilini left a comment

Choose a reason for hiding this comment

LLFourn commented Feb 21, 2022

notmandatory commented Feb 21, 2022 • edited Loading

notmandatory commented Feb 21, 2022 • edited Loading

rajarshimaitra commented Dec 16, 2021 •

edited

Loading

LLFourn left a comment •

edited

Loading

rajarshimaitra Jan 11, 2022 •

edited

Loading

LLFourn Jan 26, 2022 •

edited

Loading

LLFourn left a comment •

edited

Loading

notmandatory left a comment •

edited

Loading

notmandatory commented Feb 21, 2022 •

edited

Loading

notmandatory commented Feb 21, 2022 •

edited

Loading