Add support for BIP47 (reusable payment codes) #574
Conversation
src/utils/bip47/mod.rs
Outdated
|
|
||
| impl PaymentCode { | ||
| pub fn decode(data: &[u8]) -> Result<PaymentCode, Error> { | ||
| if data.len() != 80 { |
There was a problem hiding this comment.
I should probably define a constant for this length, it's everywhere...
This allows using the `testuitils` macro in their tests as well
Add two new traits: - `StatelessBlockchain` is used to tag `Blockchain`s that don't have any wallet-specic state, i.e. they can be used as-is to sync multiple wallets. - `BlockchainFactory` is a trait for objects that can build multiple blockchains for different descriptors. It's implemented automatically for every `Arc<T>` where `T` is a `StatelessBlockchain`. This allows a piece of code that deals with multiple sub-wallets to just get a `&B: BlockchainFactory` to sync all of them. These new traits have been implemented for Electrum, Esplora and RPC (the first two being stateless and the latter having a dedicated `RpcBlockchainFactory` struct). It hasn't been implemented on the CBF blockchain, because I don't think it would work in its current form (it throws away old block filters, so it's hard to go back and rescan). This is the first step for bitcoindevkit#549, as BIP47 needs to sync many different descriptors internally. It's also very useful for bitcoindevkit#486.
| use crate::{Error as WalletError, KeychainKind, LocalUtxo, TransactionDetails}; | ||
|
|
||
| #[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)] | ||
| pub struct PaymentCode { |
There was a problem hiding this comment.
Any specific reasoning behind creating a separate implementation here instead of using the library in rust-bitcoin?
There was a problem hiding this comment.
It didn't really integrate well with descriptors in my opinion. Adding an extra dependency just to get the PaymentCode struct and very few other things felt like a bad idea.
Also, we don't support bitmessage and multisig/bloom filters notifications, so that saved me some time and I only implemented the minimal set of BIP47 that we need.
|
The BIP links to this summary comments that says its in general a poor design, and cause privacy losses too in many use cases.. https://github.com/bitcoin/bips/wiki/Comments:BIP-0047 Any thoughts on that? Also is this the same thing as paynyms? |
My opinion is that although we shouldn't encourage bad practices, if a standard is being used in the real world there's value in having it here in BDK. On top of that there's no other alternative payment code spec in use (as far as I know), so while this may be bad we are stuck with it for the time being. This is consistent with my reasoning in the BIP69 discussion: I know it's not great, but there are people using it and if we don't support that we make BDK less "universal". We could consider adding a deprecation warning for BIP47 though, I wouldn't be against that.
It uses the same underlying technology but they are not exactly the same thing: paynyms are basically human-friendly "usernames" tied to payment codes, stored in a centralized directory. Here I'm only implementing the payment code part, so you could then take that code and upload it to the paynym directory (or lookup a name in the directory and download their code to send them a payment). |
|
Thanks @afilini for the detailed response.. BIP47 does sound like a cool magic. And if we don't have anything else it makes sense to have support for it and with proper warnings and cautions.. The BIP doesn't include a reference implementation though.. Are you following some existing refs out there? There's a lot of details in it, so I will take some time to review the code written as of now.. Initial Concept ACK.. I think its valuable to have this.. |
|
No I didn't follow any specific code reference, but I used the included tests vectors quite a lot. The long test at the end of the file that I've been using to test stuff while I was working uses the two mnemonics shown in the test vectors and the payment codes/addresses were correct. |
|
Don't spend too much time reviewing this in detail because I still need to change a few things. I just wanted a general feedback about the architecture before I start refining the little details |
I agree BDK shouldn't encourage bad practices. Also, in what way is it a standard?
Why does there need to be a payment spec for on-chain bitcoin? Unless bitcoin remains niche with just a few hundred thousand global users, payments must be done on some type of layer 2 system. What customer demand does BDK have for BIP47? |
Luke Dashjr's main concern is about notification transaction. It is optional and still not spam because paying fees like other transactions. I am not sure if there is anything else in design that he disagrees with. Clients do not need to send payment codes to third party servers as mentioned by Greg Maxwell. There have been some improvements in BIP 47,version 3 and version 4 are shared here: https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.mediawiki Concept ACK on adding support for BIP 47 payment codes in a library which is used by lot of bitcoin projects. This will help in improving privacy. |
I would like to send and receive bitcoin via BIP47 payment codes, rather than having to give and ask for a new address every time. I would like if this was more widely adopted, thus making it easier for people to avoid unintentional address reuse. Using payment codes has quite a nice user experience vs having to setup a BTC Pay Server, provided enough wallets support BIP47 payment codes. |
Well I guess in this case the fact that it's used in production in a few wallets means that whether we like it or not, the protocol is being used and somewhat standardized. There's also a BIP number, although I think this is still considered a "draft", but I don't think this is the first time a "draft" protocol ends up being used in production.
I don't think this is a good argument, you are looking way too far ahead. I agree that in, i don't know, 10 years time fees could be so high that most of the transaction will move to layer 2. But in the meantime we are not gonna just wait and do nothing: I would argue that actually us doing this work today is gonna be part of what makes bitcoin grow until users start moving to layer 2.
I think this is the PR with the most comments/reactions ever in the history of BDK. It's true that it was shared on Twitter while most of the others weren't, but still, it highlights that some people are interested in this. Also, my personal opinion is that BIP47 is pretty cool: it works with zero interaction between sender and receiver, it's relatively easy to get it working on light clients, while some of the other protocols that try to achieve the same goal are totally impossible to implement. BIP47 can be dangerous if you are not careful when sending payments, but if you just use this to receive funds I don't see how your privacy would be affected. Out of caution, we are gonna document very well how to be safe using this and we are also gonna add a warning to make sure people are aware of the risks and take precautions. |
|
Hi, I'm interested in bip47 and I picked up the branch as it is. I already rebased it to current master and I would like to try to finalize it, are you okay with that or did you change your mind on having bip47 inside bdk? |
FYI you might find this useful, appears someone has already build a BIP47 crate: https://lib.rs/crates/bip47 |
Hi, thanks for mentioning it, but it seems it's a lot more limited in scope than this PR (#549 (comment)). |
|
I read in BIP47 specs that for a refund "Bob MUST send a notification transaction to Alice prior to the first time he sends funds to Alice, even if he has received transactions from her in the past". That strikes me as unnecessary since Alice also has her notification transaction to Bob in her wallet and can create an inbound wallet using Bob's Payment Code at the same time she creates the outbound wallet to him. I think in case of wallet loss and restoration Alice would be able to find out she had a channel with Bob by retrieving this notif tx she sent? The only reason I see is that in case of wallet restoration it would be easier for a light client to ask for all transactions to a notification address rather than all the past transactions of the wallet, including those whose outputs were all spent in the meantime? But if outputs of the notif tx were eventually all spent isn't it the same? I don't remember if it's possible to both send and receive with some paynym you made a notif tx in Samourai, and documentation is not very clear on that point. I'm not familiar with the way it works in Sparrow. I'll try to find out. |
Personally, I think BIP47 could be a valuable addition. I don't really see any other protocol getting traction. |
It is possible to send the notification transaction from a different wallet than the one that manages the sender’s payment code, and in that case, a recovery would not rediscover their prior interaction with the receiver. While I would discourage¹ anyone from implementing or using BIP47, if you do pursue it, I would recommend implementing it as specified, i.e. that a notification transaction is created for each direction of sender and receiver pair. ¹I consider BIP47 detrimental to privacy, the following is especially about v1 as used by PayNyms, but from what I can tell the same applies to all other versions of BIP47. If on the other hand used with out-of-band notification, it loses its most attractive feature that all payments can be recovered from the original backup. |
Hi, thanks for the point about sending from another wallet it makes a lot of sense. I wasn't trying to reinvent the wheel (or the specifications) here, just trying to figure out why things are the way they are. I'm well aware of the notification payment limitations and the threat model you outline here can't be taken lightly, but on the other hand bip47 has other advantages and most importantly a lot more traction than any other competing proposal at the moment. We've been discussing it a lot recently in my local community and we basically agreed that despite its shortcomings bip47 is a pragmatic scheme that have been used for a while now and it just works, and while we're also really interested in competing proposals having bip47 in bdk would be useful. It is a kindof controversial proposal for a lot of reasons, and I've read all the arguments pros and cons made by you and others with interest, but now I think pretty much everything has been said, so just let's make it available on bdk and hopefully more generally used, it doesn't prevent us working on silent/private payments as a next step. I've been a bit busy with other stuff recently but I'm planning to resume work on this this month. |
|
Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open! |
An attacker wouldn't achieve anything by finding notification transactions for a payment code and I have shared this with example in this blog post: https://consentonchain.github.io/blog/posts/bip47/ |
Description
Closes #549
This PR adds support for BIP47: the idea is that the user can "attach" BIP47 support to an existing wallet, called "main_wallet" around the code. This wallet should be a P2PKH wallet, or one of the other types supported by the BIP (although BDK currently only support receiving payments from P2PKH wallets, more on that later).
Once attached the user will have its own payment code, and it will be able to send payments to other payment codes, by sending the notification transaction first, and then the actual payment.
Internally this is designed to keep track of all the other payment codes we've interacted with, either receiving from them or sending to them. We store both inbound and outbound wallets in a
BTreeMap<u32, Option<Wallet>>: the rationale behind this is that for each address we need to build a new wallet (that will have a single static private or public key). Since some indexes might be skipped because the shared secret is not in the secp group, we have to explicitly keep track of the index and storeNonewhen one of them happens to be an invalid wallet. Using aVecwould make things easier, but we would lose track of the index we are using.Stuff not implemented (yet)
The BIP also describes a few things that have not been implemented here: I will briefly describe all of them and explain they haven't been included.
Notes to the reviewers
This builds on top of #569.
There are a ton of missing things, but I wanted to get this out first to start gathering some feedback. Here's a todo list of random things that I'd like to do before we get this merged:
main_wallet, error if invalidWalletwhich could be easily moved into Expose more getters in Wallet and other useful descriptor traits #562Other nice-to-haves that could be part of future PRs:
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md