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

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

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

ulrichard
Copy link
Contributor

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

Notes to the reviewers

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
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@notmandatory
Copy link
Member

Rather than a merge commit (4fca4b8) 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
Copy link
Member

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
Copy link
Contributor Author

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 ulrichard force-pushed the reserves-separate branch 2 times, most recently from d7063cc to 2ee0d40 Compare November 10, 2021 09:48
@ulrichard
Copy link
Contributor Author

ulrichard commented Nov 10, 2021

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 ulrichard force-pushed the reserves-separate branch 2 times, most recently from 9e26917 to 3d192ec Compare November 10, 2021 10:26
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

untested Concept ACK. 3d192ec

Thanks for working on this, it can be a really useful feature. And I like the current external trait impl on bdk::wallet, that makes the reserves crate really portable without putting it into bdk crate directly. But this could be eventually added to the BDK project as a util library.

I will try to test the whole thing in more detail. I am wondering what is stopping us from using other blockchains like core RPC. From your test script it seems its working fine with compact filters. Will look and report back.

Below are few minor nits in mean time.

CHANGELOG.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ulrichard
Copy link
Contributor Author

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

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial tACK 3cc81db. I haven't run the test scripts, syncing my testnet node now. But there should not be an issue. Will play some more with it and report back if I find something odd.

Below are few more comments.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ulrichard ulrichard force-pushed the reserves-separate branch 2 times, most recently from d8a6064 to 4a91e63 Compare November 23, 2021 07:22
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reACK 4a91e63

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

Also I think the doc comments here

bdk-cli/src/lib.rs

Lines 192 to 242 in f8f65f2

/// let expected_cli_opts = CliOpts {
/// network: Network::Testnet,
/// subcommand: CliSubCommand::Wallet {
/// wallet_opts: WalletOpts {
/// wallet: "main".to_string(),
/// verbose: false,
/// descriptor: "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/44'/1'/0'/0/*)".to_string(),
/// change_descriptor: None,
/// #[cfg(feature = "electrum")]
/// electrum_opts: ElectrumOpts {
/// timeout: None,
/// server: "ssl://electrum.blockstream.info:60002".to_string(),
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-ureq")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// read_timeout: 5,
/// write_timeout: 5,
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-reqwest")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// conc: 4,
/// stop_gap: 10
/// },
/// #[cfg(feature = "rpc")]
/// rpc_opts: RpcOpts{
/// address: "127.0.0.1:18443".to_string(),
/// auth: ("user".to_string(), "password".to_string()),
/// skip_blocks: None,
/// },
/// #[cfg(feature = "compact_filters")]
/// compactfilter_opts: CompactFilterOpts{
/// address: vec!["127.0.0.1:18444".to_string()],
/// conn_count: 4,
/// skip_blocks: 0,
/// },
/// #[cfg(any(feature="compact_filters", feature="electrum", feature="esplora"))]
/// proxy_opts: ProxyOpts{
/// proxy: None,
/// proxy_auth: None,
/// retries: 5,
/// },
/// },
/// subcommand: WalletSubCommand::OnlineWalletSubCommand(Sync {
/// max_addresses: Some(50)
/// }),
/// },
/// };
and here

bdk-cli/src/lib.rs

Lines 362 to 403 in f8f65f2

/// let expected_wallet_opts = WalletOpts {
/// wallet: "main".to_string(),
/// verbose: false,
/// descriptor: "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/44'/1'/0'/0/*)".to_string(),
/// change_descriptor: None,
/// #[cfg(feature = "electrum")]
/// electrum_opts: ElectrumOpts {
/// timeout: None,
/// server: "ssl://electrum.blockstream.info:60002".to_string(),
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-ureq")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// read_timeout: 5,
/// write_timeout: 5,
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-reqwest")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// conc: 4,
/// stop_gap: 10
/// },
/// #[cfg(feature = "compact_filters")]
/// compactfilter_opts: CompactFilterOpts{
/// address: vec!["127.0.0.1:18444".to_string()],
/// conn_count: 4,
/// skip_blocks: 0,
/// },
/// #[cfg(feature = "rpc")]
/// rpc_opts: RpcOpts{
/// address: "127.0.0.1:18443".to_string(),
/// auth: ("user".to_string(), "password".to_string()),
/// skip_blocks: None,
/// },
/// #[cfg(any(feature="compact_filters", feature="electrum", feature="esplora"))]
/// proxy_opts: ProxyOpts{
/// proxy: None,
/// proxy_auth: None,
/// retries: 5,
/// },
needs to be updated to include the new flags and subcommands..

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@ulrichard
Copy link
Contributor Author

