Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 4, 2016

vValue is sorted from high to low. This adds a regression test such that pulls like #7183 would fail travis.

@maflcko
Copy link
Member Author

maflcko commented Jan 4, 2016

Note: #7183 was changed in the meantime, the original diff might come in handy to test this pull:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index d23d54e..3d1f58c 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1605,7 +1605,7 @@ static void ApproximateBestSubset(vector<pair<CAmount, pair<const CWalletTx*,uns
         bool fReachedTarget = false;
         for (int nPass = 0; nPass < 2 && !fReachedTarget; nPass++)
         {
-            for (unsigned int i = 0; i < vValue.size(); i++)
+            for (unsigned int i = 0; i < vValue.size() && !fReachedTarget; i++)
             {
                 //The solver here uses a randomized algorithm,
                 //the randomness serves no real security purpose but is just
@@ -1625,8 +1625,6 @@ static void ApproximateBestSubset(vector<pair<CAmount, pair<const CWalletTx*,uns
                             nBest = nTotal;
                             vfBest = vfIncluded;
                         }
-                        nTotal -= vValue[i].first;
-                        vfIncluded[i] = false;
                     }
                 }
             }

@laanwj
Copy link
Member

laanwj commented Jan 5, 2016

utACK

@maflcko maflcko force-pushed the Mf1601-wallet-vValue branch from 2a13d7b to fa3c7e6 Compare January 5, 2016 12:32
@murchandamus
Copy link
Contributor

ACK: Fails for the referenced original PR #7183.

@laanwj laanwj merged commit faf538b into bitcoin:master Jan 7, 2016
laanwj added a commit that referenced this pull request Jan 7, 2016
faf538b [trivial] Merge test cases and replace CENT with COIN (MarcoFalke)
fa3c7e6 [wallet] Add regression test for vValue sort order (MarcoFalke)
laanwj pushed a commit that referenced this pull request Jan 7, 2016
- [wallet] Add regression test for vValue sort order
- [trivial] Merge test cases and replace CENT with COIN

Github-Pull: #7293
Rebased-From: fa3c7e6 faf538b
@laanwj
Copy link
Member

laanwj commented Jan 7, 2016

Cherry-picked to 0.12 as ff9b610

@maflcko maflcko deleted the Mf1601-wallet-vValue branch January 7, 2016 11:23
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants