Implementing proof of reserves #356

ulrichard · 2021-05-28T14:22:03Z

Description

Adding a module that allows the construction of "proof of reserves" PSBTs and the verification thereof.

Notes to the reviewers

There will be an accompanying PR in bdk-cli. I will paste the URL here, once it's created.
https://github.com/ulrichard/bdk-cli/tree/reserves
But it builds upon this one:
bitcoindevkit/bdk-cli#28

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

Cargo.toml

notmandatory · 2021-05-30T02:54:56Z

@nicolasburtey heard you on SLP and that you are looking into proof of reserve for your project. I would be interested to have your feedback on if you think this PR is in the right direction.

src/wallet/reserves.rs

src/error.rs

afilini · 2021-06-01T08:58:48Z

Cargo.toml

@@ -67,6 +71,7 @@ env_logger = "0.7"
 base64 = "^0.11"
 clap = "2.33"
 serial_test = "0.4"
+rstest = "^0.7"


I'm not a big fan of adding extra dependencies, even if they are only used during tests since they increase compilation times. Is there a different way to achieve the same result without adding this extra dependency?

I'm sure it would be possible with some macro magic. But I don't know rust macros good enough yet.

Ok, we can leave this open for the meantime and once everything else is done I'll help you out with the macro

Cargo.toml

ulrichard · 2021-06-02T09:11:33Z

src/wallet/reserves.rs

