silent payments: integrate SPDK scanning and wallet IPC#50
Conversation
|
Thanks for adding this. I'm surprised how much code there is still even with including SPDK. Although, I guess that's due to adding supporting wallet code. I'd also prefer that SPDK used rust-bitcoin-coin-selection since it seems more robust and has more supported selection algos, although my opinion is maybe a bit biased :P |
|
Latest commits incorporate the most recent code reviews. Thanks! |
|
@pzafonte , im curious what you think about using SPDK now that you've completed PRs both with and without. Did you feel like the SPDK saved you a lot of work and effort vs the first PR you made? Im somewhat skeptical because both PRs seem to be similar in size. |
btw, this is still in Draft. Move it out of draft if you think it's a merge candidate. |
This was definitely much easier. W.R.T. size, it looks that way since I pushed up the spdk changes on the previous PR. Comparing just my original code (no test vectors), this PR is roughly ~1/3 the size. Much more maintainable, and we don't have to own the cryptography layer in this repo. |
However, I did notice there is a disclaimer we should keep in mind on the SPDK implementation:
|
This will likely be delegated to |
| shutdown_tx: mpsc::Sender<()>, | ||
| tip_state: &Arc<Mutex<TipState>>, | ||
| wallet_state: WalletState, | ||
| chainman_holder: Arc<std::sync::OnceLock<Arc<ChainstateManager>>>, |
There was a problem hiding this comment.
I had to remind myself of what OnceLock was. I'm not sure this is a great way to proceed. Have we tried using a std::sync::mpsc::channel? We can spawn a new thread that waits for blocks to scan when this callback is hit. So instead of scan_kernel_block here, we send the block over the channel to a consuming thread.
There was a problem hiding this comment.
I tried using the channel, and replaced OnceLock with a consumer thread that looks up the entry via chainman.get_block_tree_entry(hash) after the callback returns. A round trip test showed get_block_tree_entry returns None for just-connected blocks, even from a separate thread, so the entry isn't available at this stage. read_spent_outputs(entry) has to happen synchronously where the entry is alive. Since the callback is registered before ChainstateManager exists, we need some kind of slot for chainman or if the kernel callback delivered spent outputs alongside the block that might work, I think. Open to alternatives if you can think of one, but OnceLock seems structurally required given the existing state of things.
There was a problem hiding this comment.
As in we call send with the block and entry within with_block_connected_validation and the undo data is not present on the receiving thread? That seems strange if it is available within the callback.
There was a problem hiding this comment.
Unless I'm missing something, to get the undo data you call chainman.read_spent_outputs(entry), and the receiving thread doesn't have a reference to chainman.
There was a problem hiding this comment.
The receiving thread can clone the Arc cheaply once it starts up. I will push up a branch as a demo for what I'm thinking tomorrow. Other review can be addressed in the meantime.
There was a problem hiding this comment.
So far I think you're right. Brutal. The self-referential nature of this architecture bothers me a lot but I think this is the only way to proceed. Confusing why the method returns None when it should return Some, at least at first glance.
| } | ||
| }; | ||
|
|
||
| let btc_block = kernel_block_to_bitcoin_block(kernel_block); |
There was a problem hiding this comment.
I think this is leaking some wallet logic to the node module, which we would like to keep as tidy as possible. It seems like we can refactor scan_block of the wallet to take kernel::Block and kernel spent outputs to move the entirety of the scanning logic to the wallet module.
|
@pzafonte do you mind if we pair program the rest, i.e. I force-push this branch? You'll still get commit attribution, just eager to move this along while I have the time. |
I don't mind that, I just pushed up the latest changes that address the existing review. |
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct OwnedUtxo { |
There was a problem hiding this comment.
For each coin we will need the following:
ScriptPubKeyOutPoint- A scalar representing the "tweak", should be returned at some point by
scan_transaction AmountLabelfor change
There was a problem hiding this comment.
Only the receiving side of silent payments is being added here, right? Isn't this struct something that should be provided by the SPDK framework?
There was a problem hiding this comment.
fwiw, SPDK exposes the following for Receiver:
/// A struct representing a silent payment recipient.
///
/// It can be used to scan for transaction outputs belonging to us by using the [`scan_transaction`](Receiver::scan_transaction) function.
/// It optionally supports labels, which it manages internally.
/// Labels can be added with [`add_label`](Receiver::add_label).
#[derive(Debug, Clone, PartialEq)]
pub struct Receiver {
version: u8,
scan_pubkey: PublicKey,
spend_pubkey: PublicKey,
change_label: Label, // To be able to tell which label is the change
labels: BiMap<Label, PublicKey>,
pub network: Network,
}
| }; | ||
| use secp256k1::{Scalar, SecretKey, XOnlyPublicKey}; | ||
|
|
||
| pub struct InputData { |
There was a problem hiding this comment.
There was a problem hiding this comment.
SPDK doesn't use bitcoin types so the conversion is unnecessary. That being said I think we can clean up the number of types introduced in this PR.
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct FoundUtxo { |
There was a problem hiding this comment.
I believe this is TxOut with an added tweak and label: https://github.com/rust-bitcoin/rust-bitcoin/blob/0.32.x/bitcoin/src/blockdata/transaction.rs#L574
|
High level commit structure has good separation of concerns. I brainstormed a bit yesterday about final clean up for this PR if possible, here is my architectural feedback: The distinction between It appears to me that we can also roll We should probably try to avoid converting to a
We should also early-return in the callback functions of |
|
Looking good. Does SPDK have tooling for generating the address? I'd like to give this a final smoke test by sending some signet coins. |
https://github.com/cygnet3/spdk/blob/master/silentpayments/examples/create_wallet.rs |
It has |
|
I would like to develop a more thoughtful approach of how we do testing here, likely taking some inspiration from Floresta. In the meantime, let's add a final method |
Add a workspace crate implementing BIP-352 silent payment scanning. Provides Wallet (UTXO tracking, balance, history), scan_transaction for detecting incoming payments, and build_receiver for constructing a Receiver from scan/spend keys.
Add scan_kernel_block registered via with_block_connected_validation so undo data is tied to the block being scanned. OnceLock provides ChainstateManager to the callback after construction. Expose wallet state over IPC via a makeWallet sub-client on the existing Server interface, with Receiver cached on import-keys.
…nce, and history Add receive generate-keys, import-keys, balance, and history subcommands under a wallet sub-command group. The IPC-bound subcommands reach the node via a single node.sock connection, calling makeWallet to obtain the wallet sub-client capability. generate-keys runs client-side only, generating keys from OsRng via bitcoin's rand-std feature.
|
Epic. I tested the flow and was able to receive a payment on signet. I have some follow-ups in mind to clean this up and potentially make it more performant, but this is a good start. ACK fac0921 |
|
Nice! :D |
New PR using SPDK... Integrate BIP-352 silent payment scanning into kernel-node using the
silentpaymentscrate from SPDK.crates/walletas a workspace member with asilentpaymentssubmodule that wraps SPDK's cryptographic primitivesimport_keys,balance, andhistoryover the existingnode.sock