spesmilo
Feature: Add Silent Payment sending support
This PR introduces support for sending silent payments (BIP-0352), integrated into the desktop Qt interface.
Key Notes
- Silent payments can be sent from standard wallets with BIP32 keystores only. This ensures all UTXOs in the wallet are eligible for the shared secret derivation. Legacy or imported wallets may include uncompressed keys (for P2PKH), which are not compatible.
- Invoices involving silent payments are not persisted, as Electrum’s current system tracks invoice status via the scriptpubkeys of the invoice outputs. This approach doesn’t work for silent payments, since their final scriptpubkeys are only known at the point of transaction creation, not at invoice creation. Supporting this would have required intrusive changes to Electrum’s code base, with little practical benefit.
- ~RBF is disabled for silent payment transactions to prevent potential fund loss. If a user shares the same seed across two devices, broadcasts the transaction from one, and later bumps the fee on the other with added or changed inputs, the shared secret used in silent payments is broken, causing the recipient to miss the payment. If a transaction gets stuck in the mempool, users can still use CPFP as an alternative.~ RBF is allowed for silent payment transactions only when the fee bump is performed by the same wallet instance that created the transaction.
According to updated BIP21 specification silent payment addresses should be put in a sp query attribute. I don't see that supported in this PR.
In fact, it is even disallowed to put a silent payment address in the BIP21 uri root:
The bitcoinaddress body MUST be either a base58 P2SH or P2PKH address, bech32 Segwit version 0 address, bech32m Segwit address, or empty. Future address formats SHOULD instead be placed in query keys as optional payment instructions to provide backwards compatibility during upgrade cycles. The bitcoinaddress part of the URI MAY be left empty if there is at least one payment instruction provided in a query parameter, allowing for recipients wishing to avoid a standard on-chain fallback.
@accumulator Yes thanks! I completely missed that bip21 had specifications for bip352. I addressed this in commit b1d4144. It was more complicated than I thought, especially dealing with the absence of fallback addresses in wallets that can't send silent payments. For example 'bitcoin:?sp=validspaddr' is a valid bip32-URI and therefore a valid payment identifier imo, but it is unusable in non-silent-payment-wallets. I saw no other way but to inject wallet-silent-payment-capability into the relevant functions inside payment_identifier.py. This could be simplified by marking the payment identifier as invalid during BIP21 URI parsing, but that would be misleading — the URI itself is valid, even if the current wallet cannot process it.
@accumulator Yes thanks! I completely missed that bip21 had specifications for bip352. I addressed this in commit b1d4144. It was more complicated than I thought, especially dealing with the absence of fallback addresses in wallets that can't send silent payments. For example 'bitcoin:?sp=validspaddr' is a valid bip32-URI and therefore a valid payment identifier imo, but it is unusable in non-silent-payment-wallets. I saw no other way but to inject wallet-silent-payment-capability into the relevant functions inside payment_identifier.py. This could be simplified by marking the payment identifier as invalid during BIP21 URI parsing, but that would be misleading — the URI itself is valid, even if the current wallet cannot process it.
Yes, we currently do the same for lightning-only payment identifiers; The PI is valid, even if the wallet does not support lightning.
This leads to silent payment specific parameters and code in quite a few places.. I've added some review comments.
Have you considered OpenAlias resolving to a silent payment address? This would seem like an ideal combination..
https://github.com/MorenoProg/electrum/blob/1fc7ee5861d25c687400c88247a86e4c3ec5bc46/electrum/payment_identifier.py#L345-L358
Getting an exception when address_synchronizer emits remove_transaction (e.g. replaced by fee tx)
Traceback (most recent call last):
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1206, in wrapper
return await func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/electrum/interface.py", line 644, in wrapper_func
return await func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/electrum/interface.py", line 670, in run
await self.open_session(ssl_context=ssl_context)
File "/home/sander/projects/electrum/electrum/electrum/interface.py", line 840, in open_session
async with self.taskgroup as group:
^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/packages/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1381, in join
task.result()
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1511, in run_tasks_wrapper
await self._run_tasks(taskgroup=taskgroup)
File "/home/sander/projects/electrum/electrum/electrum/synchronizer.py", line 79, in _run_tasks
async with taskgroup as group:
^^^^^^^^^
File "/home/sander/projects/electrum/electrum/packages/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1381, in join
task.result()
File "/home/sander/projects/electrum/electrum/electrum/synchronizer.py", line 203, in _on_address_status
await self._request_missing_txs(hist)
File "/home/sander/projects/electrum/electrum/electrum/synchronizer.py", line 222, in _request_missing_txs
async with OldTaskGroup() as group:
^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/packages/aiorpcx/curio.py", line 304, in __aexit__
await self.join()
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1381, in join
task.result()
File "/home/sander/projects/electrum/electrum/electrum/synchronizer.py", line 244, in _get_transaction
self.adb.receive_tx_callback(tx)
File "/home/sander/projects/electrum/electrum/electrum/util.py", line 1235, in func_wrapper
return func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/electrum/address_synchronizer.py", line 440, in receive_tx_callback
self.add_transaction(tx, allow_unrelated=True)
File "/home/sander/projects/electrum/electrum/electrum/address_synchronizer.py", line 325, in add_transaction
self.remove_transaction(tx_hash2)
File "/home/sander/projects/electrum/electrum/electrum/address_synchronizer.py", line 379, in remove_transaction
self._remove_transaction(txid)
File "/home/sander/projects/electrum/electrum/electrum/address_synchronizer.py", line 420, in _remove_transaction
self.db.remove_silent_payment_address(txo.address)
File "/home/sander/projects/electrum/electrum/electrum/json_db.py", line 42, in wrapper
return func(self, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/sander/projects/electrum/electrum/electrum/wallet_db.py", line 1484, in remove_silent_payment_address
assert isinstance(onchain_addr, str)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError
@accumulator Oh.. yes, this is very likely because remove_silent_payment_address is called with txo.address as argument, which of course is None when there is a non-address scriptpubkey. Then the assertion makes no sense... I haven't thought about this, thx! I will get to it and your other comments tonight.
Have you considered OpenAlias resolving to a silent payment address? This would seem like an ideal combination..
https://github.com/MorenoProg/electrum/blob/1fc7ee5861d25c687400c88247a86e4c3ec5bc46/electrum/payment_identifier.py#L345-L358
I looked into it. Do you think the silent payment address can just be placed into the recipient_address -field? I haven't found any specs about handling different address types (like in bip21), all I found was this on the official OpenAlias Website:
As this is an open standard that is meant to be extensible, defining additional pairs is up to you. Your client-side application may require certain key-value pairs as a minimum, and you should make that information easily available. If you have a use-case where you feel certain key-value pairs may provide widespread use and benefit, please reach out to us and we can include it in the standard if appropriate.
If we just place the sp_address into the recipient_address -field it would be quite easy to implement I guess. I checked the code, and only a few places would need adjustments. That said, there's a 'TODO' note about refactoring in one of the relevant sections, so I waited with making any changes for now.
https://github.com/MorenoProg/electrum/blob/1fc7ee5861d25c687400c88247a86e4c3ec5bc46/electrum/payment_identifier.py#L687-L694
Have you considered OpenAlias resolving to a silent payment address? This would seem like an ideal combination.. https://github.com/MorenoProg/electrum/blob/1fc7ee5861d25c687400c88247a86e4c3ec5bc46/electrum/payment_identifier.py#L345-L358
I looked into it. Do you think the silent payment address can just be placed into the
recipient_address-field? I haven't found any specs about handling different address types (like in bip21), all I found was this on the official OpenAlias Website:As this is an open standard that is meant to be extensible, defining additional pairs is up to you. Your client-side application may require certain key-value pairs as a minimum, and you should make that information easily available. If you have a use-case where you feel certain key-value pairs may provide widespread use and benefit, please reach out to us and we can include it in the standard if appropriate.
The unreleased OA2 spec is a bit more formalized than OA1, and even explicitly mentions BIP352. However we only implement OA1 currently.
If we just place the sp_address into the
recipient_address-field it would be quite easy to implement I guess.
I think that would indeed be sufficient.
I checked the code, and only a few places would need adjustments. That said, there's a 'TODO' note about refactoring in one of the relevant sections, so I waited with making any changes for now.
That TODO is orthogonal to this change, and there's currently no PR or branch changing this code, so feel free to make changes there.
@accumulator I added openalias recognizing sp_addresses in 0c5e83d. It was almost nothing to change, which makes me think I missed something. I tested it by mocking the call to the openalias-server in resolve_openalias in contacts.py:
@classmethod
async def resolve_openalias(cls, url: str) -> Dict[str, Any]:
if url == "[email protected]":
await asyncio.sleep(1)
return {
'address': "valid_sp",
'name': "Silent Payment Test",
'type': 'openalias',
'validated': True
}
This worked fine, also in non-sp-wallets.
It was almost nothing to change, which makes me think I missed something.
No, you didn't miss anything, that's sufficient.
You can even remove https://github.com/MorenoProg/electrum/blob/0c5e83d3af7a8a69ed32ad28ab6b923886950a57/electrum/payment_identifier.py#L701-L703 as that is stale code
There are still two design points where feedback is appreciated:
-
Password prompt: Since creating a silent payment requires access to the input private keys, the user is currently prompted for the password before building the transaction. They’ll get prompted again when signing. To avoid this double prompt, we could pass the password to confirm_tx_dialog (and also to transaction_dialog for previews), so it's reused. Would that be ok?
-
Disabling export/save for PSBTs involving silent payments: If a user exports a PSBT with silent payment outputs and edits it on another device, funds could be lost. We could block PSBT export in that case (informing why it's disabled), and only allow exporting
.txnfiles where the transaction is already finalized. I’m aware that BIP375 addresses the issue of PSBTs involving silent payments. However, since the BIP is still in progress (open TODOs), and users who don’t interact with PSBTs aren’t affected, I think integrating this BIP can be deferred to a later stage.
EDIT: Both concerns addressed and changes implemented in 8b39e15.
Tested ACK - tests/test_silent_payment.py
Please consider implementing sending test vectors as well Additional References:
Big thank you to everyone that has reviewed this PR to date.
Gentle reminder @accumulator to review requested changes.
@macgyver13 Thanks for testing. I am using a slightly modified version of the test vectors, in which the mapping of sp_address -> taproot_outputs is preserved. Also, I cleaned the test file to remove irrelevant cases like receiving or taproot inputs.
I missed the implementation of those test cases 😢. Perhaps printing each subtest would help future reviewers.
The attached patch has the following output:
collected 15 items
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Simple_send_two_inputs] PASSED [ 6%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Simple_send_two_inputs,_order_reversed] PASSED [ 13%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Simple_send_two_inputs_from_the_same_transaction] PASSED [ 20%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Simple_send_two_inputs_from_the_same_transaction,_order_reversed] PASSED [ 26%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Outpoint_ordering_byte-lexicographically_vs._vout-integer] PASSED [ 33%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Single_recipient_multiple_UTXOs_from_the_same_public_key] PASSED [ 40%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Multiple_outputs_multiple_outputs,_same_recipient] PASSED [ 46%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Multiple_outputs_multiple_outputs,_multiple_recipients] PASSED [ 53%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Multiple_outputs_with_labels_un-labeled_and_labeled_address;_same_recipient] PASSED [ 60%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Multiple_outputs_with_labels_multiple_outputs_for_labeled_address;_same_recipient] PASSED [ 66%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_silent_payment_create_outputs[Multiple_outputs_with_labels_un-labeled,_labeled,_and_multiple_outputs_for_labeled_address;_same_recipients] PASSED [ 73%]
tests/test_silent_payment.py::TestSilentPaymentCreateOutputs::test_private_keys_sum_to_zero PASSED [ 80%]
tests/test_silent_payment.py::TestSilentPaymentParseAddress::test_malformed_silent_payment_address PASSED [ 86%]
tests/test_silent_payment.py::TestSilentPaymentParseAddress::test_silent_payment_parse_address_success PASSED [ 93%]
tests/test_silent_payment.py::TestSilentPaymentTxCreation::test_merge_duplicate_tx_outputs PASSED [100%]
There are still two design points where feedback is appreciated:
1. Password prompt: Since creating a silent payment requires access to the input private keys, the user is currently prompted for the password before building the transaction. They’ll get prompted again when signing. To avoid this double prompt, we could pass the password to confirm_tx_dialog (and also to transaction_dialog for previews), so it's reused. Would that be ok?
Yes, that would be better. The current branch immediately signs the transaction as soon as the dialog is opened, which is different behavior than normal payments, and might be confusing.
2. Disabling export/save for PSBTs involving silent payments: If a user exports a PSBT with silent payment outputs and edits it on another device, funds could be lost. We could block PSBT export in that case (informing why it's disabled), and only allow exporting `.txn` files where the transaction is already finalized. I’m aware that BIP375 addresses the issue of PSBTs involving silent payments. However, since the BIP is still in progress (open TODOs), and users who don’t interact with PSBTs aren’t affected, I think integrating this BIP can be deferred to a later stage.EDIT: Both concerns addressed and changes implemented in 8b39e15.
Instead of passing show_combine_menu from outside TxDialog it's better to encapsulate it within the dialog, to avoid accidentily enabling these options when called from other locations (e.g. from the history tab)
See my commit https://github.com/accumulator/electrum/commit/7334e77ab3069fbe172293e19cf148209cc5aacb
I think what's lacking still is a check to verify a Tx for correctness in the context of silent payments in Abstract_Wallet.sign_transaction. If for whatever reason a Tx is messing with its inputs right before signing, this has the potential to send funds into the void. Currently there is only a SIGHASH check.
Yes, that would be better. The current branch immediately signs the transaction as soon as the dialog is opened, which is different behavior than normal payments, and might be confusing.
Agreed. I addressed this in ab8486a. I re-added the sign button (disabled) and added a tool tip that informs the user about the automatic signing. My motivation for immediate signing is to never expose an incomplete sp-transaction to the user (not even for adding to history).
Instead of passing
show_combine_menufrom outsideTxDialogit's better to encapsulate it within the dialog, to avoid accidentily enabling these options when called from other locations (e.g. from the history tab)
Applied.
I think what's lacking still is a check to verify a
Txfor correctness in the context of silent payments inAbstract_Wallet.sign_transaction. If for whatever reason aTxis messing with its inputs right before signing, this has the potential to send funds into the void. Currently there is only aSIGHASHcheck.
@accumulator I added an integrity check in 5486dd0. Right after tx-creation all the relevant silent payment elements (inputs and sp-outputs) are hashed and bound to the tx. This binding can happen only once. The integrity hash is verified against the actual transaction at the time of signing to ensure it hasn't been tampered with. Actually, we could be even more careful and also add this check again when broadcasting...
I propose to consider this PR for merging, as rebases are hard to re-review with this large delta, and the PR IMO looks pretty solid. If there's still remaining doubts w.r.t. loss of coins, we could add an 'experimental feature' warning when pasting/scanning a silent payment identifier.
Just in case @accumulator's comment has gone unnoticed, tagging @ecdsa and @SomberNight.
@MorenoProg I am not keen to merge this feature into master.
At the moment, there does not seem to be a realistic way to implement both sending and receiving silent payments in a light client. This implies that silent payments will probably remain a fringe feature, not something that will see mass adoption. However, this feature also comes at a significant cost, in terms of complexity and maintenance burden for developers. It touches a bunch of files, it has its own logic, and it requires new developers to understand that logic in order not to break it.
We recently opened Electrum to third-party plugins. Why not add this as a plugin instead?
At the moment, there does not seem to be a realistic way to implement both sending and receiving silent payments in a light client.
@ecdsa Just last week, a PoC, Frigate, was released that demonstrates a practical path to receiving silent payments in light clients. It uses an optimized tweak index and extends the Electrum protocol with methods that let a light client subscribe to Silent Payment addresses, similar to normal addresses. With this, a light client can also recover from seed.
This implies that silent payments will probably remain a fringe feature, not something that will see mass adoption.
Mass adoption remains to be seen, but there is quite some WIP across the silent payment ecosystem.
We recently opened Electrum to third-party plugins. Why not add this as a plugin instead?
Your concerns listed in opening issue #9728 affect receiving only and you would prefer the receiving part as a plugin. Since sending and receiving are independent, it was my understanding that sending should belong in the core codebase.
I am suggesting that txs that pay to a silent payment addr, should not signal RBF... New versions of Electrum however could sometimes decide to ignore the signalling
An observation here: any onchain wallet that ignores RBF signalling (due to the prevalence of mempoolfullrbf=1) risks ~burning user funds should the original transaction have included a silent payments derived address. This applies regardless of whether silent payments support has been added to the wallet, since the original transaction may have been created in a wallet that does support it.
I am suggesting that txs that pay to a silent payment addr, should not signal RBF... New versions of Electrum however could sometimes decide to ignore the signalling
An observation here: any onchain wallet that ignores RBF signalling (due to the prevalence of
mempoolfullrbf=1) risks ~burning user funds should the original transaction have included a silent payments derived address. This applies regardless of whether silent payments support has been added to the wallet, since the original transaction may have been created in a wallet that does support it.
Yes. My idea was that a wallet could decide to allow RBF-ing a non-signalling tx if that tx was originally created by that wallet. In that restricted setting your concern does not apply.
However if there is a wallet out there that allows unrestricted RBF, even for txs not created by itself, that can result in burning coins, yes. This seems to be an inherent design issue with silent payments. E.g. it is not so safe anymore to move your seed between wallets.
