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
refactor: remove redundant fOnlySafe argument #21910
Conversation
The fOnlySafe argument to AvailableCoins is now redundant, since bitcoin#21359 added a similar field inside the CCoinControl struct. Not all code paths set a CCoinControl instance, but when it's missing we can default to using only safe inputs which is backwards-compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c30dd02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK c30dd02.
@@ -2197,7 +2197,7 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const | |||
|
|||
CAmount balance = 0; | |||
std::vector<COutput> vCoins; | |||
AvailableCoins(vCoins, true, coinControl); | |||
AvailableCoins(vCoins, coinControl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an actual refactor because this changes behavior. But in practice AMT there's no call to GetAvailableBalance
with m_include_unsafe_inputs = true
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, it does change behavior here but is backwards-compatible so I think it should be ok.
If later we want to make GetAvailableBalance
with m_include_unsafe_inputs = true
possible, we can use coinControl
for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c30dd02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review + test run ACK c30dd02
The
fOnlySafe
argument toAvailableCoins
is now redundant, since #21359 added a similar field inside theCCoinControl
struct (see #21359 (comment)).Not all code paths create a
CCoinControl
instance, but when it's missing we can default to using only safe inputs which is backwards-compatible.