feat(trading-proto-upgrade): fees fixes among other things #2109

mariocynicys · 2024-05-06T07:09:10Z

The main feat here is taker payment spend fee negotiation (I guess we will refactor namings in the swaps v2 code and rename this to taker payment fee/funding spend fee):

The negotiation goes as follows:

Taker and maker each decide how much this fee should be at the beginning of the swap.
The taker will send the maker their proposed fee during negotiation,
if the maker deems the fee as low enough (less than 90% of the maker's own calculated fee),
they will refuse to trade before the trade starts. Otherwise, they
will continue and use this fee for validation later.
The maker will validate that the taker has accounted for this fee in
the funding transaction.
The taker will validate that the taker payment preimage (generated by the maker)
uses the negotiated fee or greater (greater number will deduct from the maker's trading volume anyway).
If not, the taker will stop the trade since the maker is clearly stealing funds that isn't his
which might affect the completion of the swap if the fee is low.

Proto v2 issue pool:

was already fixed:
- immediate taker refund using S2
- accept_only_from our order peer.
fixed in this PR:
- use a u8 fork id
- negotiate funding spend fee and let taker add it to the funding tx
still missing:
- research: concurrent conf
- recover_funds_for_swap rpc for swaps v2
- investigation of why test_taker_completes_swap_after_restart is flaky
- other refactors

Fixmes & discuss comments are intended to be covered in the PR.

shamardy · 2024-05-06T15:07:27Z

@mariocynicys please add label for if this is under review or in progress.

laruh · 2024-05-07T03:36:47Z

@mariocynicys please merge dev to feature branch as it contains build fixes
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/8965111556/job/24617995991?pr=2109#step:8:574

error[E0463]: can't find crate for `core`
  |
  = note: the `x86_64-apple-darwin` target may not be installed
  = help: consider downloading the target with `rustup target add x86_64-apple-darwin`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

shamardy · 2024-05-07T11:52:10Z

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs

@@ -822,6 +824,8 @@ impl<MakerCoin: MmCoin + MakerCoinSwapOpsV2, TakerCoin: MmCoin + TakerCoinSwapOp
            },
        };

