Docs: [policy] Remove outdated confusing comment #24305

Xekyo commented at 9:18 PM on February 9, 2022: member

This comment described how the constructor of CFeeRate was previously indirectly used to parse fee rate arguments from RPCs. The command line input was actually in sat/vB but due to the use of AmountFromValue() it got converted to BTC/vB which then got rectified in the constructor by creating a CFeeRate from that given value and COIN as the transaction size. Since this usage pattern was removed from the codebase some months ago, the comment is now obsolete.

fanquake added the label Docs on Feb 9, 2022

MarcoFalke renamed this:
~~Docs: Remove outdated confusing comment~~
Docs: [policy] Remove outdated confusing comment
on Feb 10, 2022

MarcoFalke approved

MarcoFalke commented at 7:44 AM on February 10, 2022: member

LGTM

in src/policy/feerate.h:27 in bdc35cf75e outdated

  23 | @@ -24,12 +24,13 @@ enum class FeeEstimateMode {
  24 |  };
  25 |  
  26 |  /**
  27 | - * Fee rate in satoshis per kilobyte: CAmount / kB
  28 | + * Fee rate in satoshis per kilovirtualbyte: CAmount / kvB

laanwj commented at 1:35 PM on February 10, 2022:

fwiw this is the first use of the word "kilovirtualbyte", do we use another term normally ?

Xekyo commented at 8:01 PM on February 10, 2022:

I think the codebase just never actually got updated to clarify where it uses weight-based fee rates.

laanwj commented at 8:40 AM on February 11, 2022:

sounds good to me then, i don't mind using it here, just think unit consistency is important so was worried for a bit

Xekyo commented at 9:21 PM on February 14, 2022:

Amended all mentions of size and bytes in this file to use vsize and vbytes.

in src/policy/feerate.h:46 in bdc35cf75e outdated

  45 | +    /**
  46 | +     * Construct a fee rate from a fee in satoshis and a vsize in vB.
  47 |       *
  48 | -     *  Passing a num_bytes value of COIN (1e8) returns a fee rate in satoshis per vB (sat/vB),
  49 | -     *  e.g. (nFeePaid * 1e8 / 1e3) == (nFeePaid / 1e5),
  50 | -     *  where 1e5 is the ratio to convert from BTC/kvB to sat/vB.

jonatack commented at 10:15 AM on February 11, 2022:

These 3 lines of docs were requested in Nov 2020 review feedback to clarify passing COIN to the CFeeRate ctor to obtain a fee rate in sat/vB, like cc.m_feerate = CFeeRate(AmountFromValue(fee_rate), COIN);

Since the change in https://github.com/bitcoin/bitcoin/pull/21786/files#diff-63c7710d72381cffe692175dd70e47f1891bf75c3e09249ed826f9df3917d02dL219 we no longer do this, so ACK on removing this documentation now.

Some ideas if you agree and retouch (do the brackets around [sat/kvB] have a special meaning in doxygen?)

@@ -29,7 +29,7 @@ enum class FeeEstimateMode {
 class CFeeRate
 {
 private:
-    /** fee rate in [sat/kvB] (satoshis per 1000 virtualbytes) */
+    /** Fee rate in sat/kvB (satoshis per 1000 virtualbytes) */
     CAmount nSatoshisPerK;
 
 public:
@@ -44,8 +44,8 @@ public:
     /**
      * Construct a fee rate from a fee in satoshis and a vsize in vB.
      *
-     * param@[in]   nFeePaid    The fee paid by a transaction in satoshis
-     * param@[in]   num_bytes   The vsize of a transaction in vbytes
+     * param@[in]   nFeePaid    The fee paid by a transaction, in satoshis
+     * param@[in]   num_bytes   The vsize of a transaction, in vbytes
      */
     CFeeRate(const CAmount& nFeePaid, uint32_t num_bytes);

</p></details>

laanwj commented at 12:15 PM on February 11, 2022:

square brackets are often used around unit indicators (at least in table headers), i would guess it's an idea like that? some amusing discussion: https://hsm.stackexchange.com/questions/5907/old-square-bracket-notation-for-units

i'm fine with dropping them, to be clear

Xekyo commented at 9:20 PM on February 14, 2022:

Dropped the square brackets.

jonatack commented at 10:22 AM on February 11, 2022: member

ACK bdc35cf75efdfd866

DrahtBot commented at 11:54 AM on February 11, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23633 (Make CFeeRate work with uint64_t sizes by kiminuo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Xekyo force-pushed on Feb 14, 2022

Xekyo commented at 3:24 PM on February 14, 2022: member

Thanks, I've updated the PR to drop the square brackets and amend the commit message. I've also added the commas as suggested.

Xekyo force-pushed on Feb 14, 2022

jonatack commented at 5:00 PM on February 14, 2022: member

ACK

WDYT about the following, while here

 private:
-    /** fee rate in sat/kvB (satoshis per 1000 virtualbytes) */
+    /** Fee rate in sat/kvB (satoshis per 1000 virtualbytes) */
     CAmount nSatoshisPerK;
 
 public:
-    /** Fee rate of 0 satoshis per kB */
+    /** Fee rate of 0 satoshis per kvB */
     CFeeRate() : nSatoshisPerK(0) { }
 
     /**
      * Return the fee in satoshis for the given size in bytes.
-     * If the calculated fee would have fractional satoshis, then the returned fee will always be rounded up to the nearest satoshi.
+     * If the calculated fee would have fractional satoshis, then the
+     * returned fee will always be rounded up to the nearest satoshi.
      */
     CAmount GetFee(uint32_t num_bytes) const;

</p></details>

Remove outdated comment on CFeeRate

This comment described how the constructor of CFeeRate was previously
indirectly used to parse fee rate arguments from RPCs. The command line
input was actually in sat/vB but due to the use of AmountFromValue() it
got converted to BTC/vB. In the constructor this could be rectified by
creating a CFeeRate from that given value (in BTC/vB) and COIN as the
transaction size, turning the input effectively to sat/vB. Since this
usage pattern was removed from the codebase some months ago, the comment
is now obsolete.

Also:
• Use vsize and vbyte instead of size and byte

e50a9be154

Xekyo force-pushed on Feb 14, 2022

Xekyo commented at 9:03 PM on February 14, 2022: member

Sure, @jonatack, let's do it.

jonatack commented at 1:37 PM on February 15, 2022: member

ACK e50a9be1540c769a99fcdc1f7a109a6bf1c7516b

michaelfolkson commented at 1:51 PM on February 15, 2022: contributor

ACK e50a9be1540c769a99fcdc1f7a109a6bf1c7516b

I'm not sure about using the term vsize. As it isn't a measurement unit (e.g. kvB) and is just words I'd have thought virtual size would suffice. But it is nitty and happy to be ignored.

MarcoFalke merged this on Feb 22, 2022

MarcoFalke closed this on Feb 22, 2022

sidhujag referenced this in commit 401c9b1e67 on Feb 22, 2022

DrahtBot locked this on Feb 22, 2023