reACK 4a91e63

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

Also I think the doc comments here

bdk-cli/src/lib.rs

Lines 192 to 242 in f8f65f2

/// let expected_cli_opts = CliOpts {
/// network: Network::Testnet,
/// subcommand: CliSubCommand::Wallet {
/// wallet_opts: WalletOpts {
/// wallet: "main".to_string(),
/// verbose: false,
/// descriptor: "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/44'/1'/0'/0/*)".to_string(),
/// change_descriptor: None,
/// #[cfg(feature = "electrum")]
/// electrum_opts: ElectrumOpts {
/// timeout: None,
/// server: "ssl://electrum.blockstream.info:60002".to_string(),
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-ureq")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// read_timeout: 5,
/// write_timeout: 5,
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-reqwest")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// conc: 4,
/// stop_gap: 10
/// },
/// #[cfg(feature = "rpc")]
/// rpc_opts: RpcOpts{
/// address: "127.0.0.1:18443".to_string(),
/// auth: ("user".to_string(), "password".to_string()),
/// skip_blocks: None,
/// },
/// #[cfg(feature = "compact_filters")]
/// compactfilter_opts: CompactFilterOpts{
/// address: vec!["127.0.0.1:18444".to_string()],
/// conn_count: 4,
/// skip_blocks: 0,
/// },
/// #[cfg(any(feature="compact_filters", feature="electrum", feature="esplora"))]
/// proxy_opts: ProxyOpts{
/// proxy: None,
/// proxy_auth: None,
/// retries: 5,
/// },
/// },
/// subcommand: WalletSubCommand::OnlineWalletSubCommand(Sync {
/// max_addresses: Some(50)
/// }),
/// },
/// };

and here

bdk-cli/src/lib.rs

Lines 362 to 403 in f8f65f2

/// let expected_wallet_opts = WalletOpts {
/// wallet: "main".to_string(),
/// verbose: false,
/// descriptor: "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/44'/1'/0'/0/*)".to_string(),
/// change_descriptor: None,
/// #[cfg(feature = "electrum")]
/// electrum_opts: ElectrumOpts {
/// timeout: None,
/// server: "ssl://electrum.blockstream.info:60002".to_string(),
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-ureq")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// read_timeout: 5,
/// write_timeout: 5,
/// stop_gap: 10
/// },
/// #[cfg(feature = "esplora-reqwest")]
/// esplora_opts: EsploraOpts {
/// server: "https://blockstream.info/testnet/api/".to_string(),
/// conc: 4,
/// stop_gap: 10
/// },
/// #[cfg(feature = "compact_filters")]
/// compactfilter_opts: CompactFilterOpts{
/// address: vec!["127.0.0.1:18444".to_string()],
/// conn_count: 4,
/// skip_blocks: 0,
/// },
/// #[cfg(feature = "rpc")]
/// rpc_opts: RpcOpts{
/// address: "127.0.0.1:18443".to_string(),
/// auth: ("user".to_string(), "password".to_string()),
/// skip_blocks: None,
/// },
/// #[cfg(any(feature="compact_filters", feature="electrum", feature="esplora"))]
/// proxy_opts: ProxyOpts{
/// proxy: None,
/// proxy_auth: None,
/// retries: 5,
/// },

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.

@ulrichard ulrichard force-pushed the reserves-separate branch 3 times, most recently from 68722e2 to 4556d8e Compare November 24, 2021 14:29
Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 4556d8e

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.

Ah right, my bad.. This has nothing to do with wallet opts..

@notmandatory
Copy link
Member

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.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

