Support serialization of std::vector<bool> #16730

sipa · 2019-08-26T18:39:36Z

This adds support for serialization of std::vector<bool>, as a prerequisite of #16702.

maflcko · 2019-08-26T18:45:52Z

ACK 92528c2 (only looked at the diff on GitHub)

practicalswift · 2019-08-26T18:56:49Z

ACK 92528c2 -- diff looks correct

std::vector<bool> gotcha illustrated:

    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 (🎉)
    }

jamesob · 2019-08-26T19:17:48Z

ACK 92528c2

Low-risk change, reviewed on Github.

92528c2 Support serialization of std::vector<bool> (Pieter Wuille) Pull request description: This adds support for serialization of `std::vector<bool>`, as a prerequisite of #16702. ACKs for top commit: MarcoFalke: ACK 92528c2 (only looked at the diff on GitHub) practicalswift: ACK 92528c2 -- diff looks correct jamesob: ACK 92528c2 Tree-SHA512: 068246a55a889137098f817bf72d99fc3218a560d62a97c59cccc0392b110f6778843cee4e89d56f271ac64e03a0f64dc5e2cc22daa833fbbbe9234c5f42d7b9

Empact · 2019-08-27T00:08:16Z

ACK 92528c2

diff looks correct
confirmed the included test doesn't build without the source change

In file included from test/serialize_tests.cpp:5:
./serialize.h:602:7: error: no member named 'Serialize' in 'std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > >'
    a.Serialize(os);
    ~ ^
./serialize.h:721:11: note: in instantiation of function template specialization 'Serialize<CHashWriter, std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > > >' requested here
        ::Serialize(os, (*vi));
          ^
./serialize.h:727:5: note: in instantiation of function template specialization 'Serialize_impl<CHashWriter, bool, std::__1::allocator<bool>, bool>' requested here
    Serialize_impl(os, v, T());
    ^
./hash.h:154:11: note: in instantiation of function template specialization 'Serialize<CHashWriter, bool, std::__1::allocator<bool> >' requested here
        ::Serialize(*this, obj);
          ^
./hash.h:199:8: note: in instantiation of function template specialization 'CHashWriter::operator<<<std::__1::vector<bool, std::__1::allocator<bool> > >' requested here
    ss << obj;
       ^
test/serialize_tests.cpp:267:40: note: in instantiation of function template specialization 'SerializeHash<std::__1::vector<bool, std::__1::allocator<bool> > >' requested here
    BOOST_CHECK(SerializeHash(vec1) == SerializeHash(vec2));
                                       ^
1 error generated.

JeremyRubin · 2019-08-27T04:12:21Z

We don't want to define serialization to be a bit vector?

We're not using a std::vector<bool> serialized as bytes right now are we?

sipa · 2019-08-27T05:53:44Z

@JeremyRubin It's just for hashing, it doesn't really matter.

sipa · 2019-08-27T23:52:14Z

@JeremyRubin Longer answer: I think the expected serialization of std::vector<bool> is that it's serialized as any other vector would be serialized (compact length + serialization of each of the individual elements). If we want something that's serialized as a bitpacked array, we can introduce a BitPacker wrapper to serialize.h or so.

JeremyRubin · 2019-08-29T22:44:04Z

I would think the reason to worry about the serialization for hashing with a bitpacked array is that the incoming message can be hashed and it will be equivalent to the hash of the deserializaed struct.

Agree the best way to do this is by having a different type for a BitPacked serialization.

