Refactor OutputGroup effective value calculations and filtering to occur within the struct #17458

achow101 commented at 12:06 AM on November 13, 2019: member

Currently, the effective values and filtering for positive effective values is done outside of the OutputGroup. We should instead have functions in Outputgroup to do this and call those for each OutputGroup. So this PR does that.

This makes future changes for effective values in coin selection much easier.

fanquake added the label Wallet on Nov 13, 2019

DrahtBot commented at 2:01 AM on November 13, 2019: member

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

Conflicts

No conflicts as of last run.

DrahtBot added the label Needs rebase on Nov 22, 2019

achow101 force-pushed on Nov 22, 2019

DrahtBot removed the label Needs rebase on Nov 22, 2019

in src/wallet/coinselection.cpp:305 in ef9d0f24aa outdated

 298 | @@ -299,7 +299,7 @@ bool KnapsackSolver(const CAmount& nTargetValue, std::vector<OutputGroup>& group
 299 |  void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants) {
 300 |      m_outputs.push_back(output);
 301 |      m_from_me &= from_me;
 302 | -    m_value += output.effective_value;

instagibbs commented at 7:02 PM on December 6, 2019:

this only isn't a reachable "bug" because before this PR the values are the same, yes?

instagibbs commented at 7:37 PM on December 6, 2019:

hmm actually no, this happens for any OutputGroup, which includes Bnb?

achow101 commented at 7:57 PM on December 6, 2019:

it isn't reachable because the values are the same as the outputs' effective values aren't ever actually to their effective values.

instagibbs commented at 8:03 PM on December 6, 2019:

ah I see, it's being set externally in SelectCoinsMinConf. Ok, this set of changes makes it much easier to understand.

in src/wallet/coinselection.h:41 in d9b985a852 outdated

  35 | @@ -36,6 +36,8 @@ class CInputCoin {
  36 |      COutPoint outpoint;
  37 |      CTxOut txout;
  38 |      CAmount effective_value;
  39 | +    CAmount m_fee;

instagibbs commented at 7:10 PM on December 6, 2019:

This commit makes effective_value a bit ambiguous to the reader now that both short and long-range fees are being stored.

achow101 commented at 8:01 PM on December 6, 2019:

It makes everything easier to precompute and store these since they are used in multiple places.

instagibbs commented at 8:04 PM on December 6, 2019:

I understand the motivation, my nitting is about naming since we're expanding the "scope" of what is being cached.

instagibbs commented at 7:35 PM on December 6, 2019: member

Seems right. The remaining other location that sets effective_value is in SelectCoins for preset inputs.

I'd prefer that every setting was done under OutputGroup but that can be done later.

instagibbs commented at 8:04 PM on December 6, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17458/commits/693cfa66d10c916d4554c7639dc772727248c9fd

in src/wallet/coinselection.cpp:327 in 693cfa66d1 outdated

 317 | @@ -317,6 +318,8 @@ std::vector<CInputCoin>::iterator OutputGroup::Discard(const CInputCoin& output)
 318 |      if (it == m_outputs.end()) return it;
 319 |      m_value -= output.txout.nValue;
 320 |      effective_value -= output.effective_value;
 321 | +    fee -= output.m_fee;
 322 | +    long_term_fee -= output.m_long_term_fee;

fjahr commented at 3:02 PM on March 9, 2020:

Should the reverse not also be added to the Insert method above?

achow101 commented at 3:26 PM on March 9, 2020:

Yes, but no. The OutputGroups are created (and Insert is called) before the fees are set. But Discard is called after the fees are set. So not incrementing fee and long_term_fee in Insert is safe because they are 0 at that time. I guess it still should for consistency.

fjahr commented at 7:35 PM on May 23, 2020:

Yeah, I think it would be better if it was added. Otherwise, bugs could be introduced if Insert is used differently in the future.

achow101 commented at 6:29 PM on May 26, 2020:

I've added it to insert.

fjahr commented at 3:13 PM on March 9, 2020: member

Concept ACK

Good refactoring, code looks good too but please see my comment/question.

in src/wallet/coinselection.cpp:355 in 693cfa66d1 outdated

 345 | +        coin.effective_value = coin.txout.nValue - coin.m_fee;
 346 | +        effective_value += coin.effective_value;
 347 | +    }
 348 | +}
 349 | +
 350 | +OutputGroup OutputGroup::GetPositiveOnlyGroup()

