Skip to content

Commit fac81af

Browse files
MarcoFalkesipaajtowns
committed
Use serialization parameters for CAddress serialization
This also cleans up the addrman (de)serialization code paths to only allow `Disk` serialization. Some unit tests previously forced a `Network` serialization, which does not make sense, because Bitcoin Core in production will always `Disk` serialize. This cleanup idea was suggested by Pieter Wuille and implemented by Anthony Towns. Co-authored-by: Pieter Wuille <pieter@wuille.net> Co-authored-by: Anthony Towns <aj@erisian.com.au>
1 parent faec591 commit fac81af

19 files changed

+276
-215
lines changed

‎src/addrdb.cpp

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -47,16 +47,16 @@ bool SerializeDB(Stream& stream, const Data& data)
4747
}
4848

4949
template <typename Data>
50-
bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data, int version)
50+
bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data& data)
5151
{
5252
// Generate random temporary filename
5353
const uint16_t randv{GetRand<uint16_t>()};
5454
std::string tmpfn = strprintf("%s.%04x", prefix, randv);
5555

56-
// open temp output file, and associate with CAutoFile
56+
// open temp output file
5757
fs::path pathTmp = gArgs.GetDataDirNet() / fs::u8path(tmpfn);
5858
FILE *file = fsbridge::fopen(pathTmp, "wb");
59-
CAutoFile fileout(file, SER_DISK, version);
59+
AutoFile fileout{file};
6060
if (fileout.IsNull()) {
6161
fileout.fclose();
6262
remove(pathTmp);
@@ -86,9 +86,9 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
8686
}
8787

8888
template <typename Stream, typename Data>
89-
void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
89+
void DeserializeDB(Stream& stream, Data&& data, bool fCheckSum = true)
9090
{
91-
CHashVerifier<Stream> verifier(&stream);
91+
HashVerifier verifier{stream};
9292
// de-serialize file header (network specific magic number) and ..
9393
unsigned char pchMsgTmp[4];
9494
verifier >> pchMsgTmp;
@@ -111,11 +111,10 @@ void DeserializeDB(Stream& stream, Data& data, bool fCheckSum = true)
111111
}
112112

113113
template <typename Data>
114-
void DeserializeFileDB(const fs::path& path, Data& data, int version)
114+
void DeserializeFileDB(const fs::path& path, Data&& data)
115115
{
116-
// open input file, and associate with CAutoFile
117116
FILE* file = fsbridge::fopen(path, "rb");
118-
CAutoFile filein(file, SER_DISK, version);
117+
AutoFile filein{file};
119118
if (filein.IsNull()) {
120119
throw DbNotFoundError{};
121120
}
@@ -175,10 +174,10 @@ bool CBanDB::Read(banmap_t& banSet)
175174
bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr)
176175
{
177176
const auto pathAddr = args.GetDataDirNet() / "peers.dat";
178-
return SerializeFileDB("peers", pathAddr, addr, CLIENT_VERSION);
177+
return SerializeFileDB("peers", pathAddr, addr);
179178
}
180179

181-
void ReadFromStream(AddrMan& addr, CDataStream& ssPeers)
180+
void ReadFromStream(AddrMan& addr, DataStream& ssPeers)
182181
{
183182
DeserializeDB(ssPeers, addr, false);
184183
}
@@ -191,7 +190,7 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
191190
const auto start{SteadyClock::now()};
192191
const auto path_addr{args.GetDataDirNet() / "peers.dat"};
193192
try {
194-
DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
193+
DeserializeFileDB(path_addr, *addrman);
195194
LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->Size(), Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
196195
} catch (const DbNotFoundError&) {
197196
// Addrman can be in an inconsistent state after failure, reset it
@@ -217,14 +216,14 @@ util::Result<std::unique_ptr<AddrMan>> LoadAddrman(const NetGroupManager& netgro
217216
void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& anchors)
218217
{
219218
LOG_TIME_SECONDS(strprintf("Flush %d outbound block-relay-only peer addresses to anchors.dat", anchors.size()));
220-
SerializeFileDB("anchors", anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
219+
SerializeFileDB("anchors", anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
221220
}
222221

223222
std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
224223
{
225224
std::vector<CAddress> anchors;
226225
try {
227-
DeserializeFileDB(anchors_db_path, anchors, CLIENT_VERSION | ADDRV2_FORMAT);
226+
DeserializeFileDB(anchors_db_path, WithParams(CAddress::V2_DISK, anchors));
228227
LogPrintf("Loaded %i addresses from %s\n", anchors.size(), fs::quoted(fs::PathToString(anchors_db_path.filename())));
229228
} catch (const std::exception&) {
230229
anchors.clear();

‎src/addrdb.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@
1616
class ArgsManager;
1717
class AddrMan;
1818
class CAddress;
19-
class CDataStream;
19+
class DataStream;
2020
class NetGroupManager;
2121

22-
bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
2322
/** Only used by tests. */
24-
void ReadFromStream(AddrMan& addr, CDataStream& ssPeers);
23+
void ReadFromStream(AddrMan& addr, DataStream& ssPeers);
24+
25+
bool DumpPeerAddresses(const ArgsManager& args, const AddrMan& addr);
2526

2627
/** Access to the banlist database (banlist.json) */
2728
class CBanDB

‎src/addrman.cpp

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,7 @@ void AddrManImpl::Serialize(Stream& s_) const
171171
*/
172172

173173
// Always serialize in the latest version (FILE_FORMAT).
174-
175-
OverrideStream<Stream> s(&s_, s_.GetType(), s_.GetVersion() | ADDRV2_FORMAT);
174+
ParamsStream s{CAddress::V2_DISK, s_};
176175

177176
s << static_cast<uint8_t>(FILE_FORMAT);
178177

@@ -236,14 +235,8 @@ void AddrManImpl::Unserialize(Stream& s_)
236235
Format format;
237236
s_ >> Using<CustomUintFormatter<1>>(format);
238237

239-
int stream_version = s_.GetVersion();
240-
if (format >= Format::V3_BIP155) {
241-
// Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
242-
// unserialize methods know that an address in addrv2 format is coming.
243-
stream_version |= ADDRV2_FORMAT;
244-
}
245-
246-
OverrideStream<Stream> s(&s_, s_.GetType(), stream_version);
238+
const auto ser_params = (format >= Format::V3_BIP155 ? CAddress::V2_DISK : CAddress::V1_DISK);
239+
ParamsStream s{ser_params, s_};
247240

248241
uint8_t compat;
249242
s >> compat;
@@ -1249,12 +1242,12 @@ void AddrMan::Unserialize(Stream& s_)
12491242
}
12501243

12511244
// explicit instantiation
1252-
template void AddrMan::Serialize(HashedSourceWriter<CAutoFile>& s) const;
1253-
template void AddrMan::Serialize(CDataStream& s) const;
1254-
template void AddrMan::Unserialize(CAutoFile& s);
1255-
template void AddrMan::Unserialize(CHashVerifier<CAutoFile>& s);
1256-
template void AddrMan::Unserialize(CDataStream& s);
1257-
template void AddrMan::Unserialize(CHashVerifier<CDataStream>& s);
1245+
template void AddrMan::Serialize(HashedSourceWriter<AutoFile>&) const;
1246+
template void AddrMan::Serialize(DataStream&) const;
1247+
template void AddrMan::Unserialize(AutoFile&);
1248+
template void AddrMan::Unserialize(HashVerifier<AutoFile>&);
1249+
template void AddrMan::Unserialize(DataStream&);
1250+
template void AddrMan::Unserialize(HashVerifier<DataStream>&);
12581251

12591252
size_t AddrMan::Size(std::optional<Network> net, std::optional<bool> in_new) const
12601253
{

‎src/hash.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -234,18 +234,18 @@ class CHashVerifier : public CHashWriter
234234

235235
/** Writes data to an underlying source stream, while hashing the written data. */
236236
template <typename Source>
237-
class HashedSourceWriter : public CHashWriter
237+
class HashedSourceWriter : public HashWriter
238238
{
239239
private:
240240
Source& m_source;
241241

242242
public:
243-
explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : CHashWriter{source.GetType(), source.GetVersion()}, m_source{source} {}
243+
explicit HashedSourceWriter(Source& source LIFETIMEBOUND) : HashWriter{}, m_source{source} {}
244244

245245
void write(Span<const std::byte> src)
246246
{
247247
m_source.write(src);
248-
CHashWriter::write(src);
248+
HashWriter::write(src);
249249
}
250250

251251
template <typename T>

‎src/net.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,8 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
202202
const auto one_week{7 * 24h};
203203
std::vector<CAddress> vSeedsOut;
204204
FastRandomContext rng;
205-
CDataStream s(vSeedsIn, SER_NETWORK, PROTOCOL_VERSION | ADDRV2_FORMAT);
205+
DataStream underlying_stream{vSeedsIn};
206+
ParamsStream s{CAddress::V2_NETWORK, underlying_stream};
206207
while (!s.eof()) {
207208
CService endpoint;
208209
s >> endpoint;

‎src/net_processing.cpp

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,8 +1415,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer)
14151415

14161416
const bool tx_relay{!RejectIncomingTxs(pnode)};
14171417
m_connman.PushMessage(&pnode, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, my_services, nTime,
1418-
your_services, addr_you, // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
1419-
my_services, CService(), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
1418+
your_services, WithParams(CNetAddr::V1, addr_you), // Together the pre-version-31402 serialization of CAddress "addrYou" (without nTime)
1419+
my_services, WithParams(CNetAddr::V1, CService{}), // Together the pre-version-31402 serialization of CAddress "addrMe" (without nTime)
14201420
nonce, strSubVersion, nNodeStartingHeight, tx_relay));
14211421

14221422
if (fLogIPs) {
@@ -3281,7 +3281,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
32813281
nTime = 0;
32823282
}
32833283
vRecv.ignore(8); // Ignore the addrMe service bits sent by the peer
3284-
vRecv >> addrMe;
3284+
vRecv >> WithParams(CNetAddr::V1, addrMe);
32853285
if (!pfrom.IsInboundConn())
32863286
{
32873287
m_addrman.SetServices(pfrom.addr, nServices);
@@ -3660,17 +3660,17 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
36603660
}
36613661

36623662
if (msg_type == NetMsgType::ADDR || msg_type == NetMsgType::ADDRV2) {
3663-
int stream_version = vRecv.GetVersion();
3664-
if (msg_type == NetMsgType::ADDRV2) {
3665-
// Add ADDRV2_FORMAT to the version so that the CNetAddr and CAddress
3663+
const auto ser_params{
3664+
msg_type == NetMsgType::ADDRV2 ?
3665+
// Set V2 param so that the CNetAddr and CAddress
36663666
// unserialize methods know that an address in v2 format is coming.
3667-
stream_version |= ADDRV2_FORMAT;
3668-
}
3667+
CAddress::V2_NETWORK :
3668+
CAddress::V1_NETWORK,
3669+
};
36693670

3670-
OverrideStream<CDataStream> s(&vRecv, vRecv.GetType(), stream_version);
36713671
std::vector<CAddress> vAddr;
36723672

3673-
s >> vAddr;
3673+
vRecv >> WithParams(ser_params, vAddr);
36743674

36753675
if (!SetupAddressRelay(pfrom, *peer)) {
36763676
LogPrint(BCLog::NET, "ignoring %s message from %s peer=%d\n", msg_type, pfrom.ConnectionTypeAsString(), pfrom.GetId());
@@ -5272,15 +5272,15 @@ void PeerManagerImpl::MaybeSendAddr(CNode& node, Peer& peer, std::chrono::micros
52725272
if (peer.m_addrs_to_send.empty()) return;
52735273

52745274
const char* msg_type;
5275-
int make_flags;
5275+
CNetAddr::Encoding ser_enc;
52765276
if (peer.m_wants_addrv2) {
52775277
msg_type = NetMsgType::ADDRV2;
5278-
make_flags = ADDRV2_FORMAT;
5278+
ser_enc = CNetAddr::Encoding::V2;
52795279
} else {
52805280
msg_type = NetMsgType::ADDR;
5281-
make_flags = 0;
5281+
ser_enc = CNetAddr::Encoding::V1;
52825282
}
5283-
m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(make_flags, msg_type, peer.m_addrs_to_send));
5283+
m_connman.PushMessage(&node, CNetMsgMaker(node.GetCommonVersion()).Make(msg_type, WithParams(CAddress::SerParams{{ser_enc}, CAddress::Format::Network}, peer.m_addrs_to_send)));
52845284
peer.m_addrs_to_send.clear();
52855285

52865286
// we only send the big addr message once

‎src/netaddress.h

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,6 @@
2424
#include <string>
2525
#include <vector>
2626

27-
/**
28-
* A flag that is ORed into the protocol version to designate that addresses
29-
* should be serialized in (unserialized from) v2 format (BIP155).
30-
* Make sure that this does not collide with any of the values in `version.h`
31-
* or with `SERIALIZE_TRANSACTION_NO_WITNESS`.
32-
*/
33-
static constexpr int ADDRV2_FORMAT = 0x20000000;
34-
3527
/**
3628
* A network type.
3729
* @note An address may belong to more than one network, for example `10.0.0.1`
@@ -220,13 +212,23 @@ class CNetAddr
220212
return IsIPv4() || IsIPv6() || IsTor() || IsI2P() || IsCJDNS();
221213
}
222214

215+
enum class Encoding {
216+
V1,
217+
V2, //!< BIP155 encoding
218+
};
219+
struct SerParams {
220+
const Encoding enc;
221+
};
222+
static constexpr SerParams V1{Encoding::V1};
223+
static constexpr SerParams V2{Encoding::V2};
224+
223225
/**
224226
* Serialize to a stream.
225227
*/
226228
template <typename Stream>
227229
void Serialize(Stream& s) const
228230
{
229-
if (s.GetVersion() & ADDRV2_FORMAT) {
231+
if (s.GetParams().enc == Encoding::V2) {
230232
SerializeV2Stream(s);
231233
} else {
232234
SerializeV1Stream(s);
@@ -239,7 +241,7 @@ class CNetAddr
239241
template <typename Stream>
240242
void Unserialize(Stream& s)
241243
{
242-
if (s.GetVersion() & ADDRV2_FORMAT) {
244+
if (s.GetParams().enc == Encoding::V2) {
243245
UnserializeV2Stream(s);
244246
} else {
245247
UnserializeV1Stream(s);

‎src/protocol.h

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -390,35 +390,43 @@ class CAddress : public CService
390390
CAddress(CService ipIn, ServiceFlags nServicesIn) : CService{ipIn}, nServices{nServicesIn} {};
391391
CAddress(CService ipIn, ServiceFlags nServicesIn, NodeSeconds time) : CService{ipIn}, nTime{time}, nServices{nServicesIn} {};
392392

393-
SERIALIZE_METHODS(CAddress, obj)
393+
enum class Format {
394+
Disk,
395+
Network,
396+
};
397+
struct SerParams : CNetAddr::SerParams {
398+
const Format fmt;
399+
};
400+
static constexpr SerParams V1_NETWORK{{CNetAddr::Encoding::V1}, Format::Network};
401+
static constexpr SerParams V2_NETWORK{{CNetAddr::Encoding::V2}, Format::Network};
402+
static constexpr SerParams V1_DISK{{CNetAddr::Encoding::V1}, Format::Disk};
403+
static constexpr SerParams V2_DISK{{CNetAddr::Encoding::V2}, Format::Disk};
404+
405+
SERIALIZE_METHODS_PARAMS(CAddress, obj, SerParams, params)
394406
{
395-
// CAddress has a distinct network serialization and a disk serialization, but it should never
396-
// be hashed (except through CHashWriter in addrdb.cpp, which sets SER_DISK), and it's
397-
// ambiguous what that would mean. Make sure no code relying on that is introduced:
398-
assert(!(s.GetType() & SER_GETHASH));
399407
bool use_v2;
400-
if (s.GetType() & SER_DISK) {
408+
if (params.fmt == Format::Disk) {
401409
// In the disk serialization format, the encoding (v1 or v2) is determined by a flag version
402410
// that's part of the serialization itself. ADDRV2_FORMAT in the stream version only determines
403411
// whether V2 is chosen/permitted at all.
404412
uint32_t stored_format_version = DISK_VERSION_INIT;
405-
if (s.GetVersion() & ADDRV2_FORMAT) stored_format_version |= DISK_VERSION_ADDRV2;
413+
if (params.enc == Encoding::V2) stored_format_version |= DISK_VERSION_ADDRV2;
406414
READWRITE(stored_format_version);
407415
stored_format_version &= ~DISK_VERSION_IGNORE_MASK; // ignore low bits
408416
if (stored_format_version == 0) {
409417
use_v2 = false;
410-
} else if (stored_format_version == DISK_VERSION_ADDRV2 && (s.GetVersion() & ADDRV2_FORMAT)) {
411-
// Only support v2 deserialization if ADDRV2_FORMAT is set.
418+
} else if (stored_format_version == DISK_VERSION_ADDRV2 && params.enc == Encoding::V2) {
419+
// Only support v2 deserialization if V2 is set.
412420
use_v2 = true;
413421
} else {
414422
throw std::ios_base::failure("Unsupported CAddress disk format version");
415423
}
416424
} else {
425+
assert(params.fmt == Format::Network);
417426
// In the network serialization format, the encoding (v1 or v2) is determined directly by
418-
// the value of ADDRV2_FORMAT in the stream version, as no explicitly encoded version
427+
// the value of enc in the stream params, as no explicitly encoded version
419428
// exists in the stream.
420-
assert(s.GetType() & SER_NETWORK);
421-
use_v2 = s.GetVersion() & ADDRV2_FORMAT;
429+
use_v2 = params.enc == Encoding::V2;
422430
}
423431

424432
READWRITE(Using<LossyChronoFormatter<uint32_t>>(obj.nTime));
@@ -432,8 +440,8 @@ class CAddress : public CService
432440
READWRITE(Using<CustomUintFormatter<8>>(obj.nServices));
433441
}
434442
// Invoke V1/V2 serializer for CService parent object.
435-
OverrideStream<Stream> os(&s, s.GetType(), use_v2 ? ADDRV2_FORMAT : 0);
436-
SerReadWriteMany(os, ser_action, AsBase<CService>(obj));
443+
const auto ser_params{use_v2 ? CNetAddr::V2 : CNetAddr::V1};
444+
READWRITE(WithParams(ser_params, AsBase<CService>(obj)));
437445
}
438446

439447
//! Always included in serialization. The behavior is unspecified if the value is not representable as uint32_t.

0 commit comments

Comments
 (0)