wallet: Remove AvailableCoins nMinDepth argument #15823

issue MarcoFalke opened this issue on April 15, 2019

MarcoFalke commented at 7:08 PM on April 15, 2019: member

AvailableCoins() could be refactored to not take an nMinDepth argument now that coin control has a m_min_depth member
MarcoFalke added the label Refactoring on Apr 15, 2019
MarcoFalke added the label Wallet on Apr 15, 2019
MarcoFalke added the label good first issue on Apr 15, 2019
MarcoFalke commented at 7:10 PM on April 15, 2019: member

Having two ways to set this (where one of them is silently ignored) is a nice footgun
fanquake commented at 6:53 AM on April 16, 2019: member

How would you like to handle listunspent passing the minconf argument (as nMinDepth) straight into AvailableCoins:

https://github.com/bitcoin/bitcoin/blob/598323911e930d67e678e464353eb570ad3bf2b7/src/wallet/rpcwallet.cpp#L2797

With nMinDepth removed, I assume we'll need to create a CCoinControl object, just to set m_min_depth, and pass that into AvailableCoins rather than a nullptr.
promag commented at 7:14 AM on April 16, 2019: member

create a CCoinControl object, just to set m_min_depth, and pass that into AvailableCoins @fanquake I guess.
amitiuttarwar commented at 11:06 PM on April 16, 2019: contributor
I started looking into this & have a question about desired default behavior. Right now, AvailableCoins can be invoked without passing in a CCoinControl object, as done in ListCoins here: https://github.com/bitcoin/bitcoin/blob/0c9de67f343c0e740a7f488e85270d519a352119/src/wallet/wallet.cpp#L2348 The function declaration in the header handles this behavior by defaulting CCoinControl to a nullptr. Having that null pointer obviously creates an issue when trying to access m_min_depth on coin control.

The two options I see:
1. Have AvailableCoins default to creating a new coin control.
2. Remove the default & require the caller to create a new coin control.
  The second option seems preferable because the caller can manage memory cleanup. There could also be other ways to handle.
Thoughts?
MarcoFalke commented at 2:34 PM on April 17, 2019: member

I wouldn't mind explicitly passing through a const CCoinControl& instead of a pointer (that can be null). That way coin control is exposed to all callers and they can choose to use the default values or overwrite them.
promag commented at 9:13 AM on April 22, 2019: member

@amitiuttarwar are you going to work on this?
amitiuttarwar commented at 2:08 PM on April 22, 2019: contributor

I would like to, but I'm off the grid until Thursday.

On Apr 22, 2019, 3:14 AM, at 3:14 AM, "João Barbosa" notifications@github.com wrote:

@amitiuttarwar are you going to work on this?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/issues/15823#issuecomment-485372228
MarcoFalke removed the label good first issue on Apr 22, 2019
jnewbery commented at 4:14 PM on April 22, 2019: member

Concept ACK. See #15557 (review) for a bit more discussion/background.
laanwj referenced this in commit 00922b8720 on Jul 31, 2019
sidhujag referenced this in commit 217e4058c3 on Aug 1, 2019
MarcoFalke closed this on Oct 17, 2019
ryanofsky commented at 2:26 PM on October 17, 2019: member

Reason for closing this?
MarcoFalke commented at 2:32 PM on October 17, 2019: member

Has been fixed in 00922b8720278f07a0f6e0e22b6b38356f9ee384
jnewbery commented at 3:16 PM on October 17, 2019: member

Has been fixed in 00922b8

(which is PR #15906)
jasonbcox referenced this in commit 327d1005b2 on Jul 28, 2020
ShengguangXiao referenced this in commit 09ca614b2d on Aug 28, 2020
monstrobishi referenced this in commit 135416d1e3 on Sep 6, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15906 [wallet] Move min_depth and max_depth to coin control