Refactor fee estimation #2251

ManfredKarrer · 2019-01-11T22:24:32Z

Danger zone ;-)
This needs some good testing and careful review.

- feeTxSizeEstimationRecursionCounter was never reset, so that caused probably bugs by ignoring fee estimation after it was called >10 times. The default size was used then so the bug was not very obvious as long the tx size was not very large.

blabno · 2019-01-14T10:08:39Z

core/src/main/java/bisq/core/btc/TxFeeEstimationService.java

+
+        if (!preferences.isPayFeeInBtc()) {
+            // If we pay the fee in BSQ we have one input more which adds about 150 bytes
+            estimatedTxSize += 150;


Can we avoid magic numbers? https://en.wikipedia.org/wiki/Magic_number_(programming)

Yes we can use a constant. It is just used once so thats why I did not use one and the class is small so easty to overlook all as well there is a comment decribing the value. But no problem to make a constant for it.

blabno · 2019-01-14T10:24:33Z

core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java

+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+@RunWith(PowerMockRunner.class)


There is no need for PowerMock in this test class.

OK, I will remove it

blabno · 2019-01-14T10:27:46Z

core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java

+        assertEquals(260, result);
+
+
+        // TODO check how to use the mocking framework for repeated calls


This is separate test scenario and should be placed in separate method.
Good naming convention for test methods is testMethodName_scenario_expectedCondition
i.e. getEstimatedTxSize_nullOutputs_throwsException

You refer to the commented out lines, right? Would you mind to have a hands on the test class? More your domain, you know my love for tests is limited ;-)

@blabno I broke them up in 877d0ae and looked again into it. See commit comments... Could u have a look to try to fix it?

blabno · 2019-01-14T10:30:56Z

core/src/main/java/bisq/core/btc/TxFeeEstimationService.java

+        return new Tuple2<>(txFee, size);
+    }
+
+    @VisibleForTesting


I've been always told that this is a bad idea, as it's an attempt to test implementation details.
Either extract it to an utility class that could be tested separately or do not test it individually.

Not clear what you mean. Lets discuss on a call in a bit...

I think @blabno is talking about the fact, that if you need to make something visible for testing that wouldn't be accessible otherwise, is a sign that something should be structured or tested differently 😉

Ah ok. Yes basically agree. In that case I would not like to expose getEstimatedTxSize as it is only interesting for that class. The more high level methods carry much more special case handling that they would be harder to test then the core functionality in getEstimatedTxSize.

@bernard

Set ignore to failing tests @bernard could you have a look and try to get those working? Reason for problem is that repeated calls to getEstimatedFeeTxSize do not work (returns 0 at second call in loop which cause test to fail)

blabno · 2019-01-20T11:28:47Z

core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java

+        realTxSize = 2600;
+
+        txFee = txFeePerByte.multiply(initialEstimatedTxSize);
+        when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize);


do/while loop inside getEstimatedFeeTxSize has 2 cycles because first invocation of isInTolerance returns false.
So for the first iteration we have to mock with initial txFee:

when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFee)).thenReturn(realTxSize);

And for the second iteration we have to mock with txFee multiplied by txFeePerByte.

when(btcWalletService.getEstimatedFeeTxSize(outputValues, txFeePerByte.multiply(realTxSize))).thenReturn(realTxSize)

Alternatively we can mock getEstimatedFeeTxSize to return the same result for any txFee:

when(btcWalletService.getEstimatedFeeTxSize(eq(outputValues), any(Coin.class))).thenReturn(realTxSize);

blabno · 2019-01-20T11:52:16Z

core/src/test/java/bisq/core/btc/TxFeeEstimationServiceTest.java

+        assertEquals(2600, result);
+    }
+
+    // FIXME @Bernard could you have a look?


Same as above

@blabno Thanks. I would prefer to leave that for another PR, too buys with other high prio stuff. If you have time to take that would be great as well!

ManfredKarrer · 2019-01-21T12:10:53Z

@ripcurlx Could you ACK if you have completed review?

ripcurlx

ACK - Just a couple of questions and minor changes.

core/src/main/java/bisq/core/btc/TxFeeEstimationService.java

ripcurlx · 2019-01-18T16:06:13Z

core/src/main/java/bisq/core/btc/TxFeeEstimationService.java

+        return new Tuple2<>(txFee, size);
+    }
+
+    @VisibleForTesting


