w0xlt
commented at 0:37 am on April 17, 2022:
contributor
This PR proposes an early version of Silent Payment (author:@RubenSomsen).
In this scheme, the recipient generates a public address, but the sender tweaks the address and the recipient detects the payment by verifying all transactions on the blockchain. An example use case would be private donations.
The purpose of this PR is not a final version, but to start the discussion and get benchmarks based on a real implementation.
This version is built on top of #994 (bitcoin-core/secp256k1) for x-only ECDH support and #23480 (bitcoin/bitcoin) for rawtr(). Each new silent transaction detected is stored in wallet as a rawtr() descriptor.
In this implementation, the sender can tweak the recipient address by passing the silent_payment option to send RPC. The transaction output will be different from the address entered.
For example ./src/bitcoin-cli -regtest -named send outputs="[{\"bcrt1pwlh5xuyrpgfunwyww8cfu78yfs2yqyevl7yturavahh5kgxwdd2q5hzgfu\": 1.1}]" fee_rate=1 options="{ \"silent_payment\": true}".
will generate vout with completely unrelated outputs:
Any wallet, as long as it has access to private keys, can send silent payments. Thus, this excludes watch-only wallets or wallets with external signers .
But the recipient’s wallet needs a new flag called SILENT_PAYMENT. This flag allows an additional scan that verifies that the wallet keys match the silent payment scheme. When it detects a silent payment that belongs to the wallet, it is stored in a rawtr() descriptor.
Therefore, scanning each address for each transaction is potentially prohibitive overhead, so the node can be initialized with keypool=1 or a descriptor with range [0,1] can be imported into a blank wallet. Until there is more benchmark data, it is the safest option. The proposal recommends one static address.
I’ve been running some silent payments on signet using wallets with default keypool and default range, I haven’t noticed any relevant performance drops on the signet node.
Apparently this implementation is working as expected but I can’t guarantee that the scheme is implemented correctly or safely, so I’m opening this PR for reviews, modifications and improvements.
There is a new functional test (test/functional/wallet_silentpayment.py) that can help to better understand the implementation.
DrahtBot added the label
Build system
on Apr 17, 2022
DrahtBot added the label
Descriptors
on Apr 17, 2022
DrahtBot added the label
RPC/REST/ZMQ
on Apr 17, 2022
DrahtBot added the label
Upstream
on Apr 17, 2022
DrahtBot added the label
Utils/log/libs
on Apr 17, 2022
DrahtBot added the label
Wallet
on Apr 17, 2022
DrahtBot
commented at 4:48 am on April 17, 2022:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
ghost
commented at 1:54 am on April 18, 2022:
none
Concept ACK
in
src/wallet/silentpayment.cpp:64
in
b97fee2f44outdated
I’m just trying to understand the protocol better. This function is called “create tweaked pubkey” but what seems more significant about it to me is that it also provides the recipient with the private key they need to spend from the silent payment output. Is that correct?
Yes. I defined this name when using another approach, but that’s the idea.
secp256k1_keypair_xonly_tweak_add returns both the tweaked private and public keys in the recipient_keypair parameter. So the recipient can spend the output.
DrahtBot added the label
Needs rebase
on Apr 18, 2022
w0xlt force-pushed
on Apr 19, 2022
DrahtBot removed the label
Needs rebase
on Apr 19, 2022
luke-jr
commented at 7:06 pm on April 19, 2022:
member
Need some way to avoid users trying to send a silent payment to a wallet that doesn’t support it… am I missing something?
w0xlt
commented at 7:12 am on April 20, 2022:
contributor
@luke-jr Yes, it will need to be further discussed if this concept proves to be valid.
In this PR, all descriptor wallets support silent transactions as the SILENT_PAYMENT flag can be enabled or disabled at any time. The rescanblockchain RPC can be used to retrieve previous silent payments.
As only Taproot addresses can be used for silent payments and only descriptor wallets support Taproot, this may suffice, but it certainly needs further discussion.
RubenSomsen
commented at 8:45 am on April 20, 2022:
none
Need some way to avoid users trying to send a silent payment to a wallet that doesn’t support it
@luke-jr When the recipient publishes their silent payment address, it needs to be clear that it is not a regular taproot address. The address format should be distinct. The tweaked silent payment address that is generated by the sender and is ultimately committed on-chain should be indistinguishable from a regular taproot output.
only Taproot addresses can be used for silent payments
@w0xlt Note that nothing about the silent payment scheme is exclusive to taproot, but regardless I do think it’s good to limit the specifications to creating taproot outputs only as it encourages taproot use and means transactions without taproot outputs (albeit hopefully rare in the eventual future) don’t need to be scanned.
luke-jr
commented at 6:00 pm on April 20, 2022:
member
When the recipient publishes their silent payment address, it needs to be clear that it is not a regular taproot address. The address format should be distinct.
But I don’t see that in this current spec/code?
w0xlt force-pushed
on Apr 22, 2022
DrahtBot added the label
Needs rebase
on Apr 26, 2022
w0xlt force-pushed
on Apr 27, 2022
w0xlt force-pushed
on Apr 27, 2022
DrahtBot removed the label
Needs rebase
on Apr 28, 2022
w0xlt force-pushed
on Apr 28, 2022
JeremyRubin
commented at 4:32 am on April 28, 2022:
contributor
Need some way to avoid users trying to send a silent payment to a wallet that doesn’t support it… am I missing something?
it’s worse than that, AFAIU the address needs to support it not just the wallet – E.g., what if the Tr key is a NUMS point?
Worth making a new address type / format so that you never mix the two.
w0xlt force-pushed
on May 5, 2022
w0xlt force-pushed
on May 8, 2022
w0xlt force-pushed
on May 8, 2022
w0xlt
commented at 6:54 pm on May 16, 2022:
contributor
@luke-jr@JeremyRubin Thanks for the comments. The current version deliberately does not implement a new address/descriptor format. This requires a separate discussion and changes in wallet behavior. And it doesn’t make sense to start a discussion about it before the scheme proves itself valid in a viable time.
Currently the main focus is to validate the scheme and verify that the performance is viable.
w0xlt
commented at 6:55 pm on May 16, 2022:
contributor
There is a suggestion by @RubenSomsen to use a hash of all inputs. This would make silent payments compatible with coinjoins.
This is a possibility of change if the current version proves viable, since there shouldn’t be much difference in performance between the two versions.
0Please use bracket syntax includes ("#include <foo.h>") instead of quote syntax includes:
1src/wallet/silentpayment.cpp:#include "silentpayment.h"
2^---- failure generated from lint-includes.py
in
src/wallet/wallet.cpp:1249
in
8f616acf86outdated
1273- // Block disconnection override an abandoned tx as unconfirmed
1274- // which means user may have to call abandontransaction again
1275- TxState tx_state = std::visit([](auto&& s) -> TxState { return s; }, state);
1276- return AddToWallet(MakeTransactionRef(tx), tx_state, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, rescanning_old_block);
1277+ // TODO: can more than one txin.prevout be in the mempool (RBF) ?
1278+ assert( ++pos == coins.end() );
Linter does not like the name of include guard define:
0src/wallet/silentpayment.h seems to be missing the expected include guard:
1#ifndef BITCOIN_WALLET_SILENTPAYMENT_H
2#define BITCOIN_WALLET_SILENTPAYMENT_H
3 ...
4#endif // BITCOIN_WALLET_SILENTPAYMENT_H
in
src/rpc/blockchain.cpp:1936
in
8f616acf86outdated
1933+ throw JSONRPCError(RPC_VERIFY_ERROR, "The silent payment index is either disabled or not yet synced.");
1934+ }
1935+
1936+ ChainstateManager& chainman = EnsureChainman(node);
1937+ LOCK(cs_main);
1938+ CChainState& active_chainstate = chainman.ActiveChainstate();
0In file included from wallet/wallet.cpp:44:
1wallet/wallet.cpp: In member function ‘bool wallet::CWallet::ExtractPubkeyFromInput(const CTxIn&, XOnlyPubKey&)’:
2wallet/wallet.cpp:1292:40: warning: comparison of integer expressions of different signedness: ‘std::vector<unsigned char>::size_type’ {aka ‘long unsigned int’} and‘int’ [-Wsign-compare]
31292| assert(serializedPubKey.size() == pubKeySize);
4|~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
Sjors
commented at 2:18 pm on June 2, 2022:
member
Very cool! It’s nice to be able to test this concept with “real” demo software. I’ll share some thoughts about the concept on the mailinglist or gist. Will focus on the implementation here.
I was able to send to signet coins to myself. Here’s the public key: tb1p0ykcmjxv26utyntns8ryn70at9t38zkdta9fj3zrrnpnczuddxvqf8qwac
My first thought was that it may be better (initially) to implement silent payments as a standalone tool that uses the Bitcoin Core RPC.
However, given that this implementation requires a new index, I don’t know how practical that is. Getting a new index added to Bitcoin Core however is also not easy (https://bitcoincore.reviews/14053). The light at the end the tunnel here would be a plugin system for indexes, which should be possible once process separation is complete. See #24230. That will take a while though.
Another reason for building a separate tool that uses the RPC is that this proposal has a pretty big resource need, something that the project may not want to commit to anytime soon. The sending side is not resource intensive though, so perhaps that is easier to get in.
Limiting the scope of this implementation to taproot makes sense. Limiting it to the first input seems brittle however, hopefully it’s not too difficult to scan them all. Other implementations might order the input coins differently. Also, if you go for the approach of building an external tool that uses the RPC, then you can could use send with 1 fixed input and have the wallet automatically select the other coins. But in that case the RPC gives no guarantee which position the input will end up (it might be the first in practice atm).
In case the proposal is modified to consider all inputs, keep in mind that this makes coin selection (slightly) more complicated. The wallet has to decide, before it knows the output address, which inputs to use. That would involve some surgery in the way coin selection is integrated in the wallet. When implemented as an external tool, you would probably make two calls to send: one with a dummy destination and automatic coin selection to get the coins, and then one with manual coin selection and the real destination. But that’s not pretty, and won’t work if the wallet has a combination of taproot and non-taproot coins, because you currently can’t tell coin selection to only use taproot inputs.
Importing a descriptor with a shorter range than the keypool size has no effect afaik, it’s just treated as a descriptor with range 0-1000. We could change that behavior, so that a descriptor imported with a fixed range is not expanded unless you re-import it. Not sure if that’s ideal, cc @achow101. Another option might be to make the keypool configurable per wallet, though that also feels a bit meh. A user may have multiple wallets, not all of which use this feature, so relying on a global keypool=1 setting is not ideal. And a factor 1000 slower scanning seems suboptimal.
Ideally the scriptPubKeys are stored in the silent(pk) descriptor. This would make getaddressinfo return the right descriptor. In that case getnewaddress should always fail, because the addresses are not known in advance. You’d need another way to communicate the public key. The easiest hack might be to return it as public-key version of the silent(pk) descriptor, so the user can find it with either listdescriptors or getaddressinfo on any received payment.
w0xlt force-pushed
on Jun 4, 2022
DrahtBot removed the label
Needs rebase
on Jun 4, 2022
w0xlt
commented at 9:57 pm on June 5, 2022:
contributor
@Sjors@kristapsk@1440000bytes Thanks for reviews and tests. I addressed your suggestions in the last force-push.
@Sjors I agree that using a separate wallet/tool for silent payments can be a good solution.
I initially considered this option, but it would imply re-implementing the scan logic. The tool must be able to request and de-serialize blocks and the UTXO Set to scan coins.
So a new branch appeared to be the simplest way to implement a proof of concept.
In case the proposal is modified to consider all inputs, keep in mind that this makes coin selection (slightly) more complicated.
given that this implementation requires a new index,
This index just speeds up the transaction verification. This is not necessary for Silent Payments, although in this particular implementation it is required for UX purposes. I’m getting some metrics to see if it can be removed and still have a good user experience.
relying on a global keypool=1 setting is not ideal.
Yes. This is just for testing and should not be required in an eventual final version.
Another approach could be that a silent_payment wallet only ever generates 1 key, perhaps with a new silent(pk) descriptor type.
I think this is the best solution. This would imply changing the getnewaddress RPC to always return the same address for this silent descriptor and allowing a non-ranged descriptor to be active.
DrahtBot added the label
Needs rebase
on Jul 12, 2022
w0xlt force-pushed
on Jul 15, 2022
w0xlt
commented at 11:24 pm on July 15, 2022:
contributor
One of the error is the same as #23480, which occurs in wallet_taproot.py. This is failing almost every CI check, but not on my machine (Ubuntu 21.10).
Regarding the error inwallet_taproot.py, I suggested the solution in #23480 (comment), but had forgotten to commit it here. This error has now been fixed.
The other error is a lint error about manually merging #994. I’m still not sure how to fix this lint error.
EDIT: The only error now is the lint error mentioned above. From what I understand from the file test/lint/git-subtree-check.sh, this is because the src/secp256k1 subtree is compared with the master branch of https://github.com/bitcoin-core/secp256k1 and as it is different, it results in an error.
Anyway, it’s a lint error (formatting basically). Does not affect functionality.
w0xlt force-pushed
on Jul 16, 2022
w0xlt force-pushed
on Jul 16, 2022
w0xlt force-pushed
on Jul 16, 2022
luke-jr
commented at 5:29 am on July 16, 2022:
member
Another approach could be that a silent_payment wallet only ever generates 1 key, perhaps with a new silent(pk) descriptor type. That might also make it easier for getnewaddress to spit out something that’s not actually an address.
How would you apply a label and determine who the sender is?
Also, being non-reversible from the UTXO doesn’t make it any less of an address…
w0xlt
commented at 6:03 am on July 16, 2022:
contributor
@luke-jr Not sure if I understand your question, but this scheme doesn’t change anything regarding the sender/recipient relationship.
The sender will know the tweaked address and the recipient will know where the coins are coming from.
What this proposal prevents is external observers from knowing or tracking how many coins or transactions a public address has.
And it also prevents reuse of a public address (as it happens in on-chain donations, for example).
luke-jr
commented at 2:46 pm on July 16, 2022:
member
Not sure if I understand your question, but this scheme doesn’t change anything regarding the sender/recipient relationship.
The sender will know the tweaked address and the recipient will know where the coins are coming from.
The recipient knows who the sender is and the purpose of the transaction, based on the address the bitcoins were sent to. If getnewaddress gave the same address every call, it would be impossible to tell the difference between them.
And it also prevents reuse of a public address (as it happens in on-chain donations, for example).
I thought the whole point was to make reuse of such an address safe?
w0xlt
commented at 3:14 pm on July 16, 2022:
contributor
If getnewaddress gave the same address every call
You can label the tweaked address. It is part of the wallet. The rawtr(pk) descriptor is normally added to wallet.
I thought the whole point was to make reuse of such an address safe?
Yes. That’s the point. The same address will be tweaked on every transaction
As I understand it, the purpose of SP is primarily to increase privacy in a situation where you need to publicly post an address and anyone can send coins to it.
If you need more control to categorize each address by payer, an HD wallet is better.
luke-jr
commented at 3:31 pm on July 16, 2022:
member
You can label the tweaked address. It is part of the wallet.
That seems like a bad design… it should show the silent address.
But more importantly, for the recipient to set a label, he has to know what it’s for first….
w0xlt
commented at 3:37 pm on July 16, 2022:
contributor
it should show the silent address.
This can be done. The wallet knows that a certain transaction / address was found by verifying SP.
unknown approved
unknown
commented at 8:19 am on July 17, 2022:
none
Removed in d554e067c7715b736062312463fa8a1e36ed48c2. Thanks for review.
w0xlt force-pushed
on Aug 1, 2022
w0xlt force-pushed
on Aug 1, 2022
DrahtBot added the label
Needs rebase
on Aug 2, 2022
w0xlt force-pushed
on Aug 3, 2022
DrahtBot removed the label
Needs rebase
on Aug 3, 2022
DrahtBot added the label
Needs rebase
on Aug 5, 2022
w0xlt force-pushed
on Aug 9, 2022
DrahtBot removed the label
Needs rebase
on Aug 9, 2022
DrahtBot added the label
Needs rebase
on Aug 9, 2022
w0xlt force-pushed
on Aug 10, 2022
DrahtBot removed the label
Needs rebase
on Aug 10, 2022
w0xlt force-pushed
on Aug 16, 2022
w0xlt
commented at 0:09 am on August 17, 2022:
contributor
This PR has been updated with a new silent payment version, which eliminates some manual steps from the previous version (such as the need to set the keypool to avoid costly multi-key scan).
This is achieved by using a new descriptor type (sp()) that has no range and contains exactly one key.
Example: sp(cQq73sG9....JD51uaRD)#9llg6xjm
This descriptor introduces a new type of output: silent-payment. This output type returns a standard Taproot script (Segwit V1), but with HRP changed from bc to sp on the mainnet (or tsp on testnet and signet).
This output type will always generate the same address (unless another sp descriptor is enabled on the same wallet).
DrahtBot added the label
Needs rebase
on Aug 17, 2022
w0xlt force-pushed
on Aug 17, 2022
DrahtBot removed the label
Needs rebase
on Aug 17, 2022
LLFourn
commented at 1:37 pm on August 17, 2022:
none
is the idea of a scanning key included in this? I would have expected sp(...) to contain 64 bytes (two xonly keys).
Kixunil
commented at 3:24 pm on August 17, 2022:
none
I think there is a use case for multiple SP addresses and this is probably what @luke-jr meant. Let’s say I work on two Open Source projects. I could post the same SP address for both but it’d be nicer to post a separate SP address for each. Then I would be able to determine how much people appreciate each project. Perhaps most donations go only to one of them so I should focus on that one? It may be an advanced use case but a very reasonable one. I’d myself definitely prefer this over having one address.
luke-jr
commented at 4:03 pm on August 17, 2022:
member
Many people don’t receive only donations. If you have multiple clients, you need a different address for each so you know who is paying you.
ghost
commented at 4:03 pm on August 17, 2022:
none
@Kixunil 2 wallets (different silent payment address) for 2 projects would also make sense
ghost
commented at 4:05 pm on August 17, 2022:
none
Many people don’t receive only donations. If you have multiple clients, you need a different address for each so you know who is paying you.
@luke-jr client could share txid in which he sent the payment
Kixunil
commented at 4:16 pm on August 17, 2022:
none
@luke-jr indeed, though you could already send a different xpub to each client and avoid some scanning costs. SP address may be nicer though, not sure.
@1440000bytes yes, those hacks are available but I’d prefer to avoid such hacks because they significantly degrade UX.
ghost
commented at 4:28 pm on August 17, 2022:
none
@1440000bytes yes, those hacks are available but I’d prefer to avoid such hacks because they significantly degrade UX.
Not sure I would consider it hack because it makes things simple. Even BIP 47 has one reusable code per wallet so I would find it confusing to have multiple silent payment address in the same wallet. Although no strong opinion and won’t mind anything as long as silent payment address is available in core.
Kixunil
commented at 4:41 pm on August 17, 2022:
none
May make spending funds together impossible/difficult
Managing multiple wallets is PITA
Sending txid is just more work for humans that computers could do instead
I think these are enough reasons to at least support multiple addresses in descriptors and leaving it up to wallets to decide whether to provide support for more than one in GUI/CLI/…
w0xlt force-pushed
on Aug 17, 2022
w0xlt force-pushed
on Aug 17, 2022
w0xlt force-pushed
on Aug 17, 2022
luke-jr
commented at 10:55 pm on August 17, 2022:
member
client could share txid in which he sent the payment
Anyone could claim to have sent it. There is still no proof-of-sender in Bitcoin, and it doesn’t look like anyone is working on it.
Even if it were possible, it’d still be a pain - this kind of thing should “just work” and label transactions, without extra steps from the users.
Even BIP 47 has one reusable code per wallet
BIP 47 was a bad design that shouldn’t be implemented nor imitated…
w0xlt
commented at 11:20 pm on August 17, 2022:
contributor
The default behavior for most descriptors is to be ranged and therefore support multiple addresses.
If the idea is to have multiple sp addresses, a ranged sp version is sufficient (and is easier to implement than the current version).
The decision to use a rangeless descriptor is for validation purposes.
Verification of silent payments is costly. And if the user inadvertently generates multiple sp addresses, the overhead can be prohibitive.
Until there are more reliable statistics on how many sp addresses a node (even on a raspberrey) can process (from mempool and when loading wallets), I think it’s prudent to keep only one active address per wallet.
The label issue shouldn’t be complicated, I think.
UTXO can inherit the same label assigned to the sp address or descriptor.
I can work on that in the next iteration.
RubenSomsen
commented at 11:26 pm on August 17, 2022:
none
I think there is a use case for multiple SP addresses
@Kixunil good point about wanting to identify the reason for the payment. Naively, the issue is that two keys means twice the scanning, but an interesting alternative would be to simply use the same key (assuming you’re OK with using the same identity) but add a public identifier f to it when tweaking. So instead of hash(i*X)*G + X you get hash(i*X)*G + X + f*G . This means every additional “address” only costs one additional ECC addition when scanning (relatively cheap compared to doing ECC multiplications).
The main downside with this is that f becomes crucial for recovering from backup. If we set f as an index (0, 1, 2, 3…) then you’d only have to remember how many “addresses” you issued (and perhaps overshoot when unsure) to ensure recovery of funds, though of course you’d rather also remember which index is associated with what payment reason.
Absolute worst case scenario you could even do something similar to the gap limit where you scan the full history (not just the UTXO set so you don’t miss spent outputs) with a default max index of e.g. 100, and then if you find out most of them are in use, you scan the next 100, etc. Costly, but thorough, and only needed as a last resort.
DrahtBot added the label
Needs rebase
on Aug 19, 2022
w0xlt force-pushed
on Aug 20, 2022
DrahtBot removed the label
Needs rebase
on Aug 20, 2022
DrahtBot added the label
Needs rebase
on Aug 31, 2022
ghost
commented at 6:45 am on September 1, 2022:
none
This PR has been updated with a new silent payment version, which eliminates some manual steps from the previous version (such as the need to set the keypool to avoid costly multi-key scan).
This is achieved by using a new descriptor type (sp()) that has no range and contains exactly one key.
Example: sp(cQq73sG9....JD51uaRD)#9llg6xjm
This descriptor introduces a new type of output: silent-payment. This output type returns a standard Taproot script (Segwit V1), but with HRP changed from bc to sp on the mainnet (or tsp on testnet and signet).
This output type will always generate the same address (unless another sp descriptor is enabled on the same wallet).
The purpose of this PR is not a final version, but to start the discussion and get benchmarks based on a real implementation.
T> his version is built on top of https://github.com/bitcoin-core/secp256k1/pull/994 (bitcoin-core/secp256k1) for x-only ECDH support and #23480 (bitcoin/bitcoin) for rawtr(). Each new silent transaction detected is stored in wallet as a rawtr() descriptor.
Can you confirm this is one of the version that can be used for bitcoin?
w0xlt force-pushed
on Sep 15, 2022
DrahtBot removed the label
Needs rebase
on Sep 15, 2022
w0xlt force-pushed
on Sep 15, 2022
w0xlt
commented at 3:48 pm on September 18, 2022:
contributor
I am not sure I understand your question. The idea of current implementation is to be used with Bitcoin.
I’m evaluating the possibility of including more than one sp address per wallet, but that would be a future change .
ghost
commented at 3:55 pm on September 18, 2022:
none
Sorry I forgot to mention ‘mainnet’ after bitcoin. Wanted to know if this version is good enough for mainnet and what else could be done to get more reviews and eventually get this merged before v25.0
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
w0xlt force-pushed
on Sep 29, 2022
luke-jr
commented at 9:57 pm on September 29, 2022:
member
@1440000bytes This isn’t even close to ready for review yet… It still needs at least a BIP and support for generating multiple addresses. And the secp256k1 changes also haven’t been merged yet, much less included in the version Bitcoin Core uses.
Suggestion: We could add a new RPC command to create silent payment address instead of using getnewaddress.
Why? getnewaddress is the correct thing to use here…
w0xlt
commented at 10:03 pm on September 29, 2022:
contributor
This new version addresses most (or all) requests made in PR so far:
. Implements the new scheme suggested by @RubenSomsen that allows multiple silent addresses per wallet with minimal overhead.
. Implements a new RPC to retrieve silent addresses (suggested by @1440000bytes), which allows users to assign different labels to different addresses. That way, the user knows which silent address the UTXO came from (as suggested by @luke-jr).
Example:
0./src/bitcoin-cli -signet -rpcwallet="receiver" getspaddress
1tsp001pjgcwd9p6f2rcgf35dlgvj77h2afylg6lp5cdn0cztrk4k54w99kqxn48tq
23# This will return the same address as above (both have no label)4./src/bitcoin-cli -signet -rpcwallet="receiver" getspaddress
5tsp001pjgcwd9p6f2rcgf35dlgvj77h2afylg6lp5cdn0cztrk4k54w99kqxn48tq
67# New label, new address8./src/bitcoin-cli -signet -rpcwallet="receiver" getspaddress 'donation'9tsp011pjgcwd9p6f2rcgf35dlgvj77h2afylg6lp5cdn0cztrk4k54w99kq80t7lt
In this new scheme, the address has a new field called identifier, which tells the receiver and sender how to tweak the address correctly.
If the receiver, for whatever reason, doesn’t know which identifiers have been used, there is no problem. The wallet can scan all identifiers from 0 to 99. Currently, only 100 different identifiers per wallet are allowed. This limit, however, can be increased at any time in the future.
Unlike address formats so far, sp addresses are not script-related and may eventually include any additional information needed, such as an expiration timestamp (or block height). That way, users don’t have to track the address indefinitely.
ghost
commented at 10:08 pm on September 29, 2022:
none
@1440000bytes This isn’t even close to ready for review yet… It still needs at least a BIP and support for generating multiple addresses. And the secp256k1 changes also haven’t been merged yet, much less included in the version Bitcoin Core uses.
Suggestion: We could add a new RPC command to create silent payment address instead of using getnewaddress.
Why? getnewaddress is the correct thing to use here…
I respect for your review. At least we know whats needed for bitcoin core/knots to use this.
w0xlt
commented at 10:12 pm on September 29, 2022:
contributor
Why? getnewaddress is the correct thing to use here…
I accepted the suggestion of @1440000bytes because getspaddress will not necessarily return a new address. If the user repeats the label, the address will be the same, so getnewaddress does not exactly represent the expected behavior.
ghost
commented at 10:41 pm on September 29, 2022:
none
@1440000bytes This new push addresses all requests made so far, but as @luke-jr said, there is still a long way to go before this PR is merged (if at all). bitcoin-core/secp256k1#994 needs to be merged first.
Why? getnewaddress is the correct thing to use here…
I accepted the suggestion of @1440000bytes because getspaddress will not necessarily return a new address. If the user repeats the label, the address will be the same, so getnewaddress does not exactly represent the expected behavior.
I dont want this as another “proof of concept”
Users need privacy right now and it will be an inspiration when core wallet does it with something like silent payment. We don’t need to share bitcoin address on social media.
Ross https://twitter.com/raw_avocado/status/1575110869809143808 got it wrong with opsec by sharing email address on bitcointalk. Lot of users were tracked because they shared bitcoin address on social media. Lets fix this partially by allowing the users to share a silent payment address.
ghost
commented at 10:45 pm on September 29, 2022:
none
luke-jr
commented at 11:07 pm on September 29, 2022:
member
This new version addresses most (or all) requests made in PR so far:
I just finished going over the BIPs PRs and didn’t see one for this yet.
In this new scheme, the address has a new field called identifier, which tells the receiver and sender how to set the address correctly.
Wouldn’t this allow someone who obtains two addresses, to determine they are both for the same wallet? Seems like a major privacy problem.
Currently, only 100 different identifiers per wallet are allowed. This limit, however, can be increased at any time in the future.
Any limit seems problematic, but 100 is definitely too few.
Why? getnewaddress is the correct thing to use here…
I accepted the suggestion of @1440000bytes because getspaddress will not necessarily return a new address. If the user repeats the label, the address will be the same, so getnewaddress does not exactly represent the expected behavior.
I see, that makes sense. getaccountaddress used to do something similar. We no longer have accounts, but maybe a more generic name getreusableaddress would be better. (I assume getnewaddress can also be used still?)
in
src/wallet/wallet.cpp:1335
in
06037565e9outdated
in
src/rpc/blockchain.cpp:2191
in
79402460adoutdated
2191- needles.emplace(script);
2192- descriptors.emplace(std::move(script), std::move(inferred));
2193+ // auto scripts = EvalDescriptorStringOrObject(scanobject, provider);
2194+ auto ret = EvalDescriptorStringOrObject(scanobject);
2195+ auto desc_str = std::get<0>(ret);
2196+ auto range = std::get<1>(ret);
aureleoules
commented at 3:50 pm on September 30, 2022:
c53eec1c25c4a94935ae8e775c09e71ef24a3d0b
0 auto [desc_str, range] = EvalDescriptorStringOrObject(scanobject);
aureleoules
commented at 4:15 pm on September 30, 2022:
member
Left some code review nits/minor I found while looking at the code.
No need to address them now as I think this still needs conceptual review and benchmarking.
Will review and test this PR further later.
aureleoules
commented at 12:24 pm on October 3, 2022:
member
Currently, only 100 different identifiers per wallet are allowed. This limit, however, can be increased at any time in the future.
This identifier is encoded in the sp address, isn’t it sensitive information to leak?
Any limit seems too low, especially 100. Can’t the identifier be encoded using a CompactSize?
clarkmoody
commented at 10:53 pm on October 3, 2022:
none
This identifier is encoded in the sp address, isn’t it sensitive information to leak?
A common feature of these stealth address schemes (BIP47, BIP351) is that a static identifier may be safely associated with your identity without loss of privacy.
luke-jr
commented at 10:57 pm on October 3, 2022:
member
But that’s not correct. If someone acquires both, they now know they pay to the same wallet.
RubenSomsen
commented at 11:26 pm on October 3, 2022:
none
There seems to be some confusion about the function of the identifier. Its function is not to create more than one identity, but to be able to distinguish why someone paid you. For example, when the same entity is raising money on behalf of two different charities and wants to know for which of the two charities the sender intended their donation.
To be absolutely clear, the payments that appear on-chain are still absolutely unlinkable by a third party observer, nor are they able to tell which identifier was used.
In cases where you don’t want people to know that you’re the same entity, the identifier is insufficient – you’ll need a completely separate Silent Payment address which (roughly) doubles your scanning efforts, so it’s much more costly in terms of performance.
luke-jr
commented at 11:45 pm on October 3, 2022:
member
This is a serious privacy leak that no other invoice address format has right now (maybe Lightning? not sure), and is not generally expected/known.
RubenSomsen
commented at 0:26 am on October 4, 2022:
none
@luke-jr I suspect we’re not yet on the same page. Could you be specific about what you think is being leaked? Essentially all that is happening is that the person who is getting paid is publicly saying “you could send me money for purpose A or purpose B, so please tweak the final address a bit so I know for which purpose you sent me money”. The resulting on-chain address is still completely unrecognizable by outside parties.
So essentially all that is happening is that a few extra secret bits of information are being transmitted to the recipient along with the payment. There is no impact on privacy other than the fact that the recipient voluntarily made a statement about how it would interpret those bits.
Of course this is not equivalent to having a completely separate identity on which you could get paid, but that is not its purpose. I.e. the difference is A says pay me for X, B says pay me for Y, and A might be the same person as B but nobody knows, or A saying pay me for X or Y so we know the same person will receive the funds no matter if you pay for X or Y. Both are useful. One is more flexible than the other, but also more computationally expensive.
luke-jr
commented at 0:43 am on October 4, 2022:
member
In your example earlier, charity A does not necessarily want to leak that it is using the same service as charity B. Throughout Bitcoin’s history, there has never been any leak of such relationships between invoice addresses. It would be very unexpected, especially for it to be introduced with a privacy improvement!
RubenSomsen
commented at 0:57 am on October 4, 2022:
none
@luke-jr I agree it is a communication challenge to explain such a feature to users, but I hope you see now that it can be of benefit when used correctly. I also agree that this idea is not adequate in cases where you do not want people to know that the same entity is receiving money for separate purposes.
Another example which may be more clear: maybe you’re a known open source developer who is openly working on multiple projects and you want to distinguish for which of these projects your supporters are donating.
luke-jr
commented at 3:08 am on October 4, 2022:
member
@RubenSomsen I agree it may have use cases, but if the user asks for a new address, they should get a new address, not a variation of an existing one. (Unless they explicitly ask for a variation, which I would suggest doing in a followup PR since it necessarily adds a new API and concept)
RubenSomsen
commented at 9:22 am on October 4, 2022:
none
@luke-jr I agree it may not be a good idea to refer to the same pubkey/identity + different identifier as a “new address” if that sets the wrong expectations for users. Thanks for clarifying.
DrahtBot added the label
Needs rebase
on Oct 4, 2022
w0xlt force-pushed
on Oct 5, 2022
w0xlt force-pushed
on Oct 5, 2022
DrahtBot removed the label
Needs rebase
on Oct 5, 2022
w0xlt
commented at 11:53 am on October 5, 2022:
contributor
@aureleoules Thanks for the detailed review. I addressed all the suggestions.
the user asks for a new address, they should get a new address
@luke-jr as mentioned earlier, that’s why a new RPC was created instead of using getnewaddress, just so the user doesn’t think a new address will be generated.
w0xlt force-pushed
on Oct 11, 2022
w0xlt force-pushed
on Oct 11, 2022
w0xlt force-pushed
on Oct 11, 2022
w0xlt
commented at 7:01 am on October 11, 2022:
contributor
Silent Payment v4 (coinjoin support added)
Changes:
. Silent payments now use all inputs to create transactions. Previously, they only used the first input. This change increases privacy and makes silent payments compatible with coinjoin.
. getspaddress RPC renamed to getsilentaddress for clarity
. Added support for silent payment in PSBT via walletcreatefundedpsbt RPC.
. Added a new index scheme (which stores the sum of input public keys for each transaction). The previous index bitcoin/signet/indexes/silentpaymentindex should be removed as it is no longer compatible with this new version.
As inputs can be Taproot, this introduced a new issue as bitcoin-core/secp256k1 does not support x-only public key sum (perhaps due to missing prefix byte).
I opened a new PR https://github.com/bitcoin-core/secp256k1/pull/1143 to add a function to convert from x-only to compressed public key with even y. This is the solution being used by the current silent payment implementation.
aureleoules
commented at 12:30 pm on October 11, 2022:
member
How about returning a JSON object with some warnings for new labels to explicit that new labels are not new identities?
0./src/bitcoin-cli -testnet -rpcwallet=sp1 getsilentaddress client1
1{ 2"address": "tsp071pp809z56vwvyqhwc4fsjq8vhjw6yyguzv2u9hnwt3jvyl2ha49nwqtsc8w9",
3"index": 7,
4"label": "client1",
5"warnings": "This address is not a new identity. It is a re-use of an existing identity with a different label." 6} 7./src/bitcoin-cli -testnet -rpcwallet=sp1 getsilentaddress client1
8{ 9"address": "tsp071pp809z56vwvyqhwc4fsjq8vhjw6yyguzv2u9hnwt3jvyl2ha49nwqtsc8w9",
10"label": "client1",
11"index": 712}13./src/bitcoin-cli -testnet -rpcwallet=sp1 getsilentaddress client2
14{15"address": "tsp081pp809z56vwvyqhwc4fsjq8vhjw6yyguzv2u9hnwt3jvyl2ha49nwqyqkdah",
16"index": 8,
17"label": "client2",
18"warnings": "This address is not a new identity. It is a re-use of an existing identity with a different label."19}20./src/bitcoin-cli -testnet -rpcwallet=sp1 getsilentaddress client2
21{22"address": "tsp081pp809z56vwvyqhwc4fsjq8vhjw6yyguzv2u9hnwt3jvyl2ha49nwqyqkdah",
23"label": "client2",
24"index": 825}
0diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
1index 1f8621d48..cd6165cef 100644
2--- a/src/wallet/rpc/addresses.cpp
3+++ b/src/wallet/rpc/addresses.cpp
4@@ -110,10 +110,14 @@ RPCHelpMan getsilentaddress()
5 if (!request.params[0].isNull())
6 label = LabelFromValue(request.params[0]);
7 8+ UniValue obj(UniValue::VOBJ);
9 for (auto const& [key, val] : pwallet->m_silent_address_book)
10 {
11 if (val.m_label == label) {
12- return val.m_address;
13+ obj.pushKV("address", val.m_address);
14+ obj.pushKV("label", val.m_label);
15+ obj.pushKV("index", key);
16+ return obj;
17 }
18 }
1920@@ -128,7 +132,16 @@ RPCHelpMan getsilentaddress()
21 throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, util::ErrorString(op_dest).original);
22 }
2324- return EncodeDestination(*op_dest, silent_payment, current_index);
25+ const auto address{EncodeDestination(*op_dest, silent_payment, current_index)};
26+
27+ obj.pushKV("address", address);
28+ obj.pushKV("index", current_index);
29+ obj.pushKV("label", label);
30+ if (current_index > 0) {
31+ obj.pushKV("warnings", "This address is not a new identity. It is a re-use of an existing identity with a different label.");
32+ }
33+
34+ return obj;
35 },
36 };
37 }
in
src/index/silentpaymentindex.cpp:51
in
978fbe4a7doutdated
aureleoules
commented at 12:37 pm on October 11, 2022:
in c9b3e312d3579be66043934c3a0c1f13911ab715
This function seems to be largely duplicated from the function CWallet::ExtractPubkeyFromInput in src/wallet/wallet.cpp, is there a way to re-use code?
aureleoules
commented at 12:47 pm on October 11, 2022:
in de25bcf6b7d59ad8803748412825a99bdb812cf3
This code is from std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, FlatSigningProvider& provider), you should re-use it.
0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
1index 084d165da..672d3fd98 100644
2--- a/src/rpc/util.cpp
3+++ b/src/rpc/util.cpp
4@@ -1073,24 +1073,7 @@ std::vector<CScript> EvalDescriptorStringOrObject(const UniValue& scanobject, Fl
5 throw JSONRPCError(RPC_INVALID_PARAMETER, "Scan object needs to be either a string or an object");
6 }
7 8- std::string error;
9- auto desc = Parse(desc_str, provider, error);
10- if (!desc) {
11- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
12- }
13- if (!desc->IsRange()) {
14- range.first = 0;
15- range.second = 0;
16- }
17- std::vector<CScript> ret;
18- for (int i = range.first; i <= range.second; ++i) {
19- std::vector<CScript> scripts;
20- if (!desc->Expand(i, provider, scripts, provider)) {
21- throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Cannot derive script without private keys: '%s'", desc_str));
22- }
23- std::move(scripts.begin(), scripts.end(), std::back_inserter(ret));
24- }
25- return ret;
26+ return DescriptorToScripts(desc_str, range, provider);
27 }
2829 std::tuple<std::string, std::pair<int64_t, int64_t>> EvalDescriptorStringOrObject(const UniValue& scanobject)
107+ auto dest_silent_payment_hrp = ToLower(std::string_view(str).substr(0, params.SilentPaymentHRP().size()));
108109 // Note this will be false if it is a valid Bech32 address for a different network
110- bool is_bech32 = (ToLower(str.substr(0, params.Bech32HRP().size())) == params.Bech32HRP());
111+ bool is_bech32_or_sp = (ToLower(str.substr(0, params.Bech32HRP().size())) == params.Bech32HRP()) ||
112+ dest_silent_payment_hrp == silent_payment_hrp;
aureleoules
commented at 12:52 pm on October 11, 2022:
aureleoules
commented at 7:19 am on October 12, 2022:
fb014fefff60d87cf21f220fa756aa449bd6daf4
assertions should not have side effects
also seen at src/silentpayment.cpp:148 in same commit fb014fefff60d87cf21f220fa756aa449bd6daf4
0 int r = memcmp(shared_secret, m_shared_secret, 32)
1 assert(r == 0);
Also, why did you move src/wallet/silentpayment.cpp to src/silentpayment.cpp?
aureleoules
commented at 1:17 pm on October 12, 2022:
member
Also, I think it could be nice to have a way to list all generated silent addresses with their respective label, to quickly find the payment address for an existing client for example. Such as:
I hope this feature is not too premature for this PR.
achow101 marked this as a draft
on Oct 12, 2022
DrahtBot added the label
Needs rebase
on Oct 13, 2022
w0xlt force-pushed
on Oct 15, 2022
DrahtBot removed the label
Needs rebase
on Oct 15, 2022
w0xlt force-pushed
on Oct 16, 2022
w0xlt force-pushed
on Oct 16, 2022
w0xlt force-pushed
on Oct 16, 2022
w0xlt force-pushed
on Oct 16, 2022
w0xlt
commented at 5:05 am on October 16, 2022:
contributor
@aureleoules Thanks for all the suggestions and the benchmark file. I’ve added it (and given you authorship) as it’s crucial to determine the feasibility of this scheme. I also added the ECDHPerformance_10_100 method as ECDH is the most expensive operation (which until this PR was not used by Bitcoin Core - see f66beaa534aa42341a3c3030024a13ae9078c04c).
I also implemented the listsilentaddresses RPC. Really great suggestion.
Also, why did you move src/wallet/silentpayment.cpp to src/silentpayment.cpp?
I removed silentpayment.{cpp,h} from wallet folder because it is also used in index and RPC scantxoutset (which is not related to wallet). So this was introducing a wallet dependency on these classes.
w0xlt
commented at 5:08 am on October 16, 2022:
contributor
The new scheme basically works as follows:
. If the keys used for silent payments generate a public key with odd y, then negate them when performing ECDH.
. For x-only pubkeys, add 2 in front before running ECDH (so it becomes a public key with even y).
. Tweak the original keys, not the negated ones.
@1440000bytes This way, now all CI checks are passing successfully as the secp256k1 lib is the same as the master branch.
w0xlt
commented at 8:57 pm on October 16, 2022:
contributor
I forgot to mention that any previous index of silentpayment needs to be removed as it is not compatible with the last push.
aureleoules
commented at 1:50 pm on October 19, 2022:
Encoding the identifier in a different base could increase the maximum number of identifiers when stored on 2 bytes.
base10 (current) -> 100 identifiers
base16 -> 256 identifiers
base32 -> 1024 identifiers
base58 -> 3364 identifiers
The issue with a base58 HRP is that it’s incompatible with BIP-173 and BIP-350, maybe the identifier could be encoded in the data part instead of the hrp? Or base32 is enough.
I’ve been working on this.
Some examples:
sprt1la68kq44e29c0p85rknf59qs5eww6vr3dw08arakura622ce7wcvygce0uqnw3q4 for identifier 8090879.
sprt1yvpttj5ts7z0g8dxng2ppfjua5c8z6u7068mdc8m5543nuascg33jlcpe5xc6 for identifier 35.
aureleoules
commented at 5:36 pm on October 21, 2022:
Great, how exactly is the identifier encoded in your examples?
Currently the format is: HRP + 00 (two digits) + V1 Segwit address (bech32m)
But there is no need to be a segwit address. This is because in the first version the sender could pay for the original segwit address or tweak it to the silent address, but early discussions in this PR indicated a preference on the part of reviewers for a specific address for silent payments.
So the format in the examples above is different, which is:
HRP + identifier (variable length) + compressed public key (both identifier and PK are in bech32m format)
I haven’t pushed it yet as the above format is not compatible with current code and also adds complexity to the code as it is not a valid scriptPubKey.
However, here is the code used to obtain the format above (in key_io.cpp):
The new scheme basically works as follows: . If the keys used for silent payments generate a public key with odd y, then negate them when performing ECDH. . For x-only pubkeys, add 2 in front before running ECDH (so it becomes a public key with even y). . Tweak the original keys, not the negated ones.
@1440000bytes This way, now all CI checks are passing successfully as the secp256k1 lib is the same as the master branch.
I replied to the email, but regarding the last question: the warning was suggested by @aureleoules in #24897 (comment) and the reason was a discussion in this PR about users thinking that each address would come from a different key and not the same key.
aureleoules
commented at 3:34 pm on October 25, 2022:
member
In cases where you don’t want people to know that you’re the same entity, the identifier is insufficient – you’ll need a completely separate Silent Payment address which (roughly) doubles your scanning efforts, so it’s much more costly in terms of performance.
If #25957 gets merged, would this still be an issue? If I’m not mistaken, the double-scanning efforts problem mostly apply when importing a backup of an sp descriptor or syncing the wallet when the chain is out-of-date of many blocks. Which is not often done, with #25957 the issue should be mitigated anyway?
So, would it be reasonable to implement both the identifier feature and the multi-sp-identity feature per wallet for those who wish/need full privacy?
–
Also, rescanning a sp descriptor makes bitcoind crash with the following assert:
I haven’t debugged the issue but my guess is that the scanning process encountered a taproot output with an invalid xOnlyPubKey in the block close to the timestamp 1607448174 (~2020-12-08), which should just be skipped instead of being asserted valid?
w0xlt
commented at 2:13 am on October 27, 2022:
contributor
I didn’t know it’s a valid Taproot script as the public key is invalid. The code assumes that if TxoutType whichType = Solver(vout.scriptPubKey, solutions); returned a TxoutType::WITNESS_V1_TAPROOT, both the script and the public key are valid.
But the public key in the above scriptPubKey is evidently false.
I will change this validation and release a new version with the new address format, described in #24897 (review).
Regarding #25957, I think it’s incompatible with Silent Payments, which needs to tweak the keys for each transaction. AFAIK, there is no way to combine the silent payment scheme with Golomb-coded sets.
w0xlt
commented at 2:58 am on October 27, 2022:
contributor
The following change will allow importing the descriptor.
wallet/wallet.cpp:1315
0- assert(xOnlyPubKey.IsFullyValid());
1+ if (!xOnlyPubKey.IsFullyValid()) {
2+ continue;
3+ }
It might be better to use -rpcclienttimeout=0 when importing or scanning sp descriptors.
w0xlt force-pushed
on Oct 28, 2022
w0xlt force-pushed
on Oct 28, 2022
w0xlt force-pushed
on Oct 28, 2022
w0xlt force-pushed
on Oct 28, 2022
w0xlt force-pushed
on Oct 28, 2022
w0xlt force-pushed
on Oct 29, 2022
w0xlt force-pushed
on Oct 29, 2022
w0xlt
commented at 6:00 am on October 29, 2022:
contributor
The last push proposes and implements a new address format.
The previous format was: HRP + 00 (two digits) + V1 Segwit address (bech32m)
But there is no need for a segwit script, as detailed in #24897 (review). A silent payment address only needs to expose the public key and identifier.
So the new format is:
HRP + identifier (variable length) + compressed public key (both identifier and pubkey are in bech32m format)
The reason to use the compressed public key instead of the x-only pubkey is to save an operation and simplify the code.
If x-only pubkey were used instead of compressed public key, it would be necessary to check if the private key generates a public key with odd y and negates it if so.
Exposing the prefix byte in the public key makes this unnecessary.
in
src/index/silentpaymentindex.cpp:102
in
2fe96a4241outdated
97+ const CTxIn& txin{tx->vin.at(i)};
98+
99+ auto pubkey_variant = silentpayment::ExtractPubkeyFromInput(prev_coin, txin);
100+
101+ if (std::holds_alternative<CPubKey>(pubkey_variant)) {
102+ auto pubkey = std::get<CPubKey>(pubkey_variant);
aureleoules
commented at 9:27 am on October 31, 2022:
773+ if (!sender_secret_key.IsValid()) {
774+ error = _("The private key of one of the inputs was not found.");
775+ return false;
776+ }
777+
778+ input_private_keys.push_back({sender_secret_key, is_taproot});
aureleoules
commented at 9:48 am on October 31, 2022:
in
src/wallet/wallet.cpp:1344
in
2fe96a4241outdated
1362+
1363+Coin CWallet::FindPreviousCoin(const CTxIn& txin)
1364+{
1365+ // Find the UTXO being spent in UTXO Set to learn the transaction type
1366+ std::map<COutPoint, Coin> coins;
1367+ coins[txin.prevout];
aureleoules
commented at 9:55 am on October 31, 2022:
a56b75609aafbf6ea1a9c1a280ccd3133c28d13b
is this intended?
michaelfolkson
commented at 10:49 am on October 31, 2022:
contributor
I think this is very cool work and hopefully will be added to some ecosystem wallets in future but unsure whether it should be included in the scope for the Core wallet (the Core wallet is already attempting to do a lot e.g. complex scripting support and has been subject to a number of significant re-architectures recently). I guess that decision falls to the maintainers and long term contributors of the Core wallet as if this is merged it will need to be maintained by them. Scope creep is hard to push back on especially when the work is genuinely promising and interesting but it has to be done sometimes unfortunately.
Perhaps the wallet scope discussion can happen (if it needs to happen) in an issue as to not disrupt the review of this PR.
ghost
commented at 11:01 am on October 31, 2022:
none
I think this is very cool work and hopefully will be added to some ecosystem wallets in future but unsure whether it should be included in the scope for the Core wallet (the Core wallet is already attempting to do a lot e.g. complex scripting support and has been subject to a number of significant re-architectures recently). I guess that decision falls to the maintainers and long term contributors of the Core wallet as if this is merged it will need to be maintained by them. Scope creep is hard to push back on especially when the work is genuinely promising and interesting but it has to be done sometimes unfortunately.
Perhaps the wallet scope discussion can happen (if it needs to happen) in an issue as to not disrupt the review of this PR.
This is a basic feature that should exist in every bitcoin wallet.
I appreciate the specs by @RubenSomsen, code by @w0xlt and respect all the reviewers.
Would love to see this merged and available in v25.0 so that every bitcoin core user can share their silent payment address on social media and accept payments with privacy.
w0xlt force-pushed
on Nov 1, 2022
w0xlt force-pushed
on Nov 1, 2022
w0xlt force-pushed
on Nov 1, 2022
aureleoules
commented at 2:13 pm on November 7, 2022:
member
I can no longer use sendtoaddress nor generatetoaddress to send coins to an sp address:
It used to work in 62c0c18a69559270a0a420fc48e88d50a632062c at least.
I verified that the importdescriptors issue mentioned above is fixed and can also now sync the silentpaymentindex.
w0xlt force-pushed
on Nov 10, 2022
w0xlt force-pushed
on Nov 10, 2022
w0xlt force-pushed
on Nov 10, 2022
w0xlt force-pushed
on Nov 10, 2022
w0xlt force-pushed
on Nov 10, 2022
w0xlt force-pushed
on Nov 10, 2022
w0xlt
commented at 9:46 pm on November 10, 2022:
contributor
@aureleoules Thanks for the continuous testing.
The reason sendtoaddress was not able to recognize the sp address is that this is no longer a segwit script.
Since the last push, the address format is identifier + pubkey but in the last version it has changed again as described in the comment below.
I also implemented sp support for sendmany and sendtoaddress in this version.
w0xlt
commented at 9:49 pm on November 10, 2022:
contributor
New update:
. This new version implements a new scheme, which uses two keys (scan and spend keys). This scheme allows watch-only wallets for silent payments.
. Adds support for silent payment in sendmany and sendtoaddress
. Improvements in silentpayment.{cpp,h}. This class no longer manages secp256k1_context itself and no longer interacts directly with secp256k1 library.
. Silent watch-only wallets have not yet been implemented. Some architectural improvements are needed first.
aureleoules
commented at 1:35 pm on January 17, 2023:
GetSilentPaymentKey is executed for each transaction for each block during the index creation.
But it is reading block undo data and block data for every transaction, this can be optimized by reading it only once per block in the parent function (SilentPaymentIndex::CustomAppend).
This patch speeds up the scanning by 11x (from 11 block/s to 124 block/s)
Also note that once #24230 is merged, block data (and undo data) will be available in the BlockInfo struct directly (In SilentPaymentIndex::CustomAppend) if the index is configured with additional options.
(Currently, the data and undo_data of the BlockInfo struct is unused.)
Really good catch @aureleoules !
Thanks for the all the reviews.
This improves the index considerably. Done in 1fe05939916b779c7f9111bcbaeaace03595c014.
I added you as co-author.
in
src/index/silentpaymentindex.cpp:144
in
f7bda4c695outdated
aureleoules
commented at 1:41 pm on January 17, 2023:
It may be better to execute this function for the entire block instead of executing it for every transaction, and have the for loop inside the function. This would avoid a lot of function calls.
DrahtBot removed the label
Needs rebase
on Jan 26, 2023
w0xlt force-pushed
on Jan 26, 2023
w0xlt force-pushed
on Jan 27, 2023
w0xlt force-pushed
on Jan 28, 2023
DrahtBot added the label
Needs rebase
on Jan 28, 2023
w0xlt force-pushed
on Jan 29, 2023
DrahtBot removed the label
Needs rebase
on Jan 29, 2023
w0xlt force-pushed
on Jan 29, 2023
w0xlt force-pushed
on Jan 29, 2023
w0xlt force-pushed
on Jan 29, 2023
w0xlt
commented at 9:29 am on January 29, 2023:
contributor
Rebased and all the issues identified by SonarCloud in #26865 (comment) have been fixed.
Great tool @aureleoules
w0xlt force-pushed
on Feb 1, 2023
w0xlt force-pushed
on Feb 1, 2023
w0xlt force-pushed
on Feb 2, 2023
w0xlt force-pushed
on Feb 7, 2023
w0xlt force-pushed
on Feb 7, 2023
w0xlt force-pushed
on Feb 14, 2023
w0xlt force-pushed
on Feb 14, 2023
w0xlt force-pushed
on Feb 14, 2023
w0xlt force-pushed
on Feb 14, 2023
w0xlt force-pushed
on Feb 14, 2023
w0xlt
commented at 5:26 am on February 14, 2023:
contributor
Update: The silentpayment index has been removed from the codebase.
scantxoutset has been changed to use txindex and CBlockUndo. So the scan works without silentpayment index.
getsilentpaymentblockdata also works without any indexes, using only CBlockUndo. The code is relatively simple. Therefore, even serving light clients, there is no need for indexing.
Seems to work fine. Not sure about the performance.
A new PR, with the index, will be added soon.
DrahtBot added the label
Needs rebase
on Feb 17, 2023
w0xlt force-pushed
on Feb 19, 2023
DrahtBot removed the label
Needs rebase
on Feb 19, 2023
w0xlt force-pushed
on Feb 23, 2023
w0xlt force-pushed
on Feb 23, 2023
w0xlt force-pushed
on Feb 25, 2023
DrahtBot added the label
Needs rebase
on Mar 7, 2023
DrahtBot
commented at 1:08 am on March 7, 2023:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
3742cc75571a60bda52a41923c8244535af9ff23: maybe move that above UNKNOWN?
Sjors
commented at 3:47 pm on April 7, 2023:
member
Don’t forget to rebase. The bech32 hrp stuff moved to kernel/chainparams.{h,cpp}.
The silentpayment index has been removed from the codebase. … A new PR, with the index, will be added soon.
That’s nice. It’s more important to prove functionality, can improve performance later.
The rawtr() functionality has been merged, so the dependency can be removed from the PR description. Either this PR description or a BIP should remind the reader why rawtr() is necessary (you can’t know the untweaked key) and safe (there’s no way for the sender to unilaterally add a hidden script path?).
Enabling the ECDH module in libsecp256k1 might be a reason to put silent payments behind a configure flag initially. Then again, we also need it for BIP324 P2P encryption #23561 and I’m not seeing any discussion there about enabling the module either. That PR also introduces fuzz tests, so maybe it’s worth combining efforts and getting those merged jointly.
Most of the commits here are reasonably short. It might be worth splitting f0f9817f44fb8dc43c1f8ca0a456fcd3780350af a bit further.
I would suggest moving PSBT and light client support to a followup PR. It’s important to prove that it works, but that can be done by just keeping it rebased on this PR with the minimum functionality.
in
src/wallet/rpc/backup.cpp:1501
in
9143c7e1f1outdated
1494@@ -1495,6 +1495,11 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
1495 throw JSONRPCError(RPC_INVALID_PARAMETER, "Active descriptors must be ranged");
1496 }
14971498+ if (isSP && data.exists("next_index"))
9143c7e1f1710f123072cc272b66ab2cad0eb57a: this commit won’t compile, probably needs something from a later commit
in
src/wallet/rpc/addresses.cpp:138
in
ee652ca59aoutdated
133+
134+ ret.pushKV("address", address);
135+ ret.pushKV("label", label);
136+ ret.pushKV("identifier", identifier);
137+ if (identifier > 0) {
138+ ret.pushKV("warnings", "This address is not a new identity. It is a re-use of an existing identity with a different label.");
The term “identity” is confusing here. It’s also confusing that you get this error when you generate a new silent payment address, but not when you request an existing one (by reusing a label).
I guess what you’re trying to say here is that an outside observer can tell that two silent payment addresses belong to the same person? Maybe just say that:
The silent payment addresses are unique for accounting purposes only. They can be linked to the same wallet. For privacy a new wallet is required.
That said, I think this feature is a footgun. Also because poor coin control will eventually link funds from all labels.
Sjors
commented at 6:57 pm on April 7, 2023:
member
Silent watch-only wallets have not yet been implemented. Some architectural improvements are needed first.
This would be nice though, maybe in a followup. I was trying to test it with bitcoin-cli -signet -named createwallet wallet_name=SilentWatcher silent_payment=true disable_private_keys=true and then import a descriptor like sp(…).
I guess one thing you need for that is the ability to dump the watch key. You could add an argument scan_privkey to decodesilentaddress which would return the private, rather the public. Ideally in the form of a sp(…) descriptor ready for import (and a warning that leaking that descriptor hurts your privacy, though coins can’t be stolen)
Finally I noticed that calling listdescriptors true on the wallet reveals only 1 private key for the sp() descriptor; I guess because they are derived from each other?
conf: add EDCH moduleb6eacc3899
crypto: Add necessary new functions to the silent payments schemeebf4e8e591
add Silent Payment Scheme0a19978486
script: create a new output type `OutputType::SILENT_PAYMENT`d4774d5c01
script: add `sp()` descriptor3bb18d4dbf
wallet: add Silent Payment flage886d3abc6
script: add SP address generation02ae78ee62
wallet: enable SP option in the `createwallet`
Co-authored-by: Aurèle Oulès <aurele@oules.com>
a967b2103e
wallet: add function to create silent transaction271e78b5e6
rpc: change scantxoutset to scan silent paymentsd499a31b4c
Add silent tx function to `walletcreatefundedpsbt`8600b767f6
wallet: add silent tx function to `sendmany` and `sendtoaddress`0980159996
wallet: add `listsilentaddresses()` RPC5d2562aa67
rpc: Add `getsilentpaymentblockdata` for light client903eaf2538
test: add functional test for Silent Payments92d69ee941
bench: add benchmark for silent payments50a6320e57
w0xlt force-pushed
on Apr 14, 2023
josibake
commented at 1:15 pm on June 5, 2023:
member
I’ve rebased and created a slimmed-down version of this PR in #27827. Most notably, I took out labels and removed some of the RPC support, and made several updates as the silent payments spec has changed a bit since this PR was last updated. For anyone reviewing this PR, I’d appreciate your feedback on #27827.
Big thanks to @w0xlt for moving this draft along as far as they did! My plan is to keep cherry-picking commits from this draft for follow-up PRs to add back in label support, RPC coverage, etc
fanquake
commented at 1:18 pm on June 5, 2023:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-04-01 03:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
