Always log debug information for fee calculation in CreateTransaction by morcos · Pull Request #10284 · bitcoin/bitcoin

morcos · 2017-04-26T20:50:21Z

This is a first pass at always logging a single line of information whenever CreateTransaction is invoked which will help debug any problems in fee calculation or estimation.

Built on top of #10199 and #10283, only the last commit is new.

jonasschnelli · 2017-04-27T06:46:57Z

src/wallet/feebumper.cpp

Would logging the fee estimation details during bumpfee (all call to GetMinimumFee) be possible?

I think this is possible, but it wasn't clear to me we wanted it here.
Perhaps that could be added later

jonasschnelli · 2017-04-27T06:47:21Z

src/wallet/wallet.cpp

supernit: use brackets if (feeCalc) { feeCalc->reason = FeeReason::FALLBACK; }

No. From https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md:

If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.

Okay. Fair point.

morcos · 2017-05-18T21:23:24Z

rebased

laanwj · 2017-05-22T17:44:32Z

src/policy/fees.h

 };

+/* Enumeration of reason for returned fee estimate */
+enum FeeReason {


shouldn't this be "enum class" to avoid the enum values from leaking into global scope?

laanwj · 2017-05-22T17:45:16Z

src/wallet/wallet.cpp

        }
    }
+
+    LogPrintf("Fee Calculation: Fee:%s Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",


Nice! That should give a lot of info for troubleshooting this.

ryanofsky

utACK d0fb0f6, though needs rebase (conflicted with #10390).

ryanofsky · 2017-05-24T15:29:57Z

src/wallet/wallet.cpp

        }
    }