fjahr commented at 7:29 PM on May 23, 2020:

To me this method looks overly complicated and doesn't fit so well with the rest of the API. I would suggest simplifying it like this: https://github.com/fjahr/bitcoin/commit/404e7f2516778510f98b69a4651141d0a5944cbd

Tests passed for me with this change.

achow101 commented at 6:18 PM on May 26, 2020:

My understanding was that this was not safe and could result in undefined behavior because we are actively iterating through the vector while also modifying at the same time (Discard erases from the vector). So the safe way to do this is to use iterators.

fjahr commented at 8:15 PM on May 26, 2020:

You are right! I think this is still a bit of an improvement but only if you have to retouch https://github.com/fjahr/bitcoin/commit/a4388848b7ba409275bf1424dfd724ac1f3e15f9

fjahr changes_requested

fjahr commented at 7:42 PM on May 23, 2020: member

I had another pass and it looks good to me and works fine. But I would like to see the change in Insert() and GetPositiveOnlyGroup() can be simplified significantly, I think.

achow101 force-pushed on May 26, 2020

in src/wallet/coinselection.h:100 in 6a6d19e0e5 outdated

  94 | @@ -93,6 +95,8 @@ struct OutputGroup
  95 |      void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants);
  96 |      std::vector<CInputCoin>::iterator Discard(const CInputCoin& output);
  97 |      bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
  98 | +    void SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate);

instagibbs commented at 6:55 PM on May 26, 2020:

Comment on what this is doing would be :ok_hand:

achow101 commented at 8:24 PM on June 22, 2020:

Added a comment

in src/wallet/wallet.cpp:2289 in 6a6d19e0e5 outdated

