Always log debug information for fee calculation in CreateTransaction #10284

pull morcos wants to merge 1 commits into bitcoin:master from morcos:debugEstimates changing 7 files +134 −44
  1. morcos commented at 8:50 PM on April 26, 2017: member

    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.

  2. fanquake added the label TX fees and policy on Apr 26, 2017
  3. fanquake added the label Wallet on Apr 26, 2017
  4. in src/wallet/feebumper.cpp:170 in 6c99d77d20 outdated 
     159 | @@ -160,7 +160,7 @@ CFeeBumper::CFeeBumper(const CWallet *pWallet, const uint256 txidIn, int newConf
 160 |      } else {
 161 |          // if user specified a confirm target then don't consider any global payTxFee
 162 |          if (specifiedConfirmTarget) {
 163 | -            nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, ::feeEstimator, true);
 164 | +            nNewFee = CWallet::GetMinimumFee(maxNewTxSize, newConfirmTarget, mempool, ::feeEstimator, nullptr, true);
    jonasschnelli commented at 6:46 AM on April 27, 2017:

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

    morcos commented at 4:49 PM on June 8, 2017:

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

  5. in src/wallet/wallet.cpp:2905 in 6c99d77d20 outdated 
    2799 | +        nFeeNeeded = estimator.estimateSmartFee(nConfirmTarget, feeCalc, pool, true).GetFee(nTxBytes);
2800 |          // ... unless we don't have enough mempool data for estimatefee, then use fallbackFee
2801 | -        if (nFeeNeeded == 0)
2802 | +        if (nFeeNeeded == 0) {
2803 |              nFeeNeeded = fallbackFee.GetFee(nTxBytes);
2804 | +            if (feeCalc) feeCalc->reason = FeeReason::FALLBACK;
    jonasschnelli commented at 6:47 AM on April 27, 2017:

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

    sipa commented at 11:31 AM on April 27, 2017:

    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.

    jonasschnelli commented at 11:35 AM on April 27, 2017:

    Okay. Fair point.

  6. morcos force-pushed on May 18, 2017
  7. morcos commented at 9:23 PM on May 18, 2017: member

    rebased

  8. in src/policy/fees.h:78 in d0fb0f69b9 outdated 
      73 | @@ -74,6 +74,33 @@ enum FeeEstimateHorizon {
  74 |      LONG_HALFLIFE = 2
  75 |  };
  76 |  
  77 | +/* Enumeration of reason for returned fee estimate */
  78 | +enum FeeReason {
    laanwj commented at 5:44 PM on May 22, 2017:

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

  9. in src/wallet/wallet.cpp:2739 in d0fb0f69b9 outdated 
    2734 | @@ -2734,6 +2735,15 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
2735 |              return false;
2736 |          }
2737 |      }
2738 | +
2739 | +    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",
    laanwj commented at 5:45 PM on May 22, 2017:

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

    ryanofsky commented at 3:29 PM on May 24, 2017:

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

    jnewbery commented at 8:27 PM on June 8, 2017:

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

    morcos commented at 8:46 PM on June 8, 2017:

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

  10. in src/qt/sendcoinsdialog.cpp:682 in d0fb0f69b9 outdated 
     678 | @@ -679,7 +679,7 @@ void SendCoinsDialog::updateSmartFeeLabel()
 679 |          ui->labelSmartFee->setText(BitcoinUnits::formatWithUnit(model->getOptionsModel()->getDisplayUnit(),
 680 |                                                                  std::max(feeRate.GetFeePerK(), CWallet::GetRequiredFee(1000))) + "/kB");
 681 |          ui->labelSmartFee2->hide();
 682 | -        ui->labelFeeEstimation->setText(tr("Estimated to begin confirmation within %n block(s).", "", estimateFoundAtBlocks));
 683 | +        ui->labelFeeEstimation->setText(tr("Estimated to begin confirmation within %n block(s).", "", feeCalc.desiredTarget));
    ryanofsky commented at 3:39 PM on May 24, 2017:

    Should this be returnedTarget?

    morcos commented at 4:08 PM on May 24, 2017:

    damn, i made that same bug elsewhere. thx

  11. in src/policy/fees.h:80 in d0fb0f69b9 outdated 
      73 | @@ -74,6 +74,33 @@ enum FeeEstimateHorizon {
  74 |      LONG_HALFLIFE = 2
  75 |  };
  76 |  
  77 | +/* Enumeration of reason for returned fee estimate */
  78 | +enum FeeReason {
  79 | +    NONE = 0,
  80 | +    HALF_ESTIMATE = 1,
    ryanofsky commented at 3:40 PM on May 24, 2017:

    Could drop all these = 1, = 2.

  12. in src/policy/fees.h:91 in d0fb0f69b9 outdated 
      86 | +    FALLBACK = 7,
  87 | +    REQUIRED = 8,
  88 | +    MAXTXFEE = 9
  89 | +};
  90 | +
  91 | +static std::map<FeeReason, std::string> FeeReasonString = {
    ryanofsky commented at 3:51 PM on May 24, 2017:

    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.

  13. ryanofsky commented at 4:07 PM on May 24, 2017: member

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

  14. morcos force-pushed on Jun 8, 2017
  15. morcos commented at 8:51 PM on June 8, 2017: member

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

  16. in src/policy/fees.h:91 in 436207ef83 outdated 
      86 | +    FALLBACK,
  87 | +    REQUIRED,
  88 | +    MAXTXFEE,
  89 | +};
  90 | +
  91 | +const std::string StringForFeeReason(FeeReason reason);
    ryanofsky commented at 8:55 PM on June 12, 2017:

    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).

    morcos commented at 2:15 PM on June 13, 2017:

    done

    ryanofsky commented at 2:52 PM on June 13, 2017:

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

  17. ryanofsky commented at 9:02 PM on June 12, 2017: member

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

  18. morcos force-pushed on Jun 13, 2017
  19. morcos commented at 2:15 PM on June 13, 2017: member

    addressed nit

  20. ryanofsky commented at 2:53 PM on June 13, 2017: member

    utACK 7c287a7b5df6f33f3b7723496590e72cdd7fed2e. Only change is removed const.

  21. Output Fee Estimation Calculations in CreateTransaction 1bebfc8d3a
  22. morcos force-pushed on Jun 13, 2017
  23. morcos commented at 4:36 PM on June 13, 2017: member

    oops, actually addressed nit this time.

  24. ryanofsky commented at 4:48 PM on June 13, 2017: member

    utACK 1bebfc8d3aa615dfdd0221f21b87319e36821b71, const is gone

  25. laanwj commented at 12:20 PM on June 15, 2017: member

    utACK 1bebfc8

  26. laanwj merged this on Jun 15, 2017
  27. laanwj closed this on Jun 15, 2017
  28. laanwj referenced this in commit c2ab38bdd5 on Jun 15, 2017
  29. paveljanik referenced this in commit cc0ed26753 on Jun 15, 2017
  30. sipa referenced this in commit 7a74f88a26 on Jun 16, 2017
  31. PastaPastaPasta referenced this in commit e9bcf4f922 on Jul 17, 2019
  32. PastaPastaPasta referenced this in commit 715635a43a on Jul 17, 2019
  33. PastaPastaPasta referenced this in commit ab49b6414b on Jul 17, 2019
  34. PastaPastaPasta referenced this in commit 1a4e163704 on Jul 17, 2019
  35. PastaPastaPasta referenced this in commit d3108fc5df on Aug 6, 2019
  36. PastaPastaPasta referenced this in commit 3d8a2e175f on Aug 6, 2019
  37. PastaPastaPasta referenced this in commit 63ec500a48 on Aug 6, 2019
  38. PastaPastaPasta referenced this in commit 78e66a558e on Aug 6, 2019
  39. PastaPastaPasta referenced this in commit ad65f28aec on Aug 6, 2019
  40. PastaPastaPasta referenced this in commit 17f1afc7cc on Aug 6, 2019
  41. PastaPastaPasta referenced this in commit 62c7284c2d on Aug 7, 2019
  42. PastaPastaPasta referenced this in commit c8e472c4b2 on Aug 7, 2019
  43. PastaPastaPasta referenced this in commit b2155474eb on Aug 8, 2019
  44. PastaPastaPasta referenced this in commit 9694f6be2d on Aug 8, 2019
  45. PastaPastaPasta referenced this in commit bfa64fd15f on Aug 12, 2019
  46. PastaPastaPasta referenced this in commit 50ec763075 on Aug 12, 2019
  47. barrystyle referenced this in commit be9e15f96f on Jan 22, 2020
  48. barrystyle referenced this in commit bddf0c20af on Jan 22, 2020
  49. DrahtBot locked this on Sep 8, 2021
Contributors
morcosjonasschnellisipalaanwjryanofskyjnewbery
Labels
WalletTX fees and policy
Linked (view graph)
#10199 Better fee estimates#10283 Cleanup: reduce to one GetMinimumFee call signature
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-17 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me