+        // FIXME: I am lost here, is this the fee for funding spend or taker payment spend?
+        // if it's the former, where is the fee for the the taker payment spend?
        let taker_payment_spend_trade_fee = match state_machine.taker_coin.get_receiver_trade_fee(stage).compat().await
        {


This is the fee paid by the receiver of a payment, it was an htlc spend fee in the legacy swap protocol. For v2, it should be 0 for maker like we agreed before (but maybe not in this PR, and we make it the taker payment spend fee for now), and for taker it should be the fee needed to spend the maker payment which is the same as the legacy protocol. Is this clear enough?

komodo-defi-framework/mm2src/coins/lp_coins.rs

Lines 3102 to 3103 in 3c888aa

/// Get fee to be paid by receiver per whole swap and check if the wallet has sufficient balance to pay the fee.

fn get_receiver_trade_fee(&self, stage: FeeApproxStage) -> TradePreimageFut<TradeFee>;

and for taker it should be the fee needed to spend the maker payment which is the same as the legacy protocol.

As discussed in DM with @mariocynicys, this should be different than legacy protocol too since the redeem script is a bit different due to the immediate refund path. I will make this in progress again @mariocynicys

Actually should be called funding spend fee or taker payment fee, but we are just following the current code naming at the moment. The negotiation goes as follows: 1- Taker and maker each decide how much this fee should be at the beginning of the swap. 2- The taker will send the maker their proposed fee during negotiation, if the maker deems the fee as low enough (less than 90% of the maker's own calculated fee), they will refuse to trade before the trade starts. Otherwise, they will continue and use this fee for validation later. 3- The maker will validate that the taker has accounted for this fee in the funding transaction. 4- The taker will validate that the taker payment preimage (generated by the maker) uses the negotiated fee or greater (greater number will deduct from the maker's trading volume anyway). If not, the taker will stop the trade since the maker is clearly stealing funds that isn't his which might affect the completion of the swap if the fee is low.

…o u64 The big decimal might be a fraction less than zero and gets converted to zero when sent over the wire (because we send the fee as u64). Scale it properly before sending it.

noticed this in the integration tests, the fee might be a fraction so clamping it to one yields a fee higher than the actually one, thus fails the ratio check

use a signature-size dummy date instead

taker payment spend is actually taker payment here, next commit should fix the naming inconsistencies

laruh

Thank you for the PR! here are some small notes from my side

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs

… avoid weird precision

laruh · 2024-05-31T12:16:35Z

ps @mariocynicys this pr started to have conflicts

laruh · 2024-06-18T09:08:41Z

ps @mariocynicys lint check fails

shamardy

Thanks a lot for the enhancements! Next review iteration!

mm2src/coins/lp_coins.rs

shamardy · 2024-07-01T17:22:58Z

mm2src/coins/lp_coins.rs

+    /// Get the fee to be paid for the processing of the maker payment transaction.
+    async fn get_maker_payment_fee(&self, stage: &FeeApproxStage) -> TradePreimageResult<TradeFee>;
+
+    /// Get the fee to be paid for the processing of the maker payment spend transaction aka the claimation transaction.
+    async fn get_maker_payment_spend_fee(&self, stage: &FeeApproxStage) -> TradePreimageResult<TradeFee>;


Why would we need the get_maker_payment_spend_fee function in the future? I understand that get_maker_payment_fee will be used so that the taker can cover this fee on the funding transaction, but I don't see a need for get_maker_payment_spend_fee

I guess this is just to calculate fee from the taker side in a right way.

I think it should be used here:

komodo-defi-framework/mm2src/mm2_main/src/lp_swap/taker_swap_v2.rs

Line 972 in 4f5a04a

let maker_payment_spend_fee = match state_machine.maker_coin.get_receiver_trade_fee(stage).compat().await {

In place of get_receiver_trade_fee, but I am trying to figure out why didn't i use it O_O

mm2src/coins/lp_coins.rs

mm2src/coins/utxo/utxo_common.rs

shamardy · 2024-07-01T18:04:58Z

mm2src/coins/utxo/utxo_common.rs

+            // expected fee knowing that the maker will always accept incurring the difference.
+            calculated_fee.max(taker_instructed_fee)


Why not always use the calculated_fee? if it's less than or equal to the taker_instructed_fee makers can get some additional amount which is better than giving it to the miners.

We actually discussed this before xD

IIRC, the point was to not compromise the swap success if the maker wants to greedily cut on the fees. to stop that, any additional funds that were designated to be used as fees should only be used as fees otherwise the swap will fail.

shamardy · 2024-07-01T18:35:35Z

mm2src/coins/utxo/utxo_common.rs

+    let instructed_fee =
+        sat_from_big_decimal(&gen_args.taker_payment_fee, coin.as_ref().decimals).mm_err(|e| e.into())?;

-    let fee_div = expected_fee as f64 / actual_fee as f64;
-
-    if !(0.9..=1.1).contains(&fee_div) {
+    if actual_fee < instructed_fee {
        return MmError::err(ValidateTakerFundingSpendPreimageError::UnexpectedPreimageFee(format!(
-            "Too large difference between expected {} and actual {} fees",
-            expected_fee, actual_fee
+            "A fee of {} was used, less than the agreed upon fee of {}",
+            actual_fee, instructed_fee


Ooh, I see why you used the max of calculated_fee and taker_instructed_fee above. I don't think this validation is actually needed at all, this is done from the taker side, and once the swap starts any additional fee or reduced fee will either reduce the maker amount or benefit him so the maker can choose any fee on his side when generating the preimage and the taker doesn't have to validate it. Also for the below, maker negotiates a fee at the start / negotiation phase and taker either approves it or not, so there is no room for abuse.

komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs

Line 1075 in b6ee698

// FIXME: Possible abuse vector is that the taker can always send the fee to be just above 90% of the

once the swap starts any additional fee or reduced fee will either reduce the maker amount or benefit him so the maker can choose any fee on his side when generating the preimage and the taker doesn't have to validate it

I guess we have to lookout for the refund case though, since if maker chooses a very high fee and doesn't complete the swap, taker will lose funds on refund.

Also for the below, maker negotiates a fee at the start / negotiation phase and taker either approves it or not, so there is no room for abuse.

What about removing the fee pumping logic (so we won't calculate calculated_fee and won't use the max) and use the taker_instructed_fee right away. Re-thinking about it, the max-ing might not make that much of a difference (most of the time), and this would also stop the attack vector where the maker sets a high fee just to hit the taker on his refund.

Using the taker_instructed_fee is the right idea IMHO but the problem is fees spikes as always. There are 2 solutions:

Either just use taker_instructed_fee, taker will want to optimize for this since they wouldn't want to overpay and they will want a fast swap at the same time so there would be no attack vector and if the taker payment transaction takes too much time to confirm due to a fee spike there is always the option of immediate refund. We should allow maker paying less for fees if possible since it will benefit either the maker on success or taker on refund, so taker will validate the fee to be either taker_instructed_fee or less.

The other solution is more complicated and it's a borrow from lightning where the maker and taker renegotiate the fee if there is a fee spike but this complicates things a lot.

Let's use option one for now and wait until this is tested in a real environment. For fee spikes we can allow the taker to use a bit higher fee from his side (10% higher or we can set it more accurately once we see real data) if needed in the future.

also changed to todo

revising the commit history, looks like this was deliberate

ref. https://bitcoin.stackexchange.com/questions/48108/why-are-sighash-flags-signed-as-4-bytes-when-only-1-byte-is-included-in-the-tran

always rename discuss markers to fixmes since i forget about them xD

there is no calculatable tx_size for other tx (non-htlc) (like funding and maker payment txs). their fees depend really on the utxos we have and whether they are segwit or not. used get_sender_trade_fee for now, though I think this method needs reviewing since its doc comment doesn't make a lot of sense

laruh · 2024-11-11T10:52:47Z

ps @mariocynicys this pr started to have conflicts

shamardy requested review from shamardy and laruh May 6, 2024 15:00

shamardy changed the title ~~Proto upgrade fixups~~ feat(trading-proto-upgrade): fees fixes among other things May 6, 2024

mariocynicys added under review in progress Changes will be made from the author and removed under review in progress Changes will be made from the author labels May 6, 2024

mariocynicys force-pushed the proto-upgrade branch from fdd2451 to a9dbd14 Compare May 7, 2024 05:48

shamardy reviewed May 7, 2024

View reviewed changes

shamardy added in progress Changes will be made from the author and removed under review labels May 7, 2024

mariocynicys added 15 commits May 9, 2024 13:41

use u8 for forkid

13a798e

add fixmes, amend me

9a7d81b

Use big_decimal_from_sat & sat_from_big_decimal for conversions t…

151f5a8

…o u64 The big decimal might be a fraction less than zero and gets converted to zero when sent over the wire (because we send the fee as u64). Scale it properly before sending it.

fix tests

9f722fb

can't clamp the fee to 1 since it's probably higher than the fee

ef28a6c

noticed this in the integration tests, the fee might be a fraction so clamping it to one yields a fee higher than the actually one, thus fails the ratio check

remove wrong transition

0bd984a

add some tests

ad21f4e

add a calculator for taker payment spend transaction size

ff05d06

don't actually sign anything to avoid unwrapping

c20bac2

use a signature-size dummy date instead

account for the sighash byte

2ff43ad

get tx size methods to MmCoin trait

7a92123

get_taker_payment_spend_tx_size -> get_taker_payment_tx_size

225b776

calculate the taker payment spend tx size as per the new protocol

b221def

taker payment spend is actually taker payment here, next commit should fix the naming inconsistencies

fix formatting

481ba3e

mariocynicys force-pushed the proto-upgrade branch from a9dbd14 to 481ba3e Compare May 9, 2024 13:00

laruh requested changes May 20, 2024

View reviewed changes

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_swap/maker_swap_v2.rs Outdated Show resolved Hide resolved

suggestions from aline: use is_zero, parse string instead of float to…

4848cb0

… avoid weird precision

merge with origin/dev

b6ee698

fix tests not compiling after merge

4f5a04a

shamardy reviewed Jul 1, 2024

View reviewed changes

mariocynicys added 12 commits July 2, 2024 20:07

claimation -> claiming

130fc88

remove unused method

ec3ffe3

move the fixme to the appropriate place

9cb9818

also changed to todo

set preimage & maker payment fee as a todo

c86bade

remove fixme about the locktime used for btc

a2e4bfa

revising the commit history, looks like this was deliberate

sighashtype is encoded as four bytes even tho it's just one

c383862

ref. https://bitcoin.stackexchange.com/questions/48108/why-are-sighash-flags-signed-as-4-bytes-when-only-1-byte-is-included-in-the-tran

move the forkid fixme into a todo for later

a08a981

cap fee to the one instruced by taker, allow using less

ba0037f

implement other fee calculation methods

c970e25

use get_taker_payment_fee instead of const size get_htlc_spend_fee

0ea4f0c

use a more idiomatic syntax for func generic bounds

db11ed9

broadcast every 30s instead of 600s

28fc442

mariocynicys force-pushed the proto-upgrade branch from c68b87a to 28fc442 Compare July 5, 2024 14:29

mariocynicys added 3 commits July 29, 2024 11:42

add a missing fixme

638d90e

always rename discuss markers to fixmes since i forget about them xD

merge with origin/dev

bc32198

mariocynicys force-pushed the proto-upgrade branch from b16f050 to bc32198 Compare August 14, 2024 16:08

shamardy mentioned this pull request Aug 23, 2024

feat(utxo-swap): add utxo burn output for non-kmd taker fee #2112

Open

11 tasks

merge with origin/dev

241af5a

mariocynicys requested a review from laruh October 14, 2024 13:18

mariocynicys requested a review from shamardy October 26, 2024 16:21

merge with dev

100951b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(trading-proto-upgrade): fees fixes among other things #2109

feat(trading-proto-upgrade): fees fixes among other things #2109

mariocynicys commented May 6, 2024

shamardy commented May 6, 2024

laruh commented May 7, 2024

shamardy May 7, 2024

shamardy May 7, 2024

laruh left a comment

laruh commented May 31, 2024

laruh commented Jun 18, 2024

shamardy left a comment

shamardy Jul 1, 2024

shamardy Jul 1, 2024

mariocynicys Jul 2, 2024

shamardy Jul 1, 2024

mariocynicys Jul 2, 2024

shamardy Jul 1, 2024

shamardy Jul 1, 2024

mariocynicys Jul 2, 2024

shamardy Jul 3, 2024

laruh commented Nov 11, 2024

	/// Get fee to be paid by receiver per whole swap and check if the wallet has sufficient balance to pay the fee.
	fn get_receiver_trade_fee(&self, stage: FeeApproxStage) -> TradePreimageFut<TradeFee>;

		// expected fee knowing that the maker will always accept incurring the difference.
		calculated_fee.max(taker_instructed_fee)

feat(trading-proto-upgrade): fees fixes among other things #2109

Are you sure you want to change the base?

feat(trading-proto-upgrade): fees fixes among other things #2109

Conversation

mariocynicys commented May 6, 2024

shamardy commented May 6, 2024

laruh commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

laruh commented May 31, 2024

laruh commented Jun 18, 2024

shamardy 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

laruh commented Nov 11, 2024