search

bitcoindevkit / bdk-cli

A CLI wallet library and REPL tool to demo and test the BDK library
Other
108 stars 64 forks source link

adding the commands for proof-of-reserves using the separate repository #48

Closed ulrichard closed 2 years ago

ulrichard commented 2 years ago

adding the commands for proof-of-reserves using the separate repository

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

notmandatory commented 2 years ago

Rather than a merge commit (4fca4b8e5f1dae4c6781b4626dfc1f9278213085) it'd be better to rebased this branch on the latest master branch. Let me know if you need any help with the rebase.

notmandatory commented 2 years ago

Will the reserves feature only work right now with the electrum blockchain client? Or can it work with any blockchain that supports blockchain::Capability::GetAnyTx or some other capability?

ulrichard commented 2 years ago

Will the reserves feature only work right now with the electrum blockchain client? Or can it work with any blockchain that supports blockchain::Capability::GetAnyTx or some other capability?

So far I tested it only with electrum. I assume it could be made to work with others as well, but I didn't look into that so far.

ulrichard commented 2 years ago

I just updated the implementation to better separate between internal and external proof validation. The internal validation with a wallet (that can be built with a public key descriptor) can be used with any blockchain connector. It is only the external validation with addresses that is only available with electrum. The latter one is the more important one for me, and I'm ok with it only being available with electrum for now. I tested and validated the behavior with the following script. It also serves as a good documentation proof_of_reserves_roundtrip_blockfilters.sh.txt Is there a standard way of integrating something like this in the documentation?

ulrichard commented 2 years ago

Yes, the plan for bdk-reserves was from the beginning to add it to the BDK project as a util library.

ulrichard commented 2 years ago

reACK 4a91e63

Just few more small nits and I think its good to go.

Also I think the doc comments here

https://github.com/bitcoindevkit/bdk-cli/blob/f8f65f2bc56e46c34d17a0b6f8212e7f84933b5d/src/lib.rs#L192-L242 and here

https://github.com/bitcoindevkit/bdk-cli/blob/f8f65f2bc56e46c34d17a0b6f8212e7f84933b5d/src/lib.rs#L362-L403 needs to be updated to include the new flags and subcommands..

I don't see how the new sub commands would fit in these examples. They only list the wallet options. But reserves didn't add anything there. It only adds sub-commands.

notmandatory commented 2 years ago

I just updated the implementation to better separate between internal and external proof validation. The internal validation with a wallet (that can be built with a public key descriptor) can be used with any blockchain connector. It is only the external validation with addresses that is only available with electrum. The latter one is the more important one for me, and I'm ok with it only being available with electrum for now. I tested and validated the behavior with the following script. It also serves as a good documentation proof_of_reserves_roundtrip_blockfilters.sh.txt Is there a standard way of integrating something like this in the documentation?

I think ultimately we can document this on the website docs in a chapter on bdk-cli and sub-chapter on the reserves feature. For now if you'd like you can do a web site blog post to describe the new functionality and how to use it which we can convert to the more formal docs later.

notmandatory commented 2 years ago

You also will need to rebase on master to pickup CI testing against our new stable rust version (1.56.1).

ulrichard commented 2 years ago

Tested and looks good. I have a few small requests to make the cli more consistent.

Also I noticed that my proof of reserves validated even though one of the UTXOs wasn't confirmed on the (testnet) blockchain yet. Is this a known issue?

No, I was not aware of this. And this could be easily exploited, but also easily detected. I will add some checks and comments, also in the bdk-reserves lib.

ulrichard commented 2 years ago

After adding the checks for block_height, many manual tests of the internal verify_proof() failed. proof_of_reserves_roundtrip.sh.txt But the tests in https://github.com/weareseba/bdk-reserves/blob/main/tests/mempool.rs always succeed on my machine. Is is possible that the transaction information is not fully stored in the database?

notmandatory commented 2 years ago

ACK 7e6a4c8 I ran through some manual testing using electrum and this looks ready to merge.

I would like to eventually change the integration tests to use regtest instead of testnet so the tests will be a little quicker and not break if the utxos in the test descriptor changes, but I think it's fine for now.

ulrichard commented 2 years ago

Thanks for reviewing, testing and all the proposals for improvements @notmandatory and @rajarshimaitra I can't merge it myself, as I am not authorized.

rajarshimaitra commented 2 years ago

@ulrichard Thanks for working on this. I will test the latest commits and merge it soon.

@notmandatory IIUC for regtest support we need this to work with RPC backend right?

notmandatory commented 2 years ago

@notmandatory IIUC for regtest support we need this to work with RPC backend right?

We can use the bitcoind and electrsd crates as we do in bdk to do automated regtest testing. But we haven't had any tests that needed them in this repo until the new test_proof_of_reserves_* tests were added.