Add a check for receiver to validate minimum required fees #113

jbesraa · 2023-11-10T11:49:05Z

resolves #105

Payjoin receiver can set minimum `fee_rate`
in order to reject payjoin requests that contain
a psbt which doesnt meet the minimum `fee_rate`
threshold.

We add this functionality to `check_broadcast_suitability`
function which previously was called `check_can_broadcast`.

jbesraa · 2023-11-10T11:51:45Z

payjoin/src/receive/mod.rs

+    /// Check that the Original PSBT meet the minimum feerate receiver requires.
+    pub fn check_if_meet_min_feerate(self, receiver_min_feerate: FeeRate) -> Result<(), Error> {
+        let sender_fee_rate = self.params.min_feerate;
+        // let sender_addtional_fee_contribution = self.params.additional_fee_contribution;


what else should we take into account in this check?
we have another couple of params that can be interesting:

self.psbt.fee() (this thing returns Bitcoin::Amount but I guess we can convert it to bitcoin::FeeRate

self.params.addition_fee_contr - this need to be validated(other index input should be set) first which we do in a different check

params.min_feerate != original psbt feerate. That parameter can be set arbitrarily. What we want to check is the effective FeeRate of self.psbt to make sure this is a reliable fallback if the sender disappears. Otherwise, receiver would be subject to probing attacks where the original psbt fee rate is too low for the receiver to actually ever get it included in a block in the fallback case. As you mention, additional_fee_contribution is handled later on and can be ignored in this check.

Perhaps instead of check_if_meet_min_feerate this functionality could be combined with check_can_broadcast. Since that is already a testmempoolaccept check to prevent probing, which is the basis for this issue.

Something like check_broadcast_suitability(self, min_feerate: Option<bitcoin::FeeRate>, can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, Error> could replace check_can_broadcast.

min_feerate could be enforced on `assume_interactive_receiver(self, min_feerate: Optionbitcoin::FeeRate)`` using a shared helper function, too

As a bonus this uses our typestate machine so the check can't be so easily overlooked.

thanks for the explanation
sorry for the delay, working on it

thanks for the patience
I pushed a new version and split the commits.

payjoin/src/receive/mod.rs

payjoin/src/receive/error.rs

payjoin/src/receive/mod.rs

payjoin/src/receive/optional_parameters.rs

DanGould · 2023-12-04T20:59:23Z

payjoin/src/receive/mod.rs

+    fn proposal_from_test_vector(
+        min_feerate: Option<u64>,
+    ) -> Result<UncheckedProposal, RequestError> {


Why is this fn prematurely paramaterized if the param is not being used?

payjoin/src/receive/mod.rs

payjoin/src/receive/error.rs

payjoin/src/receive/mod.rs

DanGould · 2023-12-04T21:22:56Z

payjoin/src/receive/mod.rs

+        if let (Some(receiver_min_feerate), Ok(psbt_feerate)) = (min_feerate, self.psbt.fee()) {
+            if receiver_min_feerate.to_sat_per_kwu() > psbt_feerate.to_sat() {
+                return Err(Error::BadRequest(
+                    InternalRequestError::FeeTooLow(psbt_feerate, receiver_min_feerate).into(),
+                ));
+            }
+        }


I do not believe this is an apples-to-apples comparison. It compaes paramaterized FeeRate expressed per kwu with the absolute fee of the original_psbt.

e.g. the param could be 10sat/b which and the absolute psbt.fee() would be e.g. 14,067. Comparing 10sat/b * 250 sat per kwu = 2500sat [per kwu] to 14,067sat makes no sense. That would be comparing two totally different things.

I think it makes more sense to compare FeeRates.

First, calculate the psbt_feerate (the current so-called psbt_feerate is not a feerate at all, there is no denominator) by dividing the psbt.fee() with the extracted transaction weight. e.g.

let original_transaction = self.psbt.clone().extract_tx(); if let (Some(min_feerate), Ok(original_psbt_fee)) = (min_feerate, self.psbt.fee()) { let original_psbt_feerate = original_psbt_fee / original_transaction.weight(); if original_psbt_feerate < min_feerate { return Err(Error::BadRequest( InternalRequestError::FeeTooLow(original_psbt_feerate, min_feerate).into(), )); } }

Even my suggested code is also incomplete. It disregards the condition where self.psbt.fee().is_err(). That condition MUST also be handled with an error. I'll leave that exercise to you.

DanGould · 2023-12-04T21:25:49Z

payjoin/tests/integration.rs

+        // Here we set receiver min feerate to be higher than the proposal's psbt feerate
+        // This should fail the check
+        let receiver_min_feerate = bitcoin::FeeRate::from_sat_per_kwu(283);
+        assert!(proposal
+            .clone()
+            .check_broadcast_suitability(Some(receiver_min_feerate), |tx| {
+                Ok(receiver
+                    .test_mempool_accept(&[bitcoin::consensus::encode::serialize_hex(&tx)])
+                    .unwrap()
+                    .first()
+                    .unwrap()
+                    .allowed)
+            })
+            .is_err());
+        // Here we set receiver min feerate to be lower than the proposal's psbt feerate
+        let receiver_min_feerate = bitcoin::FeeRate::from_sat_per_kwu(281);
        let proposal = proposal
-            .check_can_broadcast(|tx| {
+            .check_broadcast_suitability(Some(receiver_min_feerate), |tx| {


If we want to show test succeeds / fails, we should get the extracted txs feerate in the test and make sure the params are relevant. Because current check has nothing to do with the actual original_psbt feerate! only absolute fee.

jbesraa · 2023-12-08T14:27:45Z

payjoin/src/receive/mod.rs

@@ -852,7 +873,7 @@ mod test {
        let headers = MockHeaders::new(body.len() as u64);
        UncheckedProposal::from_request(
            body,
-            "?maxadditionalfeecontribution=182?additionalfeeoutputindex=0",
+            "maxadditionalfeecontribution=182&additionalfeeoutputindex=0&minfeerate=0",


not required

jbesraa · 2023-12-08T14:33:14Z

payjoin/src/receive/mod.rs

@@ -890,4 +911,10 @@ mod test {

        assert!(payjoin.is_ok(), "Payjoin should be a valid PSBT");
    }
+
+    #[test]
+    fn psbt_fee_rate() {


add another test vectors

The previous `query` used `?` to separate between pairs while it should be `&` as per the `url_fromencoded` crate.

[`payjoin::receive::optional_parameters::Params`] [`payjoin::receive::UncheckedProposal`]

jbesraa · 2023-12-12T17:49:09Z

@DanGould
Rebased from master branch which required some minor changes to v2 stuff, and removed not required changes in the scope of this pull request.
We talked about test vectors for different PSBTs, but I was not able to find a quick solution to generate PSBTs with some configuration regrading fee or weights, might be something we want to develop internally.
I think this pr can go on and we can look into more robust testing in the (near) future.

DanGould

Getting much closer 🔥 I think this gets the job done and only request a few cleanups

DanGould · 2023-12-13T00:30:15Z

payjoin/src/receive/mod.rs

@@ -345,26 +346,46 @@ impl UncheckedProposal {
    }

    /// The Sender's Original PSBT
-    pub fn extract_tx_to_schedule_broadcast(&self) -> bitcoin::Transaction {
-        self.psbt.clone().extract_tx()
+    pub fn psbt_transaction(&self) -> bitcoin::Transaction { self.psbt.clone().extract_tx() }


This name change seems unrelated to the receiver check this PR is about and I'd prefer it wasn't made. extract_tx (for the specific api reason) tries to convey why the api is available, and extract lets you know it's calculating rather than returning something from memory.

Updating this would also merit a change to the documentation at the front matter of this file

DanGould · 2023-12-13T00:31:24Z

payjoin/src/receive/mod.rs

+    pub fn psbt_transaction(&self) -> bitcoin::Transaction { self.psbt.clone().extract_tx() }
+
+    /// The Sender's Original PSBT fee rate.
+    pub fn psbt_fee_rate(&self) -> Result<FeeRate, Error> {


Not sure this function should be public, especially since it returns BadRequest, which doesn't make much sense as a return type for a fee_rate here

DanGould · 2023-12-13T00:34:02Z

payjoin/src/receive/mod.rs

+                return Err(Error::BadRequest(
+                    InternalRequestError::PsbtBelowFeeRate(original_psbt_fee_rate, min_fee_rate)


I suggest implicit conversion to Error by using .into() on its own. See return Err(InternalRequestError::InvalidContentType(content_type.to_owned()).into());

DanGould · 2023-12-13T00:35:08Z

payjoin/src/receive/mod.rs

@@ -873,6 +894,7 @@ mod test {
        use bitcoin::{Address, Network};

        let proposal = proposal_from_test_vector().unwrap();
+        assert_eq!(proposal.psbt_fee_rate().unwrap().to_sat_per_vb_floor(), 2);


I understand why you might make psbt_fee_rate public for this function, but it's so brief and nonessential to the payjoin api I'd rather see it repeated in the test than exposed publically.

the pub was a leftover from when i was testing in integration, removed that.
here in the unit test it can access all the functions/structs in the same module so its fine.

DanGould · 2023-12-13T00:36:02Z

payjoin/src/receive/v2.rs

 /// call assume_interactive_receive to proceed with validation.
 pub struct UncheckedProposal {
-    inner: super::UncheckedProposal,
+    pub inner: super::UncheckedProposal,


Why make this public? Ideally these typestate machines are kept opaque so that the only modifications happen through the machine.

DanGould · 2023-12-13T00:37:11Z

payjoin/src/receive/v2.rs

+    pub fn check_broadcast_suitability(
        self,
        can_broadcast: impl Fn(&bitcoin::Transaction) -> Result<bool, Error>,
    ) -> Result<MaybeInputsOwned, Error> {
-        let inner = self.inner.check_can_broadcast(can_broadcast)?;
+        let inner = self.inner.check_broadcast_suitability(None, can_broadcast)?;


This v2 UncheckedProposal should have the same interface for self.inner.check_boradcast_suitablity and accept min_feerate. HardcodingNone should raise a yellow flag.

DanGould · 2023-12-13T00:39:29Z

payjoin/src/receive/error.rs

+            InternalRequestError::PsbtBelowFeeRate(
+                original_psbt_fee_rate,
+                receiver_min_fee_rate,
+            ) => write_error(
+                f,
+                "original-psbt-rejected",
+                &format!(
+                    "Original PSBT fee rate too low: {} < {}.",
+                    original_psbt_fee_rate, receiver_min_fee_rate
+                ),
+            ),


Very nice! 🤌

DanGould · 2023-12-13T00:40:00Z

payjoin/src/receive/error.rs

+    /// First argument is the calculated fee rate of the original PSBT.
+    /// [`UncheckedProposal::psbt_fee_rate`]


remember to remove this comment if choosing to remove psbt_fee_rate from the public interface.

Payjoin receiver can set minimum `fee_rate` in order to reject payjoin requests that contain a psbt which doesnt meet the minimum `fee_rate` threshold. We add this functionality to `check_broadcast_suitability` function which previously was called `check_can_broadcast`.

jbesraa · 2023-12-13T04:59:43Z

Thanks for the input @DanGould
I have addressed all the above

DanGould

Powerful speed on the turnaround here.

The only discrepancy I see is that between receive::UncheckedProposal and receive::v2::UncheckedProposal implementing Clone. But that's a problem because the underlying ohttp::KeyConfig is not Clone so it would be a pain to make happen.

I might have also dropped psbt_fee_rate entirely and tested check_broadcast_suitability in the unchecked_proposal_unlocks_after_checks receiver checks instead of leaving assume and calling assert_eq!(proposal.psbt_fee_rate().unwrap().to_sat_per_vb_floor(), 2); But fortunately I see this works anyhow. It may be done in a follow up.

e.g.

.check_broadcast_suitability(Some(FeeRate::from_sat_per_vb(3).unwrap()), |_| Ok(true)) // is Err

.check_broadcast_suitability(Some(FeeRate::from_sat_per_vb(2).unwrap()), |_| Ok(true)) // is Ok

DanGould · 2023-12-13T20:54:39Z

Ci failed to catch the need to change here:

rust-payjoin/payjoin-cli/src/app.rs

Line 542 in 624ab7f

let proposal = proposal.check_can_broadcast(|tx| {

We need to make sure payjoin-cli builds in v2

jbesraa commented Nov 10, 2023

View reviewed changes

payjoin/src/receive/mod.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the feat/reciever-min-feerate branch 4 times, most recently from f7e97b7 to 91792f1 Compare November 28, 2023 15:34

jbesraa added the receive receiving payjoin label Nov 28, 2023

jbesraa commented Nov 28, 2023

View reviewed changes

payjoin/src/receive/error.rs Outdated Show resolved Hide resolved

jbesraa commented Nov 28, 2023

View reviewed changes

payjoin/src/receive/mod.rs Show resolved Hide resolved

jbesraa commented Nov 28, 2023

View reviewed changes

payjoin/src/receive/mod.rs Show resolved Hide resolved

jbesraa commented Nov 28, 2023

View reviewed changes

payjoin/src/receive/optional_parameters.rs Show resolved Hide resolved

DanGould requested changes Dec 4, 2023

View reviewed changes

jbesraa force-pushed the feat/reciever-min-feerate branch 6 times, most recently from 9b06a53 to 1a26d3d Compare December 8, 2023 12:49

jbesraa commented Dec 8, 2023

View reviewed changes

jbesraa added 2 commits December 12, 2023 19:39

Fix query arg in proposal_from_test_vector

e498033

The previous `query` used `?` to separate between pairs while it should be `&` as per the `url_fromencoded` crate.

Add Clone to Params and UncheckedProposal

2082136

[`payjoin::receive::optional_parameters::Params`] [`payjoin::receive::UncheckedProposal`]

jbesraa force-pushed the feat/reciever-min-feerate branch from 1a26d3d to 7b3678f Compare December 12, 2023 17:39

DanGould requested changes Dec 13, 2023

View reviewed changes

DanGould added the api label Dec 13, 2023

jbesraa force-pushed the feat/reciever-min-feerate branch from 7b3678f to 3036b2f Compare December 13, 2023 04:51

DanGould approved these changes Dec 13, 2023

View reviewed changes

DanGould merged commit 624ab7f into payjoin:master Dec 13, 2023
5 checks passed

DanGould mentioned this pull request Dec 13, 2023

Test payjoin-cli features v2 builds in ci #138

Closed

jbesraa mentioned this pull request Dec 14, 2023

Save + load send receive payjoin v2 cli sessions for asynchronicity #136

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a check for receiver to validate minimum required fees #113

Add a check for receiver to validate minimum required fees #113

jbesraa commented Nov 10, 2023 •

edited

Loading

jbesraa Nov 10, 2023

DanGould Nov 10, 2023

jbesraa Nov 23, 2023 •

edited

Loading

jbesraa Nov 28, 2023

DanGould Dec 4, 2023

DanGould Dec 4, 2023

DanGould Dec 4, 2023

jbesraa Dec 8, 2023

jbesraa Dec 8, 2023

jbesraa commented Dec 12, 2023

DanGould left a comment

DanGould Dec 13, 2023

DanGould Dec 13, 2023

DanGould Dec 13, 2023

DanGould Dec 13, 2023

jbesraa Dec 13, 2023

DanGould Dec 13, 2023

DanGould Dec 13, 2023

DanGould Dec 13, 2023

DanGould Dec 13, 2023

jbesraa commented Dec 13, 2023

DanGould left a comment

DanGould commented Dec 13, 2023

		return Err(Error::BadRequest(
		InternalRequestError::PsbtBelowFeeRate(original_psbt_fee_rate, min_fee_rate)

		/// First argument is the calculated fee rate of the original PSBT.
		/// [`UncheckedProposal::psbt_fee_rate`]

Add a check for receiver to validate minimum required fees #113

Add a check for receiver to validate minimum required fees #113

Conversation

jbesraa commented Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Dec 12, 2023

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Dec 13, 2023

DanGould left a comment

Choose a reason for hiding this comment

DanGould commented Dec 13, 2023

jbesraa commented Nov 10, 2023 •

edited

Loading

jbesraa Nov 23, 2023 •

edited

Loading