+
+    LogPrintf("Fee Calculation: Fee:%s Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",


Should Fee:%s be Fee:%d? (maybe it doesn't matter)

ryanofsky · 2017-05-24T15:39:15Z

src/qt/sendcoinsdialog.cpp

                                                                std::max(feeRate.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB");
        ui->labelSmartFee2->hide();
-        ui->labelFeeEstimation->setText(tr("Estimated to begin confirmation within %n block(s).", "", estimateFoundAtBlocks));
+        ui->labelFeeEstimation->setText(tr("Estimated to begin confirmation within %n block(s).", "", feeCalc.desiredTarget));


Should this be returnedTarget?

damn, i made that same bug elsewhere. thx

ryanofsky · 2017-05-24T15:40:23Z

src/policy/fees.h

+/* Enumeration of reason for returned fee estimate */
+enum FeeReason {
+    NONE = 0,
+    HALF_ESTIMATE = 1,


Could drop all these = 1, = 2.

ryanofsky · 2017-05-24T15:51:47Z

src/policy/fees.h

+    MAXTXFEE = 9
+};
+
+static std::map<FeeReason, std::string> FeeReasonString = {


Should make this const. Also maybe prefer to declare this as a const char*[] array, or as a function mapping enum values to strings, or as a function returning a static map. Global objects like this have constructors that need to run before main() and will cumulatively slow down program startup. Also because this is a static global in a header file, the constructor will run once for every .cpp file that includes this.

jnewbery · 2017-06-08T20:27:56Z

src/wallet/wallet.cpp

        }
    }
+
+    LogPrintf("Fee Calculation: Fee:%s Bytes:%u Tgt:%d (requested %d) Reason:\"%s\" Decay %.5f: Estimation: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out) Fail: (%g - %g) %.2f%% %.1f/(%.1f %d mem %.1f out)\n",


Should this be LogPrint(BCLog::ESTIMATEFEE... (ie only log if estimatefee logs are enabled)?

It's meant to get logged even if you have not selected any debug log categories

morcos · 2017-06-08T20:51:13Z

Addressed comments and squashed/rebased
debugEstimates.ver2 --> debugEstimates.ver2.squash --> debugEstimates.ver2.rebased

ryanofsky

utACK 436207e. Looks good. Changes since previous review were rebase, cleaned up feereason enum/strings, fixed ui target string.

ryanofsky · 2017-06-12T20:55:34Z

src/policy/fees.h

+    MAXTXFEE,
+};
+
+const std::string StringForFeeReason(FeeReason reason);


I don't think there's any benefit to making this const (cases where you do want this are pretty rare https://stackoverflow.com/questions/8716330/purpose-of-returning-by-const-value).

Const is actually still here (you removed it from the definition but not declaration)

morcos · 2017-06-13T14:15:12Z

addressed nit

ryanofsky

utACK 7c287a7b5df6f33f3b7723496590e72cdd7fed2e. Only change is removed const.

ryanofsky · 2017-06-13T14:52:03Z

src/policy/fees.h

+    MAXTXFEE,
+};
+
+const std::string StringForFeeReason(FeeReason reason);


Const is actually still here (you removed it from the definition but not declaration)

morcos · 2017-06-13T16:36:17Z

oops, actually addressed nit this time.

ryanofsky

utACK 1bebfc8, const is gone

laanwj · 2017-06-15T12:20:12Z

utACK 1bebfc8

…ateTransaction 1bebfc8 Output Fee Estimation Calculations in CreateTransaction (Alex Morcos) Tree-SHA512: e25a27f7acbbc3a666d5d85da2554c5aaec4c923ee2fdbcfc532c29c6fbdec3c9e0d6ae6044543ecc339e7bd81df09c8d228e0b53a2c5c2dae0f1098c9453272

…0284 cc0ed26 Supress struct/class mismatch warnings introduced in #10284. (Pavel Janík) Tree-SHA512: 16a6870401b5227c276931841f188479ed5960cf38d8e685f222f58550744c9fcf96a2ea3f2be9a0b1a8d0856a802fc4ec38df7bf90cd5de1f3fe20c4ca15b9d

… in CreateTransaction 1bebfc8 Output Fee Estimation Calculations in CreateTransaction (Alex Morcos) Tree-SHA512: e25a27f7acbbc3a666d5d85da2554c5aaec4c923ee2fdbcfc532c29c6fbdec3c9e0d6ae6044543ecc339e7bd81df09c8d228e0b53a2c5c2dae0f1098c9453272

…d in bitcoin#10284 cc0ed26 Supress struct/class mismatch warnings introduced in bitcoin#10284. (Pavel Janík) Tree-SHA512: 16a6870401b5227c276931841f188479ed5960cf38d8e685f222f58550744c9fcf96a2ea3f2be9a0b1a8d0856a802fc4ec38df7bf90cd5de1f3fe20c4ca15b9d

fanquake added TX fees and policy Wallet labels Apr 26, 2017

jonasschnelli reviewed Apr 27, 2017

View reviewed changes

morcos force-pushed the debugEstimates branch from 6c99d77 to d0fb0f6 Compare May 18, 2017 21:23

laanwj reviewed May 22, 2017

View reviewed changes

ryanofsky reviewed May 24, 2017

View reviewed changes

jnewbery reviewed Jun 8, 2017

View reviewed changes

morcos force-pushed the debugEstimates branch from d0fb0f6 to 436207e Compare June 8, 2017 20:45

ryanofsky reviewed Jun 12, 2017

View reviewed changes

morcos force-pushed the debugEstimates branch from 436207e to 7c287a7 Compare June 13, 2017 14:14

ryanofsky reviewed Jun 13, 2017

View reviewed changes

Output Fee Estimation Calculations in CreateTransaction

1bebfc8

morcos force-pushed the debugEstimates branch from 7c287a7 to 1bebfc8 Compare June 13, 2017 16:35

ryanofsky reviewed Jun 13, 2017

View reviewed changes

laanwj merged commit 1bebfc8 into bitcoin:master Jun 15, 2017

paveljanik added a commit to paveljanik/bitcoin that referenced this pull request Jun 15, 2017

Supress struct/class mismatch warnings introduced in bitcoin#10284.

cc0ed26

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

morcos commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jun 8, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jun 13, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jun 13, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sipa Apr 27, 2017 •

edited

Loading