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: Revert input selection post-pruning #8298

Merged
merged 1 commit into from Jul 6, 2016

Conversation

Projects
None yet
5 participants
@laanwj
Member

laanwj commented Jul 1, 2016

This reverts PR #4906, "Coinselection prunes extraneous inputs from ApproximateBestSubset".

Apparently the previous behavior of slightly over-estimating the set of inputs was useful in cleaning up UTXOs.

See also #7664, #7657, as well as 2016-07-01 discussion on #bitcoin-core-dev IRC.

wallet: Revert input selection post-pruning
This reverts PR #4906, "Coinselection prunes extraneous inputs from
ApproximateBestSubset".

Apparently the previous behavior of slightly over-estimating the set of
inputs was useful in cleaning up UTXOs.

See also #7664, #7657, as well as 2016-07-01 discussion on #bitcoin-core-dev IRC.

@laanwj laanwj added the Wallet label Jul 1, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 1, 2016

Member
Member

MarcoFalke commented Jul 1, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 1, 2016

Member

Yes, apparently simulations are not clear that this was an improvement at all (see discussion in #4906). I know it's a huge burden for anyone proposing changes to coin selection, but I think it should have been proven that this is better than the status quo first before it was merged.

Or maybe even that's putting the cart before the horse: there should be a list of criteria for coin-selection algorithms first, before we can evaluate what (incremental) improvements are good or bad.

Member

laanwj commented Jul 1, 2016

Yes, apparently simulations are not clear that this was an improvement at all (see discussion in #4906). I know it's a huge burden for anyone proposing changes to coin selection, but I think it should have been proven that this is better than the status quo first before it was merged.

Or maybe even that's putting the cart before the horse: there should be a list of criteria for coin-selection algorithms first, before we can evaluate what (incremental) improvements are good or bad.

@MarcoFalke MarcoFalke added this to the 0.13.0 milestone Jul 1, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Jul 1, 2016

utACK 20f3cd7

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 5, 2016

Contributor

utACK 20f3cd7

Contributor

petertodd commented Jul 5, 2016

utACK 20f3cd7

@RHavar

This comment has been minimized.

Show comment
Hide comment
@RHavar

RHavar Jul 6, 2016

Contributor

I actually disagree with reverting this, conditional to the bug here: #7664 (comment) being fixed. If that's fixed, it'll be a good thing to have kept the "extraneous inputs" around, as they'll actually come in handy.

Contributor

RHavar commented Jul 6, 2016

I actually disagree with reverting this, conditional to the bug here: #7664 (comment) being fixed. If that's fixed, it'll be a good thing to have kept the "extraneous inputs" around, as they'll actually come in handy.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 6, 2016

Member

@RHavar the challenge right now is that we're pretty close to a release to undertake a serious effort at coin selection reworking; so whatever we do has the be as minimal as possible

Member

gmaxwell commented Jul 6, 2016

@RHavar the challenge right now is that we're pretty close to a release to undertake a serious effort at coin selection reworking; so whatever we do has the be as minimal as possible

@RHavar

This comment has been minimized.

Show comment
Hide comment
@RHavar

RHavar Jul 6, 2016

Contributor

In that case, I think (temporarily?) reverting it is the way to go. The behavior on 0.12 was definitely a big regression =)

Contributor

RHavar commented Jul 6, 2016

In that case, I think (temporarily?) reverting it is the way to go. The behavior on 0.12 was definitely a big regression =)

@laanwj laanwj merged commit 20f3cd7 into bitcoin:master Jul 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 6, 2016

Merge #8298: wallet: Revert input selection post-pruning
20f3cd7 wallet: Revert input selection post-pruning (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8298: wallet: Revert input selection post-pruning
20f3cd7 wallet: Revert input selection post-pruning (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge #8298: wallet: Revert input selection post-pruning
20f3cd7 wallet: Revert input selection post-pruning (Wladimir J. van der Laan)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge #8298: wallet: Revert input selection post-pruning
20f3cd7 wallet: Revert input selection post-pruning (Wladimir J. van der Laan)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment