doc: Add method comments in coinselection #21269

pull cwoodard111 wants to merge 1 commits into bitcoin:master from cwoodard111:patch-1 changing 1 files +4 −3
  1. cwoodard111 commented at 3:13 PM on February 22, 2021: none

    added comments for the insert method and the eligibleforspending method

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. add method comments
    added comments for the insert method and the eligibleforspending method
    4eaae7570f
  3. DrahtBot added the label Wallet on Feb 22, 2021
  4. in src/wallet/coinselection.cpp:38 in 4eaae7570f 
      34 | @@ -35,6 +35,7 @@ struct {
  35 |   * inputs, plus the amount the selection exceeds the spending target:
  36 |   *
  37 |   * waste = selectionTotal - target + inputs × (currentFeeRate - longTermFeeRate)
  38 | + *
    laanwj commented at 8:57 AM on February 23, 2021:

    Please don't add an empty line here. Also there is trailing space on this line tripping up the linter.

    cwoodard111 commented at 12:30 AM on March 19, 2021:

    ah sorry that was my IDE

  5. in src/wallet/coinselection.cpp:162 in 4eaae7570f 
     158 | @@ -158,7 +159,7 @@ bool SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& target_v
 159 |          return false;
 160 |      }
 161 |  
 162 | -    // Set output set
 163 | +    // Set the input set
    laanwj commented at 8:59 AM on February 23, 2021:

    This change looks incorrect: it's a set of UTXOs (outputs) Also, another trailing space

  6. in src/wallet/coinselection.cpp:304 in 4eaae7570f 
     299 | @@ -299,7 +300,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 300 |   OutputGroup
 301 |  
 302 |   ******************************************************************************/
 303 | -
 304 | +// adds coin to the output group
 305 |  void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants, bool positive_only) {
    laanwj commented at 9:01 AM on February 23, 2021:

    Method comments should be in the class {} where they are declared, not where they are implemented. In this case here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/coinselection.h#L92

    Also, please use the Doxygen format for these comments so that they will appear in HTML documentation. Same comment for EligibleForSpending below

    /**
 * Adds coin to the output group.
 */
  7. laanwj renamed this:
    add method comments
    doc: Add method comments in coinselection
    on Feb 23, 2021
  8. laanwj added the label Docs on Feb 23, 2021
  9. in src/wallet/coinselection.cpp:335 in 4eaae7570f 
     331 | @@ -331,7 +332,7 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size
 332 |      // coin itself; thus, this value is counted as the max, not the sum
 333 |      m_descendants = std::max(m_descendants, descendants);
 334 |  }
 335 | -
 336 | +// checks if eligible for spending using the coinelibibilityfilter
    brunoerg commented at 12:35 AM on March 13, 2021:

    Is this necessary? I think it's redundant since the name of the function is self explanatory. bool OutputGroup::EligibleForSpending

  10. fanquake commented at 8:32 AM on April 1, 2021: member

    Thanks, however I'm going to close this. PRs like this one should really be opened in a ready-to-merge state. This one has multiple issues. I also think the value add of some of these comments, which are essentially just the method name, is fairly low. If you'd like to continue contributing I'd suggest checking out the good first issue list.

  11. fanquake closed this on Apr 1, 2021
  12. DrahtBot locked this on Aug 16, 2022
Contributors
cwoodard111laanwjbrunoergfanquake
Labels
DocsWallet
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-21 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me