wallet: Fix Char as Bool in Wallet #16572

JeremyRubin commented at 11:36 PM on August 8, 2019: contributor

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.

Fix Char as Bool in interfaces 2dbfb37b40

fanquake added the label Wallet on Aug 8, 2019

laanwj commented at 11:49 PM on August 8, 2019: member

Seems like an improvement, any idea why this was a char in the first place (or how it came to be)?

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.

Wait, this metadata is never part of the transaction hash right?

JeremyRubin commented at 11:56 PM on August 8, 2019: contributor

It seems like it is just old/crufty. It was previously proposed by @ryanofsky to be removed (dummied out), but that went nowhere. #9351

Keeping the distinction seems OK to me, but the consistency issue should be fixed in any case.

The metadata isn't a part of the transaction hash, but it could be a part of the hash (like a siphash, not a crypto hash) of the object itself, e.g., for storing in a map.

kallewoof commented at 2:45 AM on August 9, 2019: member

Concept ACK

There's also this which I never got why it was a char:

https://github.com/bitcoin/bitcoin/blob/e5fdda68c6d2313edb74443f0d1e6d2ce2d97f5e/src/wallet/coinselection.cpp#L173

JeremyRubin commented at 3:13 AM on August 9, 2019: contributor

Actually there is a reason for that one!

Vec bool is typically compacted, so it's probably slower running because of masking.

Maybe worth profiling to find out!

sipa commented at 3:24 AM on August 9, 2019: member

From a march 2011 mail from Satoshi to me about how the wallet tracks unspent outputs:

Everything that uses fSpent must be changed to use
vfSpent instead.  I reorganised IMPLEMENT_SERIALIZE
a little better, now that it's clear how the pattern is going. 
Debatable whether to use vector<char> or vector<bool>. 
A lot of people hate vector<bool> because it has
irregularities to trip over.

So I guess the reason was indeed some worry about the std::vector weirdness (it's special cased to be bitbacked).

JeremyRubin commented at 3:35 AM on August 9, 2019: contributor

Hm; may be worth it to go ahead and modify these locations to use vec<char>...

blockencodings.cpp
merkleblock.cpp
rest.cpp
script/interpreter.cpp
test/pmt_tests.cpp
txdb.cpp
wallet/coinselection.cpp

Would require some profiling to ensure that there isn't a regression.

The only place that is using vector<bool> for compactness is the cuckoocache.

kallewoof commented at 5:37 AM on August 9, 2019: member

Gotta love the unexpected history lessons. :)

laanwj commented at 9:12 AM on August 9, 2019: member

Hm; may be worth it to go ahead and modify these locations to use vec<char>...

Why? Unless you can demonstrate specific performance improvements, where the (possible) compaction causes bottlenecks, I strongly disagree that this is a reason to go around refactoring things.

From a developer angle vector<bool> is definitely less surprising to encounter (use bools for what should be bools) and unless there's a specific reason the compaction gets in the way (serialization voodoo ?), it shouldn't be a reason for concern at all.

JeremyRubin commented at 5:39 PM on August 9, 2019: contributor

Ah; I don't suspect there's a performance reason to modify them, but if there's general concern for surprising behavior with the std::vector<bool>, it would be good to avoid that.

In any case, this is a bit of a side-quest from the scope of this PR.

fanquake renamed this:
~~Fix Char as Bool in Wallet~~
wallet: Fix Char as Bool in Wallet
on Aug 14, 2019

fanquake requested review from meshcollider on Aug 14, 2019

fanquake requested review from achow101 on Aug 14, 2019

laanwj commented at 9:12 AM on August 14, 2019: member

Ah; I don't suspect there's a performance reason to modify them, but if there's general concern for surprising behavior with the std::vector<bool>, it would be good to avoid that.

What kind of surprising behavior, really? the implementation might be different but as far as I know, the "API contract" is exactly the same?

sipa commented at 5:02 PM on August 14, 2019: member

I think the only surprising thing about it is that you can't cast a pointer to the data to a char pointer and expect any consistent representation of the data... which you shouldn't be doing anyway, but I think the early codebase did stuff like that.

JeremyRubin commented at 8:32 PM on August 14, 2019: contributor

The main one is the data pointer to a bool pointer.

There are a few other differences:

Concurrent modification of different bits is not guaranteed to be safe
The "API Contract" differs, such that generic code can't be written accepting vector<bool> (e.g., range for loops don't work).

achow101 commented at 8:36 PM on August 14, 2019: member

Code review ACK 2dbfb37b407ed23b517f507d78fb77334142dce5

I checked that fFromMe is only used as a bool. I also checked bools and chars are being serialized in exactly the same way so this does not change any behavior.

laanwj commented at 11:52 AM on August 15, 2019: member

The "API Contract" differs, such that generic code can't be written accepting vector<bool> (e.g., range for loops don't work).

FWIW this works fine here … (but maybe it's a gcc-ism?)

#include <iostream>
#include <vector>

int main() {
    std::vector<bool> test;
    test.push_back(false);
    test.push_back(true);
    test.push_back(false);
    for (auto const &x: test) {
        std::cout << x << std::endl;
    };
    return 0;
}

practicalswift commented at 12:47 PM on August 15, 2019: contributor

@laanwj Note that the x you get in your example is likely a "bit reference" (std::_Bit_reference) and not a bool.

The gotcha:

    std::vector<char> v1;
    for (const auto& x : v1) {
      // const char& x (as expected)
    }

    std::vector<int> v2;
    for (const auto& x : v2) {
      // const int& x (as expected)
    }

    std::vector<bool> v3;
    for (const auto& x : v3) {
      // const std::_Bit_reference& x (🎉)
    }

Also note that it is implementation-defined if any space efficiency optimisations are done.

DrahtBot commented at 12:36 AM on August 16, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16624 (wallet : encapsulate transactions state by ariard)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

meshcollider commented at 9:43 PM on August 16, 2019: contributor

Regardless of the above discussion on vectors, this change itself looks safe to me, and does make things more sensible IMO.

Code review ACK 2dbfb37b407ed23b517f507d78fb77334142dce5

(Your commit isn't linked to your GitHub account btw Jeremy)

fanquake referenced this in commit 01ebaa05a4 on Aug 21, 2019

fanquake merged this on Aug 21, 2019

fanquake closed this on Aug 21, 2019

laanwj commented at 7:33 AM on August 21, 2019: member

Regardless of the above discussion on vectors, this change itself looks safe to me, and does make things more sensible IMO.

Yes, agree, sorry it got derailed. Posthumous ACK 2dbfb37b407ed23b517f507d78fb77334142dce5. This change is the right way around, I was arguing against going bool to char.

sidhujag referenced this in commit 6e8436c1e1 on Aug 22, 2019

MarcoFalke locked this on Dec 16, 2021