2283 | -                    ++it;
2284 | -                } else {
2285 | -                    it = group.Discard(coin);
2286 | -                }
2287 | +            if (coin_selection_params.m_subtract_fee_outputs) {
2288 | +                group.SetFees(CFeeRate(0), long_term_feerate);

instagibbs commented at 6:58 PM on May 26, 2020:

please annotate arg since you're throwing a special(weird) value at it

achow101 commented at 8:24 PM on June 22, 2020:

Added a comment

instagibbs approved

instagibbs commented at 7:07 PM on May 26, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/17458/commits/6a6d19e0e5d91673a0d361ffa732b6dc43890034

achow101 commented at 8:02 PM on May 26, 2020: member

There seems to be a random sigabrt in the BnBExhaustion benchmark.

fjahr commented at 12:22 PM on June 14, 2020: member

ACK 6a6d19e0e5d91673a0d361ffa732b6dc43890034

Tests run fine for me locally.

in src/wallet/coinselection.cpp:344 in 6a6d19e0e5 outdated

 339 | +{
 340 | +    fee = 0;
 341 | +    long_term_fee = 0;
 342 | +    effective_value = 0;
 343 | +    for (CInputCoin& coin : m_outputs) {
 344 | +        coin.m_fee = coin.m_input_bytes < 0 ? 0 : effective_feerate.GetFee(coin.m_input_bytes);

Xekyo commented at 4:19 AM on June 16, 2020:

How would a UTXO's input size ever be smaller than 0? Shouldn't that be an exception?

achow101 commented at 9:20 PM on June 16, 2020:

m_input_bytes default is -1 to indicate it wasn't calculated.

Xekyo commented at 7:10 AM on August 14, 2020:

I see thank you.

in src/wallet/coinselection.h:101 in 6a6d19e0e5 outdated

  94 | @@ -93,6 +95,8 @@ struct OutputGroup
  95 |      void Insert(const CInputCoin& output, int depth, bool from_me, size_t ancestors, size_t descendants);
  96 |      std::vector<CInputCoin>::iterator Discard(const CInputCoin& output);
  97 |      bool EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const;
  98 | +    void SetFees(const CFeeRate effective_feerate, const CFeeRate long_term_feerate);
  99 | +    OutputGroup GetPositiveOnlyGroup();

Xekyo commented at 4:41 AM on June 16, 2020:

My understanding from #17526 was that one instance of OutputGroup described a single UTXO with its ancestry, which seems consistent with the struct above. It seems to me though, that the function GetPositiveOnlyGroup() operates on a set of OutputGroup in the above sense, though. Is that the case?

achow101 commented at 9:22 PM on June 16, 2020:

No, OutputGroup is a group of UTXOs that we want to consider together. It doesn't include ancestry. IIRC this is used for avoid reuse and for some privacy stuff.

Xekyo commented at 7:12 AM on August 14, 2020:

I see, thanks. I wasn't thinking about the bundling of all utxos associated with one address.

MarcoFalke commented at 3:59 PM on June 19, 2020: member

re-run ci

MarcoFalke closed this on Jun 19, 2020

MarcoFalke reopened this on Jun 19, 2020

adamjonas commented at 4:50 PM on June 19, 2020: member

I thought it was a CI problem, but I'm able to reproduce the test failures from the last run.

wallet/test/coinselector_tests.cpp:176: error: in "coinselector_tests/bnb_search_test": check SelectCoinsBnB(GroupCoins(utxo_pool), 5 * CENT, 0.5 * CENT, selection, value_ret, not_input_fees) has failed
wallet/test/coinselector_tests.cpp:177: error: in "coinselector_tests/bnb_search_test": check equal_sets(selection, actual_selection) has failed
wallet/test/coinselector_tests.cpp:178: error: in "coinselector_tests/bnb_search_test": check value_ret == 5 * CENT has failed [2000000 != 5000000]
wallet/test/coinselector_tests.cpp:206: error: in "coinselector_tests/bnb_search_test": check equal_sets(selection, actual_selection) has failed
wallet/test/coinselector_tests.cpp:230: error: in "coinselector_tests/bnb_search_test": check SelectCoinsBnB(GroupCoins(utxo_pool), target, 0, selection, value_ret, not_input_fees) has failed

achow101 force-pushed on Jun 19, 2020

achow101 commented at 11:01 PM on June 19, 2020: member

I'm unable to reproduce the test failures. After rebasing, I also can't get the random sigabrt I was getting.

in src/wallet/coinselection.cpp:317 in 3ed30f4eed outdated

 311 | @@ -311,15 +312,19 @@ void OutputGroup::Insert(const CInputCoin& output, int depth, bool from_me, size
 312 |      // descendants is the count as seen from the top ancestor, not the descendants as seen from the
 313 |      // coin itself; thus, this value is counted as the max, not the sum
 314 |      m_descendants = std::max(m_descendants, descendants);
 315 | -    effective_value = m_value;
 316 | +    effective_value += output.effective_value;
 317 | +    fee += output.m_fee;
 318 | +    long_term_fee += output.m_long_term_fee;

MarcoFalke commented at 11:27 AM on June 20, 2020:

 node1 stderr wallet/coinselection.cpp:317:19: runtime error: signed integer overflow: 7389747844179034116 + 7389747844179034116 cannot be represented in type 'long'

    [#0](/bitcoin-bitcoin/0/) 0x55cf4e900813 in OutputGroup::Insert(CInputCoin const&, int, bool, unsigned long, unsigned long) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/coinselection.cpp:317:19
    [#1](/bitcoin-bitcoin/1/) 0x55cf4e7a29a3 in CWallet::GroupOutputs(std::vector<COutput, std::allocator<COutput> > const&, bool, unsigned long) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/wallet.cpp:4152:32
    [#2](/bitcoin-bitcoin/2/) 0x55cf4e7a02c4 in CWallet::SelectCoins(std::vector<COutput, std::allocator<COutput> > const&, long const&, std::set<CInputCoin, std::less<CInputCoin>, std::allocator<CInputCoin> >&, long&, CCoinControl const&, CoinSelectionParams&, bool&) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/wallet.cpp:2391:39
    [#3](/bitcoin-bitcoin/3/) 0x55cf4e7aa361 in CWallet::CreateTransaction(std::vector<CRecipient, std::allocator<CRecipient> > const&, std::shared_ptr<CTransaction const>&, long&, int&, bilingual_str&, CCoinControl const&, bool) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/wallet.cpp:2812:26
    [#4](/bitcoin-bitcoin/4/) 0x55cf4e6685d4 in SendMoney(CWallet*, boost::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown> const&, long, bool, CCoinControl const&, std::map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > >) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/rpcwallet.cpp:347:19
    [#5](/bitcoin-bitcoin/5/) 0x55cf4e65042f in sendtoaddress(JSONRPCRequest const&) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/wallet/rpcwallet.cpp:444:26
    [#6](/bitcoin-bitcoin/6/) 0x55cf4def5849 in CRPCCommand::CRPCCommand(char const*, char const*, UniValue (*)(JSONRPCRequest const&), std::initializer_list<char const*>)::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./rpc/server.h:107:94
    [#7](/bitcoin-bitcoin/7/) 0x55cf4def51f8 in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), CRPCCommand::CRPCCommand(char const*, char const*, UniValue (*)(JSONRPCRequest const&), std::initializer_list<char const*>)::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
    [#8](/bitcoin-bitcoin/8/) 0x55cf4db18858 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
    [#9](/bitcoin-bitcoin/9/) 0x55cf4e4f591f in interfaces::(anonymous namespace)::WalletClientImpl::registerRpcs()::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/interfaces/wallet.cpp:495:24
    [#10](/bitcoin-bitcoin/10/) 0x55cf4e4f591f in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), interfaces::(anonymous namespace)::WalletClientImpl::registerRpcs()::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
    [#11](/bitcoin-bitcoin/11/) 0x55cf4db18858 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
    [#12](/bitcoin-bitcoin/12/) 0x55cf4db0dfd8 in interfaces::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(CRPCCommand const&)::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/interfaces/chain.cpp:113:24
    [#13](/bitcoin-bitcoin/13/) 0x55cf4db0dfd8 in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), interfaces::(anonymous namespace)::RpcHandlerImpl::RpcHandlerImpl(CRPCCommand const&)::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
    [#14](/bitcoin-bitcoin/14/) 0x55cf4db18858 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
    [#15](/bitcoin-bitcoin/15/) 0x55cf4e0473c9 in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/server.cpp:469:20
    [#16](/bitcoin-bitcoin/16/) 0x55cf4e046cb0 in CRPCTable::execute(JSONRPCRequest const&) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/rpc/server.cpp:452:17
    [#17](/bitcoin-bitcoin/17/) 0x55cf4e42abc4 in HTTPReq_JSONRPC(util::Ref const&, HTTPRequest*) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/httprpc.cpp:207:40
    [#18](/bitcoin-bitcoin/18/) 0x55cf4e429f24 in StartHTTPRPC(util::Ref const&)::$_0::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/httprpc.cpp:293:81
    [#19](/bitcoin-bitcoin/19/) 0x55cf4e429f24 in std::_Function_handler<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&), StartHTTPRPC(util::Ref const&)::$_0>::_M_invoke(std::_Any_data const&, HTTPRequest*&&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
    [#20](/bitcoin-bitcoin/20/) 0x55cf4e44adf2 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
    [#21](/bitcoin-bitcoin/21/) 0x55cf4e44aa42 in HTTPWorkItem::operator()() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/httpserver.cpp:56:9
    [#22](/bitcoin-bitcoin/22/) 0x55cf4e453a84 in WorkQueue<HTTPClosure>::Run() /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/httpserver.cpp:115:13
    [#23](/bitcoin-bitcoin/23/) 0x55cf4e43d04f in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) /tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/httpserver.cpp:343:12
    [#24](/bitcoin-bitcoin/24/) 0x55cf4e45a0fc in void std::__invoke_impl<void, void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>(std::__invoke_other, void (*&&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:60:14
    [#25](/bitcoin-bitcoin/25/) 0x55cf4e459f8a in std::__invoke_result<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>::type std::__invoke<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int>(void (*&&)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*&&, int&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/invoke.h:95:14
    [#26](/bitcoin-bitcoin/26/) 0x55cf4e459d51 in void std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> >::_M_invoke<0ul, 1ul, 2ul>(std::_Index_tuple<0ul, 1ul, 2ul>) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:244:13
    [#27](/bitcoin-bitcoin/27/) 0x55cf4e4594fc in std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> >::operator()() /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:251:11
    [#28](/bitcoin-bitcoin/28/) 0x55cf4e4594fc in std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(WorkQueue<HTTPClosure>*, int), WorkQueue<HTTPClosure>*, int> > >::_M_run() /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/thread:195:13
    [#29](/bitcoin-bitcoin/29/) 0x7fce19ab3cb3  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xd6cb3)
    [#30](/bitcoin-bitcoin/30/) 0x7fce19eb2608 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x9608)
    [#31](/bitcoin-bitcoin/31/) 0x7fce19790102 in clone (/lib/x86_64-linux-gnu/libc.so.6+0x122102)
SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow wallet/coinselection.cpp:317:19 in

https://cirrus-ci.com/task/4765844100087808?command=ci#L3848

achow101 commented at 8:24 PM on June 22, 2020:

Should be fixed now.

achow101 force-pushed on Jun 22, 2020

adamjonas commented at 12:33 PM on June 24, 2020: member

Confirming the test failures from 3ed30f4ee are passing in 410c8b1734bfd0d113b50f0728a20a7962b05449.

Use real value when calculating OutputGroup value

OutputGroup::m_value is the true value, not the effective value,
so use the true values of the outputs, not their effective values.

7d07e864b8

achow101 force-pushed on Jul 30, 2020

Refactor OutputGroups to handle effective values, fees, and filtering

Instead of having callers set the fees, effective values, and filtering
of outputs, do these within OutputGroups themselves as member functions.

m_fee and m_long_term_fee is added to OutputGroup to track the fees of
the OutputGroup.

9adc2f80fc

achow101 force-pushed on Aug 11, 2020

instagibbs commented at 1:57 AM on August 13, 2020: member

reACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f

fanquake commented at 2:05 AM on August 13, 2020: member

@Xekyo did you want to take another look here?

fjahr commented at 2:35 PM on August 13, 2020: member

re-ACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f

per git range-diff 4af01b37d40246cd1fdb54719855927e36a36b46..6a6d19e0e5d91673a0d361ffa732b6dc43890034 edec7f7c254294cd5c46ae5cf304353d458bb852..9adc2f80fc14f11ee2b1f989ee7be71b58481e6f

meshcollider commented at 9:36 PM on August 13, 2020: contributor

Light code review ACK 9adc2f80fc14f11ee2b1f989ee7be71b58481e6f

I'll wait a bit longer for @Xekyo but otherwise I'll merge this tomorrow

in src/wallet/coinselection.cpp:348 in 9adc2f80fc

 343 | +    for (CInputCoin& coin : m_outputs) {
 344 | +        coin.m_fee = coin.m_input_bytes < 0 ? 0 : effective_feerate.GetFee(coin.m_input_bytes);
 345 | +        fee += coin.m_fee;
 346 | +
 347 | +        coin.m_long_term_fee = coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes);
 348 | +        long_term_fee += coin.m_long_term_fee;

Xekyo commented at 7:30 AM on August 14, 2020:

Actually, why is the long_term_fee tracked additionally to just the fee here? Is that just for calculating the waste metric?

If I understand that right, at this point the output group is already being cast into the context of the current transaction (i.e. the fee is specific to the transaction we are building). Since the waste metric is only dependent on the current fee rate and the long term fee rate (if that's still the same from when we discussed two years ago), perhaps it would be better to calculate the waste here instead of carrying around the long term fee to calculate the waste later. Might be better to do that in separate PR later, though, even if you agree. ;)

achow101 commented at 4:08 PM on August 14, 2020:

Indeed, I think it would be better to track the waste. I will do that in a followup as it is a slightly larger change which touches the actual coin selection algorithms.

meshcollider merged this on Aug 14, 2020

meshcollider closed this on Aug 14, 2020

sidhujag referenced this in commit c606dd512f on Aug 16, 2020

meshcollider referenced this in commit 7dc4807691 on Feb 1, 2021

Fabcien referenced this in commit 01e27ccd51 on Sep 10, 2021

Fabcien referenced this in commit a0c263af35 on Sep 10, 2021

DrahtBot locked this on Aug 16, 2022