src/lib.rs Outdated
ProduceProof {
/// Sets the message
#[structopt(name = "MESSAGE", long = "message")]
msg: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From code looks like this is not an optional parameter so should be:

Suggested change
msg: Option<String>,
msg: String,

src/lib.rs Outdated
VerifyProof {
/// Sets the PSBT to verify
#[structopt(name = "BASE64_PSBT", long = "psbt")]
psbt: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above not optional:

Suggested change
psbt: Option<String>,
psbt: String,

src/lib.rs Outdated
psbt: Option<String>,
/// Sets the message to verify
#[structopt(name = "MESSAGE", long = "message")]
msg: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above not optional:

Suggested change
msg: Option<String>,
msg: String,

src/lib.rs Outdated
@@ -1076,6 +1134,47 @@ where
let txid = maybe_await!(wallet.broadcast(tx))?;
Ok(json!({ "txid": txid }))
}
#[cfg(feature = "reserves")]
ProduceProof { msg } => {
let message = if let Some(msg) = msg {
Copy link
Member

@notmandatory notmandatory Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If msg is not optional and is enforced by StructOpt then this logic isn't needed.

src/lib.rs Outdated
}
#[cfg(feature = "reserves")]
VerifyProof { psbt, msg } => {
let psbt = if let Some(psbt) = psbt {
Copy link
Member

@notmandatory notmandatory Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If psbt is not optional and is enforced by StructOpt then this logic can be simplified.

src/lib.rs Outdated
} else {
panic!("Missing `psbt` option")
};
let message = if let Some(msg) = msg {
Copy link
Member

@notmandatory notmandatory Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If msg is not optional and is enforced by StructOpt then this logic isn't needed.

@notmandatory
Copy link
Member

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

@ulrichard
Copy link
Contributor Author

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 ulrichard force-pushed the reserves-separate branch 3 times, most recently from 4d67e95 to fb9ae70 Compare December 1, 2021 14:44
src/lib.rs Outdated
let _max_confirmation_height = current_height - confirmations.unwrap_or(6);

// ToDO: use this line after upgrading to 0.14
//let spendable = maybe_await!(wallet.verify_proof(&psbt, &msg, max_confirmation_height))?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after #60 is merged to master.

@ulrichard ulrichard force-pushed the reserves-separate branch 2 times, most recently from 109a44d to 09a9435 Compare December 1, 2021 15:41
src/lib.rs Outdated
#[structopt(name = "MESSAGE", long = "message")]
msg: String,
/// Sets the number of block confirmations for UTXOs to be considered. If nothing is specified, 6 is used.
#[structopt(name = "CONFIRMATIONS", long = "confirmations")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better way to specify the default for the cli is like this, it will also show in the help so you don't need to mention it in the above docs line:

Suggested change
#[structopt(name = "CONFIRMATIONS", long = "confirmations")]
#[structopt(name = "CONFIRMATIONS", long = "confirmations", default_value = "6")]

src/lib.rs Outdated
msg: String,
/// Sets the number of block confirmations for UTXOs to be considered. If nothing is specified, 6 is used.
#[structopt(name = "CONFIRMATIONS", long = "confirmations")]
confirmations: Option<u32>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you specify the default_value via struct opt then you don't need an Option here:

Suggested change
confirmations: Option<u32>,
confirmations: u32,

src/lib.rs Outdated
let psbt = base64::decode(&psbt).unwrap();
let psbt: PartiallySignedTransaction = deserialize(&psbt).unwrap();
let current_height = wallet.client().get_height()?;
let _max_confirmation_height = current_height - confirmations.unwrap_or(6);
Copy link
Member

@notmandatory notmandatory Dec 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If default is specified via struct opt then this can be simplified a little and by moving to struct opt will always be in sync with cli docs:

Suggested change
let _max_confirmation_height = current_height - confirmations.unwrap_or(6);
let _max_confirmation_height = current_height - confirmations;

@ulrichard ulrichard force-pushed the reserves-separate branch 4 times, most recently from d6a9564 to 75dde25 Compare December 2, 2021 07:42
@ulrichard
Copy link
Contributor Author

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?

src/lib.rs Outdated Show resolved Hide resolved
@ulrichard ulrichard force-pushed the reserves-separate branch 3 times, most recently from d8ae6e6 to 1567bc1 Compare December 2, 2021 16:14
@notmandatory
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Member

@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.

Copy link
Contributor

@rajarshimaitra rajarshimaitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ReACK 7e6a4c8

@notmandatory yes that makes sense. Previously we didn't need such integrations tests here, but now that we need one, maybe it would make sense to also add some generic wallet tests with regtest rpc too along with PoR tests? We should be able to do all the stuffs in PoR except the ExternalReserves thing with RPC. I will open a new issue to keep track of it.

@ulrichard I am merging this now. Below is one future improvement question.

dependencies = [
"base64 0.11.0",
"bdk",
"bitcoinconsensus",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bdk has a verify feature that uses bitcoinconsensus. Would there a be a way in future to use bdk::verify instead of bitcoinconsensus in the bdk-reserves dependency?

@rajarshimaitra rajarshimaitra merged commit 58386a0 into bitcoindevkit:master Dec 8, 2021
ulrichard added a commit to bitcoindevkit/bdk-reserves that referenced this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants