Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 9, 2015

This is a follow up of #4906.

Please review commit-by-commit:

  • First commit: The test case is supposed to verify that input pruning works. However, for inputs [100] * 2, [10] * 12 and a target value of 221 the old code as well as the new code always return [100] * 2, [10] * 3, regardless of the number of iterations, etc.
    To actually test pruning, one needs a sufficient amount of "big coins" so that the random approximation misses at least one big coin even if iterations=1000 (default). Thus, "smaller coins" get added by the algorithm. (But the algorithm does not yet know they are not sufficient and will later be useless, after executing the second pass)
  • Second commit: Use clang-format because the code style is really messed up here. (Reminder to use ?w=0 flag on GitHub to disable white space diff) // moved to a later pull

@maflcko
Copy link
Member Author

maflcko commented Dec 9, 2015

If someone feels like testing that pruning the trimming test must fail on 0.12, git diff 08000~1..fafd093 src/wallet/test/wallet_tests.cpp | git apply onto 0.12 may come in handy.

@jonasschnelli
Copy link
Contributor

Confirmed as move-only expect of the new pruning_in_ApproximateBestSet test (L339-L346).
utACK.

@murchandamus
Copy link
Contributor

utACK Those changes make sense to me.

@laanwj laanwj merged commit fafd093 into bitcoin:master Jan 5, 2016
laanwj added a commit that referenced this pull request Jan 5, 2016
fafd093 [wallet] Adjust pruning test (MarcoFalke)
@maflcko maflcko deleted the MarcoFalke-2015-WalletTests branch January 5, 2016 11:58
laanwj pushed a commit that referenced this pull request Jan 5, 2016
Github-Pull: #7193
Rebased-From: fafd093
@laanwj
Copy link
Member

laanwj commented Jan 5, 2016

Backported to 0.12 as bfdaa3c - 96e8d12 is also in there, so the test doesn't fail on 0.12.

@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