Page MenuHomePhabricator

[backport#16572] wallet: Fix Char as Bool in Wallet

Authored by majcosta on Mon, Jul 27, 18:17.


Group Reviewers
Restricted Project
rABCdc175d1eeef2: [backport#16572] wallet: Fix Char as Bool in Wallet

Fix Char as Bool in interfaces (Jeremy Rubin)

Pull request description:

In a few places in src/wallet/wallet.h, we use a char when semantically we want a bool.

This is kind of an issue because it means we can unserialize the same transaction with different fFromMe flags (as differing chars) and evaluate the following section in wallet/wallet.cpp
        if (wtxIn.fFromMe && wtxIn.fFromMe != wtx.fFromMe)
             wtx.fFromMe = wtxIn.fFromMe;
             fUpdated = true;
incorrectly (triggering an fUpdated where both fFromMe values represent true, via different chars).

I don't think this is a vulnerability, but it's just a little messy and unsemantic, and could lead to issues with stored wtxIns not being findable in a map by their hash.

The serialize/unserialize code for bool internally uses a char, so it should be safe to make this substitution.

NOTE: Technically, this is a behavior change -- I haven't checked too closely that nowhere is depending on storing information in this char. Theoretically, this could break something because after this change a tx unserialized with such a char would preserve it's value, but now it is converted to a ~true~ canonical bool.

Backport of Core PR16572

Test Plan
ninja check check-functional

Diff Detail

rABC Bitcoin ABC
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

majcosta created this revision.Mon, Jul 27, 18:17
Herald added a reviewer: Restricted Project. · View Herald TranscriptMon, Jul 27, 18:17
majcosta requested review of this revision.Mon, Jul 27, 18:17
deadalnix accepted this revision.Mon, Jul 27, 19:06
This revision is now accepted and ready to land.Mon, Jul 27, 19:06
This revision was automatically updated to reflect the committed changes.
teamcity edited the summary of this revision. (Show Details)Mon, Jul 27, 22:04

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.