wallet: sum ancestors rather than taking max in output groups #13812

1. 
   kallewoof commented at 7:44 pm on July 30, 2018: member

   This is pointed out in #12257 (review).

   Basically, the ancestors gives an indication as to how many ancestors the resulting transaction will have, which is more precise when summing up the values, rather than taking the maximum, since all the coins in the group will become ancestors if selected.

2. in src/wallet/coinselection.cpp:302 in b9a689c92b outdated

   303-    // not ideal, as a wallet will consider e.g. thirty 2-ancestor coins as having two ancestors,
   304-    // when in reality it has 60 ancestors.
   305-    m_ancestors = std::max(m_ancestors, ancestors);
   306-    // m_descendants is the count as seen from the top ancestor, not the descendants as seen from the
   307-    // coin itself; thus, this value is accurate
   308+    // ancestors here express the number of ancestors the new coin will end up having, which is
   

   ---

   ---

   

   sdaftuar commented at 7:48 pm on July 30, 2018:

   Perhaps you could update the comment to explain that this calculation will generally be overestimating the number of ancestors of a group (eg when two coins share a common ancestor)? Otherwise a future reader might be confused about the correctness of this calculation.

   ---

   

   kallewoof commented at 7:55 pm on July 30, 2018:

   How about now?

   ---

   

   sdaftuar commented at 7:57 pm on July 30, 2018:

   Looks good, thanks!
3. 
   sdaftuar commented at 7:50 pm on July 30, 2018: member

   utACK apart from the comment nit, thanks for addressing.
4. wallet: sum ancestors rather than taking max in output groups 23fbbb100f
5. kallewoof force-pushed on Jul 30, 2018
6. 
   sdaftuar commented at 7:59 pm on July 30, 2018: member

   utACK 23fbbb100f63cb621b4b901dac0c0f16d7d74bc7
7. fanquake added the label Wallet on Jul 30, 2018
8. 
   gmaxwell commented at 4:51 am on August 1, 2018: contributor

   utACK
9. 
   laanwj commented at 3:23 pm on August 7, 2018: member

   makes sense utACK 23fbbb100f63cb621b4b901dac0c0f16d7d74bc7
10. laanwj merged this on Aug 7, 2018
11. laanwj closed this on Aug 7, 2018

    ---

12. laanwj referenced this in commit 9d86aad287 on Aug 7, 2018
13. kallewoof deleted the branch on Aug 7, 2018
14. UdjinM6 referenced this in commit e1f4da907e on Jul 4, 2021
15. UdjinM6 referenced this in commit 5e51a7299a on Jul 6, 2021
16. UdjinM6 referenced this in commit 389aaa07d1 on Jul 6, 2021
17. DrahtBot locked this on Sep 8, 2021

Contributors
kallewoof sdaftuar gmaxwell laanwj

Labels
Wallet