I think @blabno is talking about the fact, that if you need to make something visible for testing that wouldn't be accessible otherwise, is a sign that something should be structured or tested differently 😉

core/src/main/java/bisq/core/btc/TxFeeEstimationService.java

desktop/src/main/java/bisq/desktop/main/offer/MutableOfferDataModel.java

ripcurlx · 2019-01-23T09:00:55Z

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java

        } else {
-            feeTxSize = 320;
-            txFeeFromFeeService = getTxFeeBySize(feeTxSize);
+            feeTxSize = 380;


Why did this change from 320?

In the TxFeeEstimationService we use as min. the payout tx size (380), so that reflects the same logic. It is to be more on the safe side and prefer to overpay the other txs and not underpay the payout tx.

From: TxFeeEstimationService
int averageSize = (estimatedTxSize + DEPOSIT_TX_SIZE) / 2; // deposit tx has about 320 bytes
// We use at least the size of the payout tx to not underpay at payout.
size = Math.max(PAYOUT_TX_SIZE, averageSize);

desktop/src/main/java/bisq/desktop/main/offer/takeoffer/TakeOfferDataModel.java

ManfredKarrer · 2019-01-23T12:44:21Z

No idea why tests fail, the EditOfferDataModelTest does not exist. Seems a caching issue.

ManfredKarrer added 5 commits January 11, 2019 23:20

Add TxFeeEstimationService

d876cc1

Add TxFeeEstimationServiceTest

76dd6b5

Add getEstimatedFeeTxSize method

274beef

Remove estimateBtcTradingFeeTxSize

ba114cb

Use TxFeeEstimationService

d09296e

- feeTxSizeEstimationRecursionCounter was never reset, so that caused probably bugs by ignoring fee estimation after it was called >10 times. The default size was used then so the bug was not very obvious as long the tx size was not very large.

ManfredKarrer requested review from ripcurlx and blabno January 11, 2019 22:24

Fix missing param in test class

28d0db8

ripcurlx added in:gui a:refactoring labels Jan 14, 2019

blabno reviewed Jan 14, 2019

View reviewed changes

ManfredKarrer added 3 commits January 14, 2019 14:33

Remove PowerMockRunner

645719c

Use constants for tx size values

461dd1d

Break up test method

877d0ae

Set ignore to failing tests @bernard could you have a look and try to get those working? Reason for problem is that repeated calls to getEstimatedFeeTxSize do not work (returns 0 at second call in loop which cause test to fail)

ManfredKarrer assigned ripcurlx Jan 14, 2019

Add comment to give more background about the fee estimation

ed8df87

blabno reviewed Jan 20, 2019

View reviewed changes

ripcurlx approved these changes Jan 23, 2019

View reviewed changes

ManfredKarrer added 3 commits January 23, 2019 12:32

Improve comment

250c7c1

Fix typos

64f65e0

Make fields private

90e95e5

ManfredKarrer merged commit 200c90b into bisq-network:master Jan 23, 2019

ManfredKarrer deleted the refactor-fee-estimation branch January 28, 2019 17:50

ripcurlx mentioned this pull request Jan 30, 2019

For January 2019 bisq-network/compensation#206

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor fee estimation #2251

Refactor fee estimation #2251

ManfredKarrer commented Jan 11, 2019

blabno Jan 14, 2019

ManfredKarrer Jan 14, 2019

blabno Jan 14, 2019

ManfredKarrer Jan 14, 2019

blabno Jan 14, 2019

ManfredKarrer Jan 14, 2019

ManfredKarrer Jan 14, 2019

blabno Jan 14, 2019

ManfredKarrer Jan 14, 2019

ripcurlx Jan 18, 2019

ManfredKarrer Jan 23, 2019

blabno Jan 20, 2019

blabno Jan 20, 2019

ManfredKarrer Jan 21, 2019

ManfredKarrer commented Jan 21, 2019

ripcurlx left a comment

ripcurlx Jan 18, 2019

ripcurlx Jan 23, 2019

ManfredKarrer Jan 23, 2019

ManfredKarrer commented Jan 23, 2019

		assertEquals(260, result);


		// TODO check how to use the mocking framework for repeated calls

Refactor fee estimation #2251

Refactor fee estimation #2251

Conversation

ManfredKarrer commented Jan 11, 2019

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

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

ManfredKarrer commented Jan 21, 2019

ripcurlx 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

ManfredKarrer commented Jan 23, 2019