wallet: Fix typo in assert that is compile-time true #18853
Conversation
|
Looks like this was pointed out at the time:
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 |
maflcko
force-pushed
the
2004-walletAssertAlwaysTrue
branch
2 times, most recently
from
fafdd1d to
fa1529d
Compare
|
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;
} |
Reading the source code and boost documentation |
There was a problem hiding this comment.
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.
maflcko
force-pushed
the
2004-walletAssertAlwaysTrue
branch
from
fa1529d to
fa9bec6
Compare
|
switched to |
maflcko
force-pushed
the
2004-walletAssertAlwaysTrue
branch
from
fa9bec6 to
fa51acb
Compare
maflcko
force-pushed
the
2004-walletAssertAlwaysTrue
branch
from
fa51acb to
fa47cf9
Compare
|
utACK fa47cf9 |
|
Open-Close to re-run ci. See #15847 (comment) |
…true fa47cf9 wallet: Fix typo in assert that is compile-time true (MarcoFalke) Pull request description: Commit 92bcd70 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 ACKs for top commit: Sjors: utACK fa47cf9 Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
Summary: > Commit 92bcd70 (D8667] 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: This is a backport of Core [[bitcoin/bitcoin#18853 | PR18853]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9081
7 participants
Commit 92bcd70 presumably added a check that a
destof typeCNoDestinationimplies an emptyscriptChange.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