Summary: PR description: > This adds support for serialization of std::vector<bool>, as a prerequisite of [[bitcoin/bitcoin#16702 | PR16702]]. Backport of Core [[bitcoin/bitcoin#16730 | PR16730]]. Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7907

bitcoin/bitcoin#16730 Without this fix we got a following error: no member named 'Serialize' in 'std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > >' during clang build.

bitcoin@92528c2

merge bitcoin#17812, bitcoin#16702, bitcoin#16730, bitcoin#18023: supplying and using asmap to improve IP bucketing in addrman

bitcoin/bitcoin#16730 Without this fix we got a following error: no member named 'Serialize' in 'std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > >' during clang build.

bbd14ce Support serialization of std::vector<bool> (Pieter Wuille) Pull request description: Backport of bitcoin#16730, fixing the following clang warning: ``` ./serialize.h:631:44: warning: loop variable 'elem' is always a copy because the range of type 'const std::vector<bool, std::allocator<bool> >' does not return a reference [-Wrange-loop-analysis] for (const typename V::value_type& elem : v) { ^ ./serialize.h:484:77: note: in instantiation of function template specialization 'VectorFormatter<DefaultFormatter>::Ser<CHashWriter, std::vector<bool, std::allocator<bool> > >' requested here template<typename Stream> void Serialize(Stream &s) const { Formatter().Ser(s, m_object); } ^ ./serialize.h:806:7: note: in instantiation of function template specialization 'Wrapper<VectorFormatter<DefaultFormatter>, const std::vector<bool, std::allocator<bool> > &>::Serialize<CHashWriter>' requested here a.Serialize(os); ^ ./serialize.h:956:5: note: in instantiation of function template specialization 'Serialize<CHashWriter, Wrapper<VectorFormatter<DefaultFormatter>, const std::vector<bool, std::allocator<bool> > &> >' requested here Serialize(os, Using<VectorFormatter<DefaultFormatter>>(v)); ^ ./serialize.h:962:5: note: in instantiation of function template specialization 'Serialize_impl<CHashWriter, bool, std::allocator<bool>, bool>' requested here Serialize_impl(os, v, T()); ^ ./hash.h:247:11: note: in instantiation of function template specialization 'Serialize<CHashWriter, bool, std::allocator<bool> >' requested here ::Serialize(*this, obj); ^ ./hash.h:292:8: note: in instantiation of function template specialization 'CHashWriter::operator<<<std::vector<bool, std::allocator<bool> > >' requested here ss << obj; ^ init.cpp:1504:39: note: in instantiation of function template specialization 'SerializeHash<std::vector<bool, std::allocator<bool> > >' requested here const uint256 asmap_version = SerializeHash(asmap); ``` ACKs for top commit: furszy: good finding, utACK bbd14ce Tree-SHA512: ff26242545f727e6f6925e2f4f63f2616d89306c0bcc42bdf778eae95b85673ae5062637601fb5a27b49cb793de2ec144abcb2af63f62c18a0edbac5e07aea68

Support serialization of std::vector<bool>

92528c2

DrahtBot added the Tests label Aug 26, 2019

maflcko added Utils/log/libs and removed Tests labels Aug 26, 2019

fanquake merged commit 92528c2 into bitcoin:master Aug 26, 2019

kwvg mentioned this pull request Mar 8, 2021

partial #8149 and merge #19845, #19954: Complete the BIP155 implementation and upgrade to TORv3 dashpay/dash#4025

Closed

UdjinM6 mentioned this pull request Mar 9, 2021

merge #17812, #16702, #16730, #18023: supplying and using asmap to improve IP bucketing in addrman dashpay/dash#4028

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Mar 9, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

ed647db

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Mar 9, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

688985b

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Apr 21, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

be03114

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Apr 23, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

05d2cfb

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Apr 23, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

87419c9

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Apr 23, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

e3dbb6f

bitcoin@92528c2

kwvg added a commit to kwvg/dash that referenced this pull request Apr 23, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

abc1c3c

bitcoin@92528c2

kwvg pushed a commit to kwvg/dash that referenced this pull request May 11, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

a509aca

kwvg pushed a commit to kwvg/dash that referenced this pull request May 11, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

53aaa4b

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request May 15, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

a970798

kwvg pushed a commit to kwvg/dash that referenced this pull request May 18, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

2f86184

kwvg added a commit to kwvg/dash that referenced this pull request May 18, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

6a25151

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request May 19, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

0b0e8c5

PastaPastaPasta added a commit to dashpay/dash that referenced this pull request May 19, 2021

Merge pull request #4028 from kittywhiskers/asmap

2f555f8

merge bitcoin#17812, bitcoin#16702, bitcoin#16730, bitcoin#18023: supplying and using asmap to improve IP bucketing in addrman

kwvg added a commit to kwvg/dash that referenced this pull request May 20, 2021

Merge bitcoin#16730: Support serialization of std::vector<bool>

4042973

random-zebra mentioned this pull request Nov 5, 2021

[Util] Support serialization of std::vector<bool> PIVX-Project/PIVX#2629

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Support serialization of std::vector<bool> #16730

Support serialization of std::vector<bool> #16730

Uh oh!

sipa commented Aug 26, 2019

Uh oh!

maflcko commented Aug 26, 2019

Uh oh!

practicalswift commented Aug 26, 2019

Uh oh!

jamesob commented Aug 26, 2019

Uh oh!

Empact commented Aug 27, 2019

Uh oh!

JeremyRubin commented Aug 27, 2019

Uh oh!

sipa commented Aug 27, 2019

Uh oh!

sipa commented Aug 27, 2019

Uh oh!

JeremyRubin commented Aug 29, 2019

Uh oh!

Uh oh!

Support serialization of std::vector<bool> #16730

Support serialization of std::vector<bool> #16730

Uh oh!

Conversation

sipa commented Aug 26, 2019

Uh oh!

maflcko commented Aug 26, 2019

Uh oh!

practicalswift commented Aug 26, 2019

Uh oh!

jamesob commented Aug 26, 2019

Uh oh!

Empact commented Aug 27, 2019

Uh oh!

JeremyRubin commented Aug 27, 2019

Uh oh!

sipa commented Aug 27, 2019

Uh oh!

sipa commented Aug 27, 2019

Uh oh!

JeremyRubin commented Aug 29, 2019

Uh oh!

Uh oh!