Commit 92bcd70808b9cac56b184903aa6d37baf9641b37 presumably added a check that a dest of type CNoDestination implies an empty scriptChange.
However, it accidentally checked for boost::variant::empty, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
Looks like this was pointed out at the time:
It's not clear to me what this assert is checking for.
If
destis not empty, we got a change destination and soscriptChangeis not empty. If dest is empty, thenscriptChangewill be empty. So this just covers all cases and can't fail? .... I guess it's fine, but it just seems like useless code that might be confusing to future readers. I would suggest that it be removed in a followup that touches this code.
Maybe just remove entirely as suggested by @achow101 ?
At least now the check will fail when the assumption doesn't hold, so it is no longer entirely useless and confusing. I have a slight preference to document this assumption. Either with a CHECK_NONFATAL, or a test, or a developer comment, not sure about removing it altogether.
2776 | @@ -2777,7 +2777,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std 2777 | strFailReason = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.").translated; 2778 | } 2779 | scriptChange = GetScriptForDestination(dest); 2780 | - assert(!dest.empty() || scriptChange.empty()); 2781 | + CHECK_NONFATAL(!boost::get<CNoDestination>(&dest) == !scriptChange.empty());
Could the boost::get() return value be converted to bool explicitly?
Could the boost::get() return value be converted to bool explicitly?
Yes, for example with the !! "operator".
CHECK_NONFATAL(!!boost::get<CNoDestination>(&dest) == scriptChange.empty());
Concept ACK.
Concept ACK, how did you catch this?
Could we also do this?
CScript GetScriptForDestination(const CTxDestination& dest)
{
CScript script;
bool result = boost::apply_visitor(CScriptVisitor(&script), dest);
assert(result != script.empty());
return script;
}
Concept ACK, how did you catch this?
Reading the source code and boost documentation
2776 | @@ -2777,7 +2777,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std 2777 | strFailReason = _("Transaction needs a change address, but we can't generate it. Please call keypoolrefill first.").translated; 2778 | } 2779 | scriptChange = GetScriptForDestination(dest); 2780 | - assert(!dest.empty() || scriptChange.empty()); 2781 | + CHECK_NONFATAL(bool(boost::get<CNoDestination>(&dest)) == scriptChange.empty());
Maybe this is more readable:
// scriptChange must be empty if dest is a dummy CNoDestination
CHECK_NONFATAL(IsValidDestination(dest) || scriptChange.empty());
Thanks, done
Yikes, good catch.
The reason I added that check was there used to be an early return if the keypool ran out. I want to make sure we don't send change into a void. We now return later (line 2999) if this error occurs. That check relies on scriptChange.empty(), hence my assert here to make really sure of that.
switched to IsValidDestination helper
utACK fa47cf9d95dc2c2822fc96df16f179176935bf96
Open-Close to re-run ci. See #15847 (comment)