Skip to content

Commit 4dd746a

Browse files
node-smithxby72wnode-smithxby72w
committed
Use FnMut closures for v1 receiver typestate checks
In our liana wallet integration we are encountering some issues due to how the liana devs have setup their database, namely that their db always references a mutable self even on read calls within the db so we need to have some mutability when accessing things like an address or outpoint from the db. This was not possible when our closures were limited to non-mutable. I considered using FnOnce but we have some map methods that prevent us from doing that as map have the possiblity of forcing the closure being called more than once.
1 parent 7c963c6 commit 4dd746a

File tree

5 files changed

+108
-23
lines changed

5 files changed

+108
-23
lines changed

‎payjoin-cli/src/app/v2/mod.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -374,7 +374,7 @@ impl App {
374374
) -> Result<()> {
375375
let wallet = self.wallet();
376376
let proposal = proposal
377-
.check_inputs_not_owned(|input| {
377+
.check_inputs_not_owned(&mut |input| {
378378
wallet
379379
.is_mine(input)
380380
.map_err(|e| ImplementationError::from(e.into_boxed_dyn_error()))
@@ -389,7 +389,9 @@ impl App {
389389
persister: &ReceiverPersister,
390390
) -> Result<()> {
391391
let proposal = proposal
392-
.check_no_inputs_seen_before(|input| Ok(self.db.insert_input_seen_before(*input)?))
392+
.check_no_inputs_seen_before(&mut |input| {
393+
Ok(self.db.insert_input_seen_before(*input)?)
394+
})
393395
.save(persister)?;
394396
self.identify_receiver_outputs(proposal, persister).await
395397
}
@@ -401,7 +403,7 @@ impl App {
401403
) -> Result<()> {
402404
let wallet = self.wallet();
403405
let proposal = proposal
404-
.identify_receiver_outputs(|output_script| {
406+
.identify_receiver_outputs(&mut |output_script| {
405407
wallet
406408
.is_mine(output_script)
407409
.map_err(|e| ImplementationError::from(e.into_boxed_dyn_error()))

‎payjoin-ffi/src/receive/mod.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ impl MaybeInputsOwned {
417417
is_owned: impl Fn(&Vec<u8>) -> Result<bool, ImplementationError>,
418418
) -> MaybeInputsOwnedTransition {
419419
MaybeInputsOwnedTransition(Arc::new(RwLock::new(Some(
420-
self.0.clone().check_inputs_not_owned(|input| Ok(is_owned(&input.to_bytes())?)),
420+
self.0.clone().check_inputs_not_owned(&mut |input| Ok(is_owned(&input.to_bytes())?)),
421421
))))
422422
}
423423
}
@@ -473,7 +473,7 @@ impl MaybeInputsSeen {
473473
MaybeInputsSeenTransition(Arc::new(RwLock::new(Some(
474474
self.0
475475
.clone()
476-
.check_no_inputs_seen_before(|outpoint| Ok(is_known(&(*outpoint).into())?)),
476+
.check_no_inputs_seen_before(&mut |outpoint| Ok(is_known(&(*outpoint).into())?)),
477477
))))
478478
}
479479
}
@@ -532,7 +532,7 @@ impl OutputsUnknown {
532532
OutputsUnknownTransition(Arc::new(RwLock::new(Some(
533533
self.0
534534
.clone()
535-
.identify_receiver_outputs(|input| Ok(is_receiver_output(&input.to_bytes())?)),
535+
.identify_receiver_outputs(&mut |input| Ok(is_receiver_output(&input.to_bytes())?)),
536536
))))
537537
}
538538
}

‎payjoin/src/core/receive/v1/mod.rs‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,8 @@ impl UncheckedProposal {
141141
/// Call [`Self::check_inputs_not_owned`] to proceed.
142142
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
143143
pub struct MaybeInputsOwned {
144-
psbt: Psbt,
145-
params: Params,
144+
pub(crate) psbt: Psbt,
145+
pub(crate) params: Params,
146146
}
147147

148148
impl MaybeInputsOwned {
@@ -160,8 +160,9 @@ impl MaybeInputsOwned {
160160
/// An attacker can try to spend the receiver's own inputs. This check prevents that.
161161
pub fn check_inputs_not_owned(
162162
self,
163-
is_owned: impl Fn(&Script) -> Result<bool, ImplementationError>,
163+
is_owned: impl FnMut(&Script) -> Result<bool, ImplementationError>,
164164
) -> Result<MaybeInputsSeen, ReplyableError> {
165+
let mut is_owned = is_owned;
165166
let mut err: Result<(), ReplyableError> = Ok(());
166167
if let Some(e) = self
167168
.psbt
@@ -206,8 +207,9 @@ impl MaybeInputsSeen {
206207
/// original proposal PSBT of the current, new payjoin.
207208
pub fn check_no_inputs_seen_before(
208209
self,
209-
is_known: impl Fn(&OutPoint) -> Result<bool, ImplementationError>,
210+
is_known: impl FnMut(&OutPoint) -> Result<bool, ImplementationError>,
210211
) -> Result<OutputsUnknown, ReplyableError> {
212+
let mut is_known = is_known;
211213
self.psbt.input_pairs().try_for_each(|input| {
212214
match is_known(&input.txin.previous_output) {
213215
Ok(false) => Ok::<(), ReplyableError>(()),
@@ -248,8 +250,9 @@ impl OutputsUnknown {
248250
/// outputs.
249251
pub fn identify_receiver_outputs(
250252
self,
251-
is_receiver_output: impl Fn(&Script) -> Result<bool, ImplementationError>,
253+
is_receiver_output: impl FnMut(&Script) -> Result<bool, ImplementationError>,
252254
) -> Result<WantsOutputs, ReplyableError> {
255+
let mut is_receiver_output = is_receiver_output;
253256
let owned_vouts: Vec<usize> = self
254257
.psbt
255258
.unsigned_tx
@@ -899,6 +902,13 @@ pub(crate) mod test {
899902
UncheckedProposal { psbt: PARSED_ORIGINAL_PSBT.clone(), params }
900903
}
901904

905+
pub(crate) fn maybe_inputs_owned_from_test_vector() -> MaybeInputsOwned {
906+
let pairs = url::form_urlencoded::parse(QUERY_PARAMS.as_bytes());
907+
let params = Params::from_query_pairs(pairs, &[Version::One])
908+
.expect("Could not parse params from query pairs");
909+
MaybeInputsOwned { psbt: PARSED_ORIGINAL_PSBT.clone(), params }
910+
}
911+
902912
fn wants_outputs_from_test_vector(proposal: UncheckedProposal) -> WantsOutputs {
903913
proposal
904914
.assume_interactive_receiver()
@@ -925,6 +935,35 @@ pub(crate) mod test {
925935
.expect("Contributed inputs should allow for valid fee contributions")
926936
}
927937

938+
#[test]
939+
fn test_mutable_receiver_state_closures() {
940+
let mut call_count = 0;
941+
let maybe_inputs_owned = maybe_inputs_owned_from_test_vector();
942+
943+
fn mock_callback(call_count: &mut usize, ret: bool) -> Result<bool, ImplementationError> {
944+
*call_count += 1;
945+
Ok(ret)
946+
}
947+
948+
let maybe_inputs_seen =
949+
maybe_inputs_owned.check_inputs_not_owned(|_| mock_callback(&mut call_count, false));
950+
assert_eq!(call_count, 1);
951+
952+
let outputs_unknown = maybe_inputs_seen
953+
.map_err(|_| "Check inputs owned closure failed".to_string())
954+
.expect("Next receiver state should be accessible")
955+
.check_no_inputs_seen_before(|_| mock_callback(&mut call_count, false));
956+
assert_eq!(call_count, 2);
957+
958+
let _wants_outputs = outputs_unknown
959+
.map_err(|_| "Check no inputs seen closure failed".to_string())
960+
.expect("Next receiver state should be accessible")
961+
.identify_receiver_outputs(|_| mock_callback(&mut call_count, true));
962+
// there are 2 receiver outputs so we should expect this callback to run twice incrementing
963+
// call count twice
964+
assert_eq!(call_count, 4);
965+
}
966+
928967
#[test]
929968
fn is_output_substitution_disabled() {
930969
let mut proposal = unchecked_proposal_from_test_vector();

‎payjoin/src/core/receive/v2/mod.rs‎

Lines changed: 53 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ impl Receiver<MaybeInputsOwned> {
568568
/// An attacker can try to spend the receiver's own inputs. This check prevents that.
569569
pub fn check_inputs_not_owned(
570570
self,
571-
is_owned: impl Fn(&Script) -> Result<bool, ImplementationError>,
571+
is_owned: &mut impl FnMut(&Script) -> Result<bool, ImplementationError>,
572572
) -> MaybeFatalTransition<SessionEvent, Receiver<MaybeInputsSeen>, ReplyableError> {
573573
let inner = match self.state.v1.clone().check_inputs_not_owned(is_owned) {
574574
Ok(inner) => inner,
@@ -618,7 +618,7 @@ impl Receiver<MaybeInputsSeen> {
618618
/// original proposal PSBT of the current, new payjoin.
619619
pub fn check_no_inputs_seen_before(
620620
self,
621-
is_known: impl Fn(&OutPoint) -> Result<bool, ImplementationError>,
621+
is_known: &mut impl FnMut(&OutPoint) -> Result<bool, ImplementationError>,
622622
) -> MaybeFatalTransition<SessionEvent, Receiver<OutputsUnknown>, ReplyableError> {
623623
let inner = match self.state.v1.clone().check_no_inputs_seen_before(is_known) {
624624
Ok(inner) => inner,
@@ -673,7 +673,7 @@ impl Receiver<OutputsUnknown> {
673673
/// outputs.
674674
pub fn identify_receiver_outputs(
675675
self,
676-
is_receiver_output: impl Fn(&Script) -> Result<bool, ImplementationError>,
676+
is_receiver_output: &mut impl FnMut(&Script) -> Result<bool, ImplementationError>,
677677
) -> MaybeFatalTransition<SessionEvent, Receiver<WantsOutputs>, ReplyableError> {
678678
let inner = match self.state.inner.clone().identify_receiver_outputs(is_receiver_output) {
679679
Ok(inner) => inner,
@@ -1099,6 +1099,50 @@ pub mod test {
10991099
}
11001100
}
11011101

1102+
pub(crate) fn maybe_inputs_owned_v2_from_test_vector() -> MaybeInputsOwned {
1103+
let pairs = url::form_urlencoded::parse(QUERY_PARAMS.as_bytes());
1104+
let params = Params::from_query_pairs(pairs, &[Version::Two])
1105+
.expect("Test utils query params should not fail");
1106+
MaybeInputsOwned {
1107+
v1: v1::MaybeInputsOwned { psbt: PARSED_ORIGINAL_PSBT.clone(), params },
1108+
context: SHARED_CONTEXT.clone(),
1109+
}
1110+
}
1111+
1112+
#[test]
1113+
fn test_v2_mutable_receiver_state_closures() {
1114+
let mut call_count = 0;
1115+
let maybe_inputs_owned = maybe_inputs_owned_v2_from_test_vector();
1116+
let receiver = v2::Receiver { state: maybe_inputs_owned };
1117+
1118+
fn mock_callback(call_count: &mut usize, ret: bool) -> Result<bool, ImplementationError> {
1119+
*call_count += 1;
1120+
Ok(ret)
1121+
}
1122+
1123+
let maybe_inputs_seen =
1124+
receiver.check_inputs_not_owned(&mut |_| mock_callback(&mut call_count, false));
1125+
assert_eq!(call_count, 1);
1126+
1127+
let outputs_unknown = maybe_inputs_seen
1128+
.0
1129+
.map_err(|_| "Check inputs owned closure failed".to_string())
1130+
.expect("Next receiver state should be accessible")
1131+
.1
1132+
.check_no_inputs_seen_before(&mut |_| mock_callback(&mut call_count, false));
1133+
assert_eq!(call_count, 2);
1134+
1135+
let _wants_outputs = outputs_unknown
1136+
.0
1137+
.map_err(|_| "Check no inputs seen closure failed".to_string())
1138+
.expect("Next receiver state should be accessible")
1139+
.1
1140+
.identify_receiver_outputs(&mut |_| mock_callback(&mut call_count, true));
1141+
// there are 2 receiver outputs so we should expect this callback to run twice incrementing
1142+
// call count twice
1143+
assert_eq!(call_count, 4);
1144+
}
1145+
11021146
#[test]
11031147
fn test_unchecked_proposal_transient_error() -> Result<(), BoxError> {
11041148
let unchecked_proposal = unchecked_proposal_v2_from_test_vector();
@@ -1127,7 +1171,7 @@ pub mod test {
11271171
let receiver = v2::Receiver { state: unchecked_proposal };
11281172

11291173
let maybe_inputs_owned = receiver.assume_interactive_receiver();
1130-
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(|_| {
1174+
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(&mut |_| {
11311175
Err(ImplementationError::new(ReplyableError::Implementation("mock error".into())))
11321176
});
11331177

@@ -1150,9 +1194,9 @@ pub mod test {
11501194
let receiver = v2::Receiver { state: unchecked_proposal };
11511195

11521196
let maybe_inputs_owned = receiver.assume_interactive_receiver();
1153-
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(|_| Ok(false));
1197+
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(&mut |_| Ok(false));
11541198
let outputs_unknown = match maybe_inputs_seen.0 {
1155-
Ok(state) => state.1.check_no_inputs_seen_before(|_| {
1199+
Ok(state) => state.1.check_no_inputs_seen_before(&mut |_| {
11561200
Err(ImplementationError::new(ReplyableError::Implementation("mock error".into())))
11571201
}),
11581202
Err(_) => panic!("Expected Ok, got Err"),
@@ -1177,13 +1221,13 @@ pub mod test {
11771221
let receiver = v2::Receiver { state: unchecked_proposal };
11781222

11791223
let maybe_inputs_owned = receiver.assume_interactive_receiver();
1180-
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(|_| Ok(false));
1224+
let maybe_inputs_seen = maybe_inputs_owned.0 .1.check_inputs_not_owned(&mut |_| Ok(false));
11811225
let outputs_unknown = match maybe_inputs_seen.0 {
1182-
Ok(state) => state.1.check_no_inputs_seen_before(|_| Ok(false)),
1226+
Ok(state) => state.1.check_no_inputs_seen_before(&mut |_| Ok(false)),
11831227
Err(_) => panic!("Expected Ok, got Err"),
11841228
};
11851229
let wants_outputs = match outputs_unknown.0 {
1186-
Ok(state) => state.1.identify_receiver_outputs(|_| {
1230+
Ok(state) => state.1.identify_receiver_outputs(&mut |_| {
11871231
Err(ImplementationError::new(ReplyableError::Implementation("mock error".into())))
11881232
}),
11891233
Err(_) => panic!("Expected Ok, got Err"),

‎payjoin/tests/integration.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -763,7 +763,7 @@ mod integration {
763763

764764
// Receive Check 2: receiver can't sign for proposal inputs
765765
let proposal = proposal
766-
.check_inputs_not_owned(|input| {
766+
.check_inputs_not_owned(&mut |input| {
767767
let address = bitcoin::Address::from_script(input, bitcoin::Network::Regtest)
768768
.map_err(ImplementationError::new)?;
769769
receiver
@@ -775,9 +775,9 @@ mod integration {
775775

776776
// Receive Check 3: have we seen this input before? More of a check for non-interactive i.e. payment processor receivers.
777777
let payjoin = proposal
778-
.check_no_inputs_seen_before(|_| Ok(false))
778+
.check_no_inputs_seen_before(&mut |_| Ok(false))
779779
.save(&noop_persister)?
780-
.identify_receiver_outputs(|output_script| {
780+
.identify_receiver_outputs(&mut |output_script| {
781781
let address =
782782
bitcoin::Address::from_script(output_script, bitcoin::Network::Regtest)
783783
.map_err(ImplementationError::new)?;

0 commit comments

Comments
 (0)