Skip to content
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

[wallet] Add regression test for vValue sort order #7293

Merged
merged 2 commits into from Jan 7, 2016

Conversation

@MarcoFalke
Copy link
Member

MarcoFalke commented Jan 4, 2016

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

@MarcoFalke
Copy link
Member Author

MarcoFalke 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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1601-wallet-vValue branch to fa3c7e6 Jan 5, 2016
@Xekyo
Copy link
Contributor

Xekyo commented Jan 5, 2016

ACK: Fails for the referenced original PR #7183.

@laanwj laanwj merged commit faf538b into bitcoin:master Jan 7, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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 added 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

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1601-wallet-vValue branch Jan 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.