New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport bitcoin#10637 (partial) #3878
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't f950981 be called "More of 10637..."?
Have CInputCOin store effective value information. This includes the effective value itself, the fee, and the long term fee for the input
Create a new file for coin selection logic and implement the BnB algorithm in it.
Instead of specifying 3 parameters, use a struct for those parameters in order to reduce the number of arguments to SelectCoinsMinConf.
Changes CInputCoin to coinselection and to use CTransactionRef in order to avoid a circular dependency. Also moves other coin selection specific variables out of wallet.h to coinselectoin.h
Moves the current coin selection algorithm out of SelectCoinsMinConf and puts it in coinselection.{cpp,h}. The new function, KnapsackSolver, instead of taking a vector of COutputs, will take a vector of CInputCoins that is prepared by SelectCoinsMinConf.
…t (partial) Allows SelectCoinsMinConf and SelectCoins be able to switch between using BnB or Knapsack for choosing coins. Has SelectCoinsMinConf do the preprocessing necessary to support either BnB or Knapsack. This includes calculating the filtering the effective values for each input. Uses BnB in CreateTransaction to find an exact match for the output. If BnB fails, it will fallback to the Knapsack solver. Dash specific note: just always use Knapsack in CreateTransaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
* Calculate and store the number of bytes required to spend an input * Store effective value, fee, and long term fee in CInputCoin Have CInputCOin store effective value information. This includes the effective value itself, the fee, and the long term fee for the input * Implement Branch and Bound coin selection in a new file Create a new file for coin selection logic and implement the BnB algorithm in it. * Move output eligibility to a separate function * Use a struct for output eligibility Instead of specifying 3 parameters, use a struct for those parameters in order to reduce the number of arguments to SelectCoinsMinConf. * Remove coinselection.h -> wallet.h circular dependency Changes CInputCoin to coinselection and to use CTransactionRef in order to avoid a circular dependency. Also moves other coin selection specific variables out of wallet.h to coinselectoin.h * Add tests for the Branch and Bound algorithm * Move current coin selection algorithm to coinselection.{cpp,h} Moves the current coin selection algorithm out of SelectCoinsMinConf and puts it in coinselection.{cpp,h}. The new function, KnapsackSolver, instead of taking a vector of COutputs, will take a vector of CInputCoins that is prepared by SelectCoinsMinConf. * Move original knapsack solver tests to coinselector_tests.cpp * Add a GetMinimumFeeRate function which is wrapped by GetMinimumFee * Have SelectCoinsMinConf and SelectCoins use BnB or Knapsack and use it (partial) Allows SelectCoinsMinConf and SelectCoins be able to switch between using BnB or Knapsack for choosing coins. Has SelectCoinsMinConf do the preprocessing necessary to support either BnB or Knapsack. This includes calculating the filtering the effective values for each input. Uses BnB in CreateTransaction to find an exact match for the output. If BnB fails, it will fallback to the Knapsack solver. Dash specific note: just always use Knapsack in CreateTransaction. * Benchmark BnB in the worst case where it exhausts * Add a test to make sure that negative effective values are filtered * More of 12747: Fix typos Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Backports bitcoin#10637
This one is pretty complex, backported it commit-by-commit. It's partial because I omitted most of the changes in
CWallet::CreateTransaction
(see 5137de5, original commit bitcoin@6a34ff5), so bnb is not actually used there atm and the main reason to backport it is simply to avoid future merge conflicts everywhere else.