Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce BSQ fee estimation constant for segwit #5450

Merged
merged 2 commits into from May 17, 2021
Merged

reduce BSQ fee estimation constant for segwit #5450

merged 2 commits into from May 17, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 29, 2021

Fee estimation for Maker/Taker tx is done using the BTC wallet. If BSQ is being burnt, a constant is added to represent the size of adding a BSQ input (2 inputs, 3 outputs tx).

  • For legacy BSQ this was 150 vBytes which would result typically in a 325 vByte tx.
  • For segwit BSQ we can reduce the constant to 65 which will result typically in a 240 vByte tx.

The number 65 was calculated by figuring the vsize of an offer tx both pre and post Segwit BSQ wallet functionality.
It was verified by checking the estimated vsize and the resultant transaction's vsize after applying the code change.

One caveat with this solution is that Make/Take offer using legacy BSQ UTXO will be estimated 25% low because the fee estimation code is not aware of BSQ wallet details.

I leave it up to the maintainers whether they think this approach is acceptable or not.
If it is too risky, perhaps scaling-in the reduction could be a compromise? (Possibly based on the block height)

@@ -58,7 +58,7 @@
public static int TYPICAL_TX_WITH_1_INPUT_VSIZE = 175;
private static int DEPOSIT_TX_VSIZE = 233;

private static int BSQ_INPUT_INCREASE = 150;
private static int BSQ_INPUT_INCREASE = 65;
Copy link
Member

@sqrrm sqrrm Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be too low. I just had a look at this and the table I came up with was, to be on the safe side

Tx size:

Element size (vbyte)
Overhead 10
Inputs 41 * N
Outputs 31 * N_sw + 34 * N_p2pkh
Sig p2pkh 108
Sig SW 29

So I think the BSQ sw input should be counted as 41 + 29 = 70 vbytes extra, although it's sometimes a bit smaller.

Got inspiration from here https://bitcoinops.org/en/tools/calc-size/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

70 sounds good. Do you have any thoughts on the caveat (legacy BSQ utxo) described above?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the atomic protocol I use this code to calculate the size. It overestimates a little bit due to sigs sometimes being a byte shorter, but otherwise quite accurate I think. This works since each side is only paying for their own and known inputs and outputs. Not sure if this will work for the normal trade protocol.

        // Overhead
        var size = payForOverhead ? 10L : 0L;

        // Inputs and signatures
        for (var input : parent.tx.getInputs()) {
            size += 41 + (input.hasWitness() ? 29 : 108);
        }

        // Outputs
        for (var output : parent.tx.getOutputs()) {
            size += ScriptPattern.isP2WPKH(output.getScriptPubKey()) ? 31 : 34;
        }

        return txFeePerVbyte.multiply(size);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the Bisq code which does fee estimation has no visibility into the utxo being used. BitcoinJ is used for that and it also has no visibility into the BSQ side of things, hence that constant BSQ_INPUT_INCREASE.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, so for now I think this is fine. It's also the taker that pays for the whole tx which makes even worse to get an accurate calculation.

Perhaps we should wait with this change and add in the next release when more BSQ utxos are segwit. If included right now it would guarantee to be wrong in the beginning.

@ghost
Copy link
Author

ghost commented Apr 30, 2021

Showing how the change affects the dynamic between using BTC vs BSQ for overall fees. Currently, for small offers it is only economical to use BSQ when mempool fees are < 30 sat/vB.

image

 
After this PR it would be economical to use BSQ when the mempool fees are < 60 sat/vb.

image

@ripcurlx ripcurlx added this to the v1.6.4 milestone Apr 30, 2021
@ripcurlx
Copy link
Contributor

As we are having already 76+% running on v1.6.4 I think it is fine to merge this for the upcoming release.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@ripcurlx ripcurlx merged commit ee6ccc4 into bisq-network:master May 17, 2021
@ghost ghost mentioned this pull request Jun 5, 2021
@ghost ghost deleted the bsq_fee_estimation branch May 29, 2022 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants