Skip to content

Commit b5c88a5

Browse files
author
MarcoFalke
committed
Merge bitcoin/bitcoin#26909: net: prevent peers.dat corruptions by only serializing once
5eabb61 addrdb: Only call Serialize() once (Martin Zumsande) da6c7ae hash: add HashedSourceWriter (Martin Zumsande) Pull request description: There have been various reports of corruption of `peers.dat` recently, see #26599. As explained in [this post](bitcoin/bitcoin#26599 (comment)) in more detail, the underlying issue is likely that we currently serialize `AddrMan` twice - once for the file stream, once for the hasher that helps create the checksum - and if `AddrMan` changes in between these two calls, the checksum doesn't match the data and the resulting `peers.dat` is corrupted. This PR attempts to fix this by introducing and using `HashedSourceWriter` - a class that keeps a running hash while serializing data, similar to the existing `CHashVerifier` which does the analogous thing while unserializing data. Something like this was suggested before, see bitcoin/bitcoin#10248 (comment). Fixes #26599 (not by changing the behavior in case of a crash, but by hopefully fixing the underlying cause of these corruptions). ACKs for top commit: sipa: utACK 5eabb61 naumenkogs: utACK 5eabb61 Tree-SHA512: f19ad37c37088d3a9825c60de2efe85cc2b7a21b79b9156024d33439e021443ef977a5f8424a7981bcc13d73d11e30eaa82de60e14d88b3568a67b03e02b153b
2 parents 05e3468 + 5eabb61 commit b5c88a5

File tree

4 files changed

+43
-6
lines changed

4 files changed

+43
-6
lines changed

src/addrdb.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,9 @@ bool SerializeDB(Stream& stream, const Data& data)
3434
{
3535
// Write and commit header, data
3636
try {
37-
CHashWriter hasher(stream.GetType(), stream.GetVersion());
38-
stream << Params().MessageStart() << data;
39-
hasher << Params().MessageStart() << data;
40-
stream << hasher.GetHash();
37+
HashedSourceWriter hashwriter{stream};
38+
hashwriter << Params().MessageStart() << data;
39+
stream << hashwriter.GetHash();
4140
} catch (const std::exception& e) {
4241
return error("%s: Serialize or I/O error - %s", __func__, e.what());
4342
}

src/addrman.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,8 +1178,7 @@ void AddrMan::Unserialize(Stream& s_)
11781178
}
11791179

11801180
// explicit instantiation
1181-
template void AddrMan::Serialize(CHashWriter& s) const;
1182-
template void AddrMan::Serialize(CAutoFile& s) const;
1181+
template void AddrMan::Serialize(HashedSourceWriter<CAutoFile>& s) const;
11831182
template void AddrMan::Serialize(CDataStream& s) const;
11841183
template void AddrMan::Unserialize(CAutoFile& s);
11851184
template void AddrMan::Unserialize(CHashVerifier<CAutoFile>& s);

src/hash.h

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#ifndef BITCOIN_HASH_H
77
#define BITCOIN_HASH_H
88

9+
#include <attributes.h>
910
#include <crypto/common.h>
1011
#include <crypto/ripemd160.h>
1112
#include <crypto/sha256.h>
@@ -199,6 +200,30 @@ class CHashVerifier : public CHashWriter
199200
}
200201
};
201202

203+
/** Writes data to an underlying source stream, while hashing the written data. */
204+
template <typename Source>
205+
class HashedSourceWriter : public CHashWriter
206+
{
207+
private:
208+
Source& m_source;
209+
210+
public:
211+
explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : CHashWriter{source.GetType(), source.GetVersion()}, m_source{source} {}
212+
213+
void write(Span<const std::byte> src)
214+
{
215+
m_source.write(src);
216+
CHashWriter::write(src);
217+
}
218+
219+
template <typename T>
220+
HashedSourceWriter& operator<<(const T& obj)
221+
{
222+
::Serialize(*this, obj);
223+
return *this;
224+
}
225+
};
226+
202227
/** Compute the 256-bit hash of an object's serialization. */
203228
template<typename T>
204229
uint256 SerializeHash(const T& obj, int nType=SER_GETHASH, int nVersion=PROTOCOL_VERSION)

src/test/streams_tests.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,4 +500,18 @@ BOOST_AUTO_TEST_CASE(streams_buffered_file_rand)
500500
fs::remove(streams_test_filename);
501501
}
502502

503+
BOOST_AUTO_TEST_CASE(streams_hashed)
504+
{
505+
CDataStream stream(SER_NETWORK, INIT_PROTO_VERSION);
506+
HashedSourceWriter hash_writer{stream};
507+
const std::string data{"bitcoin"};
508+
hash_writer << data;
509+
510+
CHashVerifier hash_verifier{&stream};
511+
std::string result;
512+
hash_verifier >> result;
513+
BOOST_CHECK_EQUAL(data, result);
514+
BOOST_CHECK_EQUAL(hash_writer.GetHash(), hash_verifier.GetHash());
515+
}
516+
503517
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)