feat: receive payjoin #445
Conversation
I think this is the right approach - get something running, figure out the boilerplate and perhaps e2e testing and we can talk about the details of the integration later. Its always easier to move code around / refactor to the right design with a working skeleton than on a blank slate. |
There was a problem hiding this comment.
This skeleton is getting closer. It attempts to run a payjoin v1 server coupled to the bria api server. I imagine in prod you have a very specific access architecture in mind, which this prototype naturally ignores.
Since this is an early prototype it does have excessive imports, probably unnecessary clones, and dirty access patterns. Errors are handled but will definitely need to be combed over at some point.
One major confusion sticks out. Payjoin needs to be able to interface with some 'wallet' but I'm not sure which one(s) bria makes available. I'm ignorant to the profile / keychain abstraction that needs to be navigated in order to contribute utxos to payjoins. Does each blink client have a bria-custodies Profile? Or do they interface with a large counterparty Profile which would have access to all payjoin funds? Or both? If my framing sounds uncertain it is because it is.
I'm also still missing a few components of the Bria api necessary to get a skeleton working which I address directly in self review comments below
src/payjoin/app.rs
Outdated
| let proposal = proposal.check_broadcast_suitability(None, |tx| { | ||
| let _raw_tx = bitcoin::consensus::encode::serialize_hex(&tx); | ||
| // TODO test_mempool_accept e.g.: | ||
| // | ||
| // let mempool_results = | ||
| // bitcoind.test_mempool_accept(&[raw_tx]).map_err(|e| Error::Server(e.into()))?; | ||
| // match mempool_results.first() { | ||
| // Some(result) => Ok(result.allowed), | ||
| // None => Err(Error::Server( | ||
| // anyhow!("No mempool results returned on broadcast check").into(), | ||
| // )), | ||
| // } | ||
| Ok(true) | ||
| })?; | ||
| tracing::trace!("check1"); |
There was a problem hiding this comment.
What's the most straightforward way to access a testmempoolaccept function like bitcoind's?
There was a problem hiding this comment.
Since bria uses Fulcrum and Fulcrum does not have such a function, we'll need to devise a new way to check mempool acceptance so that we know the original_psbt proposal is acceptable fallback collateral if the sender disappears to prevent probing attacks.
There was a problem hiding this comment.
I’m okay adding a dependency to bitcoind for this as we want to eventually get rid of fulcrum anyhow.
src/payjoin/app.rs
Outdated
| // Create a channel | ||
| let (tx, rx) = mpsc::channel::<(ScriptBuf, Sender<Result<bool, ApplicationError>>)>(); | ||
| let tx = Arc::new(Mutex::new(tx)); | ||
|
|
||
| let app = self.app.clone(); | ||
| let profile = self.profile.clone(); | ||
| let network = network.clone(); | ||
|
|
||
| // Receive Check 2: receiver can't sign for proposal inputs | ||
| tokio::spawn(async move { | ||
| while let Ok((input, response_tx)) = rx.recv() { | ||
| let result = check_script_owned(&app, &profile, network, &input).await; | ||
| let _ = response_tx.send(result); | ||
| } | ||
| }); | ||
|
|
||
| async fn check_script_owned( | ||
| app: &App, | ||
| profile: &Profile, | ||
| network: bitcoin::Network, | ||
| input: &Script, | ||
| ) -> Result<bool, ApplicationError> { | ||
| let address = bitcoin::BdkAddress::from_script(&input, network)?; | ||
| match app | ||
| .find_address(&profile.clone(), address.to_string()) | ||
| .await | ||
| { | ||
| Err(ApplicationError::AddressError(e)) => Err(ApplicationError::AddressError(e)), | ||
| Err(_) => Ok(false), // good address script, but not found | ||
| Ok(_) => Ok(true), | ||
| } | ||
| } | ||
|
|
||
| let proposal = proposal.check_inputs_not_owned(|input| { | ||
| let (response_tx, response_rx) = mpsc::channel::<Result<bool, ApplicationError>>(); | ||
| { | ||
| let tx = tx.lock().expect("lock"); // TODO Handle lock error if needed | ||
| tx.send((input.to_owned(), response_tx)) | ||
| .map_err(|e| Error::Server(e.into()))?; | ||
| } | ||
| let recv_res = response_rx.recv().map_err(|e| Error::Server(e.into()))?; | ||
| Ok(recv_res.map_err(|e| Error::Server(e.into()))?) | ||
| })?; |
There was a problem hiding this comment.
This hack looks ugly but is necessary with the current payjoin library since proposal.check_* functions take closures, async closures are unstable, and the payjoin app needs to make async db calls for various checks if e.g. a particular address is ours or not in this case.
Open to suggestions for improvements here in either implementation approach or upstream api.
There was a problem hiding this comment.
The most straightforward solution is to block_on a tokio::runtime::Handle
Justin and I explored async_trait too which will expand the scope of the library. I'd like to avoid that for now.
// Receive Check 2: receiver can't sign for proposal inputs
let proposal = proposal.check_inputs_not_owned(|input| {
self.rt.block_on(async {
let address = bitcoin::BdkAddress::from_script(&input, network)
.map_err(|e| Error::Server(e.into()))?;
match self.addresses.find_address(address.to_string()).await {
Ok(_) => Ok(true),
Err(AddressError::AddressNotFound(_)) => Ok(false),
Err(e) => Err(Error::Server(e.into())),
}
})
})?;
src/payjoin/app.rs
Outdated
| let payjoin = proposal.check_no_inputs_seen_before(|input| { | ||
| // TODO implement input_seen_before database check | ||
| // Ok(!self.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into()))?) | ||
| Ok(false) | ||
| })?; | ||
| tracing::trace!("check4"); |
There was a problem hiding this comment.
Since clients can POST payjoin requests which return proposal_psbt including bria utxos, they should be limited with this check. If an adversary were to submit many requests and never sign the proposal_psbt then they would be able to see many utxos within bria's wallets. This is mitigated with this check to only reply with bria utxos for request inputs the first time they are seen and also by broadcasting original_psbt included in the POST request after some timeout if the proposal_psbt does is not signed and broadcast.
There was a problem hiding this comment.
This will require a new repo that keeps track of seen inputs
There was a problem hiding this comment.
Anything Utxo related (selection, filter, tagging) should be put in the utxo module. There is already an entity for utxo.
src/payjoin/app.rs
Outdated
| // Don't throw an error. Continue optimistic process even if we can't contribute inputs. | ||
| self.try_contributing_inputs(&mut provisional_payjoin) | ||
| .await | ||
| .map_err(|e| tracing::warn!("Failed to contribute inputs: {}", e)); | ||
|
|
||
| // Output substitution could go here | ||
|
|
||
| let payjoin_proposal = provisional_payjoin.finalize_proposal( | ||
| |psbt: &bitcoin::psbt::Psbt| { | ||
| Err(Error::Server(anyhow!("TODO sign psbt").into())) | ||
| // TODO sign proposal psbt with our inputs & subbed outputs e.g.: | ||
| // | ||
| // bitcoind | ||
| // .wallet_process_psbt(&base64::encode(psbt.serialize()), None, None, Some(false)) | ||
| // .map(|res| bitcoin::psbt::Psbt::from_str(&res.psbt).map_err(|e| Error::Server(e.into()))) | ||
| // .map_err(|e| Error::Server(e.into()))? | ||
| }, | ||
| None, // TODO set to bitcoin::FeeRate::MIN or similar | ||
| )?; |
There was a problem hiding this comment.
How can the payjoin app submit a psbt for signing? I'm guessing this is where either submit_payout / trigger_payout would need to be called with the logic that a payjoin preempts an existing batch to include its contents in a payjoin. If it's preempting a batch, coin contributions will need to include those batch inputs as well.
|
One way of identifying which account an inbound payjoin is headed for would be to use a subdirectory. Rather than receiving all payjoins to a Of course, privacy of the account should be taken into consideration. One could also save payjoin-specific bitcoin addresses to a repo and check against those on proposal receipt, or generate linked id subdirectory that wouldn't reveal the whole account id. |
Perhaps an account can have a ‘public_alias’ that can be used as a path (instead of account id). Though we should probably discuss the merit of activating pj on account vs wallet vs address |
|
payjoin v2 receivers listen to sessions identified by an authentication/encryption keypair on a subdirectory. This public alias problem sounds like one that the v2 protocol can solve. Each payjoin session would be associated with an account and saved in a repository. |
Yes a PayjoinSessionRepo makes a lot of sense |
|
I peeled another layer back today addressing payjoin integration with the payout_queue and related abstractions. Once a sender's proposal is checked to be a suitable as a fallback to turn into a payjoin, we can think of it as being added to bria's mempool. This is distinct from our instance of bitcoind's mempool, since it's only in bria's section of memory, but we know it still is able to pay us if broadcast, gives us some funds via control of a foreign utxo to partially account for (since some will be paid back to the sender as change), and specifies at least one Payout, including a self-spend to one of our wallets that we may update and perhaps change that the sender has specified. It seems to me the foreign utxo may be accounted for by being already encumbered with value owed to the sender's change address as a precondition. TL;DR account for the sender's original_psbt input as our utxo rather than the original_psbt output. Either way bria should spend this utxo in the original_psbt fallback or a payjoin. We must first account for the sender's utxo(s) we can spend and then queue the fallback transaction extracted from the original_psbt for broadcast after an interval. After accounting we may construct payouts in a payjoin psbt that are dependent on those specific foreign utxos. Once utxo and payout accounting is set up on the fallback, we initialize a I believe if the |
|
Okay if I understood it correctly I like the general approach. Just a note on the accounting - I would figure that bit out last. When we have the capability to receive / process / sign /broadcast a pj tx e2e it should be a lot more straightforward to see what is missing on the accounting side. I find it hard to conceive upfront what the right way to deal with it is... So my recommendation would be to code the end-to-end process accepting that accounting will be off and then come back to that later. Regarding adding additional capabilities to the |
|
I think my point is that there are 2 moments of accounting rather than 1. Upon request, and then mempool update. Should it be possible to add a utxo to the repository without accounting for it? |
bodymindarts
force-pushed
the
main
branch
2 times, most recently
from
7057976 to
6ebd118
Compare
This is my hacking around trying to copy
adminmod into apayjoinserver that could trigger payouts when it senses a payjoin incoming. It does not build yet.I'm going to have to link Bria & Payjoin services because payjoin needs utxos and access to a wallet. I think it makes sense for
briato be a dependency of thepayjoinservice that hosts the endpoint rather than vice-versa and I'll be digging into it to find out.Totally possible I'm going down the wrong path, but I figure I've got to start somewhere to get a server running based on our conversation in #266