+    network: Network,
+) -> Result<u64, Error> {
+    let tx = psbt.clone().extract_tx();
+


I'm not sure if it would make sense to copy the outpoints into a sorted container for faster lookups here.

nicolasburtey · 2021-06-02T11:03:57Z

@notmandatory super cool I'll have a look. thanks.

notmandatory · 2021-06-02T17:37:58Z

@ulrichard I have a meta question also about this PR. It seems that you don't need to access any wallet internals to create or validate a proof of reserve. So how would you feel about making this it's own repo here in the bitcoindevkit project? I think this would make it easier to iterate on the PoR concepts and release on your own schedule. And you can still have pub extern crate bdk so anyone using your crate will have full access to bdk wallet features.

ulrichard · 2021-06-04T06:00:44Z

@ulrichard I have a meta question also about this PR. It seems that you don't need to access any wallet internals to create or validate a proof of reserve. So how would you feel about making this it's own repo here in the bitcoindevkit project? I think this would make it easier to iterate on the PoR concepts and release on your own schedule. And you can still have pub extern crate bdk so anyone using your crate will have full access to bdk wallet features.

I'm still fairly new to rust, and thus still learning how to structure projects. If you think that's better, it's fine with me.

Do you have naming conventions for stuff like that? How about "bdk-reserves"?
Can I just create the project, and you add it to the bitcoindevkit project, or is it better if you create it first?

notmandatory · 2021-06-06T02:02:39Z

I'm still fairly new to rust, and thus still learning how to structure projects. If you think that's better, it's fine with me.

Thanks, we're still figuring it out too, but I think in this case since proof of reserve is still an experimental topic it makes sense for it to live in it's own repo.

1. Do you have naming conventions for stuff like that? How about "bdk-reserves"?

We don't have an official convention, the most important thing is that the name isn't taken in https://crates.io yet. The name bdk-reserves is a available.

2. Can I just create the project, and you add it to the `bitcoindevkit` project, or is it better if you create it first?

It's probably easier if you create the repo in your account and then when you have it ready to re-review you can transfer it to the bitcoindevkit project. I don't think your code will need to change much, you should still be able to implementing the ProofOfReserves trait on Wallet. And I think this could be a good dev model for how other experimental features (mixing, DLCs, stuff like that), could be built on bdk.

rajarshimaitra · 2021-06-07T07:23:39Z

Hello @ulrichard , I haven't looked into the PR in detail yet, but judging from the overall objective is it something similar to BIP322? That BIP is a generic message signing protocol using specially formed Bitcoin transactions. And one of the major applications of such generic signing is "Proof of Reserve", as outlined here.

My understanding is BIP322 can be used for many things including proof of reserve, so maybe you might wanna try to include that standard in your implementation?

Unfortunately, there is no working implementation of BIP322 and rust-miniscript had a feature request open for it for some time now. As far as my understanding, nobody in Blockstream has time to write it, but they would appreciate PRs.

I have worked on a very rough sketch of it a few months ago trying to layout how basic BIP322 rust structures might look like. rust-bitcoin/rust-miniscript#240

It's not complete but it does basic BIP322 functions of signing and verifying. That might be something of your interest?

I am also happy to put in hands with you finishing BIP322 (it has been stale for some time now) and BDK can simply use that (it already has rust-miniscript dependency), if that aligns with your need.

Overall I feel this magic should reside in some upstream rust library like rust-miniscript or rust-bitcoin instead of wallet tools. Wallet tools like BDK can then simply expose simple APIs to use it.

ulrichard · 2021-06-07T09:20:39Z

Hello @ulrichard , I haven't looked into the PR in detail yet, but judging from the overall objective is it something similar to BIP322? That BIP is a generic message signing protocol using specially formed Bitcoin transactions. And one of the major applications of such generic signing is "Proof of Reserve", as outlined here.

My understanding is BIP322 can be used for many things including proof of reserve, so maybe you might wanna try to include that standard in your implementation?

Unfortunately, there is no working implementation of BIP322 and rust-miniscript had a feature request open for it for some time now. As far as my understanding, nobody in Blockstream has time to write it, but they would appreciate PRs.

I have worked on a very rough sketch of it a few months ago trying to layout how basic BIP322 rust structures might look like. rust-bitcoin/rust-miniscript#240

It's not complete but it does basic BIP322 functions of signing and verifying. That might be something of your interest?

I am also happy to put in hands with you finishing BIP322 (it has been stale for some time now) and BDK can simply use that (it already has rust-miniscript dependency), if that aligns with your need.

Overall I feel this magic should reside in some upstream rust library like rust-miniscript or rust-bitcoin instead of wallet tools. Wallet tools like BDK can then simply expose simple APIs to use it.

Hi @rajarshimaitra, Yes BIP322 along with BIP127 were the main inspirations. Of which I followed BIP127 more closely.
I didn't realize BIP322 could be implemented in miniscript. I'll have a look at your PR.

Made the verify_sighash_type_all() function work with the test data. But it does not all make sense to me.

notmandatory · 2021-09-30T23:51:18Z

Closing the PR and will work with @ulrichard on his https://github.com/ulrichard/bdk-reserves repo instead.

notmandatory reviewed May 28, 2021

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

ulrichard force-pushed the feature/proof_of_reserves branch 4 times, most recently from 551c70e to dff145e Compare May 31, 2021 09:19

ulrichard mentioned this pull request May 31, 2021

Can't use in conjunction with bdk rust-bitcoin/rust-bitcoinconsensus#26

Closed

ulrichard force-pushed the feature/proof_of_reserves branch from dff145e to 6568691 Compare May 31, 2021 09:41

ulrichard mentioned this pull request May 31, 2021

Doesn't compile any more ElementsProject/reserves#6

Open

afilini requested changes Jun 1, 2021

View reviewed changes

ulrichard force-pushed the feature/proof_of_reserves branch 2 times, most recently from 62227ac to 53316ea Compare June 1, 2021 14:42

ulrichard commented Jun 2, 2021

View reviewed changes

ulrichard force-pushed the feature/proof_of_reserves branch 4 times, most recently from dca18fd to 061cfbd Compare June 11, 2021 10:02

nicolasburtey mentioned this pull request Jun 16, 2021

proof of reserve for bitcoin in channels lightningnetwork/lnd#5391

Open

ulrichard force-pushed the feature/proof_of_reserves branch from b5519c0 to 3e5bf16 Compare June 21, 2021 06:36

ulrichard force-pushed the feature/proof_of_reserves branch 2 times, most recently from 4680c7d to def6089 Compare June 29, 2021 08:08

ulrichard mentioned this pull request Jun 29, 2021

why 2 licenses? bitcoindevkit/bdk-reserves#1

Closed

ulrichard force-pushed the feature/proof_of_reserves branch from def6089 to 4120a0a Compare June 29, 2021 12:55

ulrichard force-pushed the feature/proof_of_reserves branch from 4120a0a to ea9746b Compare July 8, 2021 06:39

ulrichard and others added 5 commits July 23, 2021 11:09

Implementing proof of reserves

cdd661f

Adding a separate error enum for proof of reserves

7a2b1bc

also feeding the TxOut to verify_proof

9244975

First step for validating the SigHashType on the tx instead of psbt.

dc7079f

Made the verify_sighash_type_all() function work with the test data. But it does not all make sense to me.

adapted to the updated version of the bitcoin crate

0379cdf

ulrichard force-pushed the feature/proof_of_reserves branch from 7c50ff4 to 0379cdf Compare July 23, 2021 09:10

notmandatory closed this Sep 30, 2021

nud3l mentioned this pull request Oct 23, 2023

Question: Support for Discreet Log Contracts (DLCs) #1181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing proof of reserves #356

Implementing proof of reserves #356

ulrichard commented May 28, 2021 •

edited

Loading

notmandatory commented May 30, 2021 •

edited

Loading

afilini Jun 1, 2021

ulrichard Jun 1, 2021

afilini Jun 4, 2021

ulrichard Jun 2, 2021

nicolasburtey commented Jun 2, 2021

notmandatory commented Jun 2, 2021

ulrichard commented Jun 4, 2021

notmandatory commented Jun 6, 2021

rajarshimaitra commented Jun 7, 2021

ulrichard commented Jun 7, 2021

notmandatory commented Sep 30, 2021

Implementing proof of reserves #356

Implementing proof of reserves #356

Conversation

ulrichard commented May 28, 2021 • edited Loading

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

notmandatory commented May 30, 2021 • edited Loading

afilini Jun 1, 2021

Choose a reason for hiding this comment

ulrichard Jun 1, 2021

Choose a reason for hiding this comment

afilini Jun 4, 2021

Choose a reason for hiding this comment

ulrichard Jun 2, 2021

Choose a reason for hiding this comment

nicolasburtey commented Jun 2, 2021

notmandatory commented Jun 2, 2021

ulrichard commented Jun 4, 2021

notmandatory commented Jun 6, 2021

rajarshimaitra commented Jun 7, 2021

ulrichard commented Jun 7, 2021

notmandatory commented Sep 30, 2021

ulrichard commented May 28, 2021 •

edited

Loading

notmandatory commented May 30, 2021 •

edited

Loading