peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
244- return DeserializeFileDB(pathAddr, addr, CLIENT_VERSION);
245+ DeserializeDB(ssPeers, addr, false);
246 }
247
248-bool CAddrDB::Read(CAddrMan& addr, CDataStream& ssPeers)
249+std::optional<bilingual_str> LoadAddrman(const ArgsManager& args, std::unique_ptr<CAddrMan>& addrman)
I would expect LoadAddrman() to return, eh… addrman, no? Like Foo x = LoadFoo();. Consider this (or ignore it):
0std::unique_ptr<CAddrMan> LoadAddrman(const ArgsManager& args, bilingual_str& error);
1
2bilingual_str error;
3node.addrman = LoadAddrman(args, error);
4if (!node.addrman) {
5 return InitError(error);
6}
259+ DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
260+ LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart);
261+ } catch (const DbNotFoundError&) {
262+ // Addrman can be in an inconsistent state after failure, reset it
263+ addrman = std::make_unique<CAddrMan>(/* deterministic */ false, /* consistency_check_ratio */ check_addrman);
264+ LogPrintf("Recreating peers.dat because it was not found\n");
nit: there is no re-creating if the file is missing:
0 LogPrintf("Creating peers.dat because it was not found\n");
or just “Creating peers.dat” or “peers.dat not found, creating an empty one”
197 FILE* file = fsbridge::fopen(path, "rb");
198 CAutoFile filein(file, SER_DISK, version);
199 if (filein.IsNull()) {
200- LogPrintf("Missing or invalid file %s\n", path.string());
201- return false;
202+ throw DbNotFoundError{""};
The string argument is excessive. Maybe inherit DbNotFoundError from std::exception and use throw DbNotFoundError;? Or, I guess, the proper C++ way to do that is:
0try {
1 throw std::system_error(std::make_error_code(std::errc::no_such_file_or_directory));
2} catch (const std::exception& e) {
3 auto se = dynamic_cast<const std::system_error*>(&e);
4 if (se != nullptr && se->code() == std::errc::no_such_file_or_directory) {
5 std::cout << "no peers.dat" << std::endl;
6 } else {
7 std::cout << e.what() << std::endl;
8 }
9}
"".
197 FILE* file = fsbridge::fopen(path, "rb");
198 CAutoFile filein(file, SER_DISK, version);
199 if (filein.IsNull()) {
200- LogPrintf("Missing or invalid file %s\n", path.string());
201- return false;
202+ throw DbNotFoundError{};
{} is not necessary
Not for me:
0addrdb.cpp:174:15: error: 'DbNotFoundError' does not refer to a value
1 throw DbNotFoundError;
2 ^
Concept ACK.
Like other open pulls to change or fix addrman behavior, it might be good to have functional/regression test coverage in place first.
16 #include <random.h>
17 #include <streams.h>
18 #include <sync.h>
19 #include <timedata.h>
20 #include <tinyformat.h>
21-#include <util/system.h>
1042 CDataStream ssPeers2 = AddrmanToStream(addrmanUncorrupted);
1043
1044 CAddrMan addrman2(/* asmap */ std::vector<bool>(), /* deterministic */ false, /* consistency_check_ratio */ 100);
1045 BOOST_CHECK(addrman2.size() == 0);
1046- BOOST_CHECK(CAddrDB::Read(addrman2, ssPeers2));
1047+ ReadFromStream(addrman2, ssPeers2);
fa63ebb21 CAddrDB removed in this commit; update these references
0--- a/src/test/addrman_tests.cpp
1+++ b/src/test/addrman_tests.cpp
2@@ -1002,7 +1002,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
3 BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
4 }
5
6-BOOST_AUTO_TEST_CASE(caddrdb_read)
7+BOOST_AUTO_TEST_CASE(load_addrman)
8 {
9 CAddrManUncorrupted addrmanUncorrupted;
10
11@@ -1047,7 +1047,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
12 }
13
14
15-BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
16+BOOST_AUTO_TEST_CASE(load_corrupted_addrman)
17 {
18 CAddrManCorrupted addrmanCorrupted;
207+ addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
208+ LogPrintf("Creating peers.dat because it was not found\n");
209+ DumpPeerAddresses(args, *addrman);
210+ } catch (const std::exception& e) {
211+ addrman = nullptr;
212+ return strprintf(_("Addrman invalid or corrupt (%s). If you believe this is a bug, please report it to %s. As a workaround you can move the database (%s) out of the way (rename, move, or delete) to use a fresh one on the next start."),
faa75278d076874194073a6cb397 suggestions
0 return strprintf(_("Invalid or corrupt peers.dat (%s). If you believe this is a bug, please report it to %s. As a workaround, you can move the file (%s) out of the way (rename, move, or delete) and bitcoind will create a new one on the next start."),
203+ DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
204+ LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart);
205+ } catch (const DbNotFoundError&) {
206+ // Addrman can be in an inconsistent state after failure, reset it
207+ addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
208+ LogPrintf("Creating peers.dat because it was not found\n");
Tested/reviewed approach ACK fabe5b5838812e58cc.
Cherry-picked #22831 onto this branch and the test coverage passed after the following changes; can update that PR if this is merged first:
0@@ -106,7 +106,7 @@ class AddrmanAsmapTest(BitcoinTestFramework):
1 self.log.info('Test bitcoind with missing peers.dat and addrman checks')
2 self.stop_node(0)
3 os.remove(os.path.join(self.datadir, 'peers.dat'))
4- with self.node.assert_debug_log([f'Missing or invalid file {self.datadir}/peers.dat', 'Recreating peers.dat']):
5+ with self.node.assert_debug_log(['Creating peers.dat because it was not found']):
6 self.start_node(0, ['-checkaddrman=1'])
7 self.node.getnodeaddresses()
8
9@@ -114,9 +114,12 @@ class AddrmanAsmapTest(BitcoinTestFramework):
10 self.log.info('Test bitcoind with peers.dat having invalid network magic and addrman checks')
11 self.stop_node(0)
12 shutil.copyfile(self.asmap_raw, os.path.join(self.datadir, 'peers.dat'))
13- with self.node.assert_debug_log(['Invalid network magic number', 'Recreating peers.dat']):
14- self.start_node(0, ['-checkaddrman=1'])
15- self.node.getnodeaddresses()
16+ msg = (
17+ 'Error: Addrman invalid or corrupt (Invalid network magic number). If you believe this is a bug, please '
18+ 'report it to https://github.com/bitcoin/bitcoin/issues. As a workaround you can move the database '
19+ f'("{self.datadir}/peers.dat") out of the way (rename, move, or delete) to use a fresh one on the next start.'
20+ )
21+ self.node.assert_start_raises_init_error(extra_args=['-checkaddrman=1'], expected_msg=msg)
22
23 def run_test(self):
24 self.node = self.nodes[0]
46@@ -52,6 +47,9 @@ class CBanDB
47 bool Read(banmap_t& banSet);
48 };
49
50+/** Returns an error string on failure */
fa035c1 suggestion if you retouch
0/** Load addresses from peers.dat into addrman. Returns an error string on failure */
1199@@ -1200,20 +1200,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1200 LogPrintf("Using /16 prefix for IP bucketing\n");
1201 }
1202
1203- auto check_addrman = std::clamp<int32_t>(args.GetArg("-checkaddrman", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), 0, 1000000);
1204- node.addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
1205-
1206- // Load addresses from peers.dat
ACK fa7003f949f21a0ba81388caff1ff986d8b43eed review / debug build clean on each commit, verified the test coverage of #22831 passes on this branch with the following changes
0--- a/test/functional/feature_addrman_asmap.py
1+++ b/test/functional/feature_addrman_asmap.py
2@@ -106,17 +106,21 @@ class AddrmanAsmapTest(BitcoinTestFramework):
3 self.log.info('Test bitcoind with missing peers.dat and addrman checks')
4 self.stop_node(0)
5 os.remove(os.path.join(self.datadir, 'peers.dat'))
6- with self.node.assert_debug_log([f'Missing or invalid file {self.datadir}/peers.dat', 'Recreating peers.dat']):
7+ msg = f'Creating peers.dat because the file was not found ("{self.datadir}/peers.dat")'
8+ with self.node.assert_debug_log(expected_msgs=[msg]):
9 self.start_node(0, ['-checkaddrman=1'])
10
11 def test_peers_dat_with_invalid_network_magic(self):
12 self.log.info('Test bitcoind with peers.dat having invalid network magic and addrman checks')
13 self.stop_node(0)
14 shutil.copyfile(self.asmap_raw, os.path.join(self.datadir, 'peers.dat'))
15- with self.node.assert_debug_log(['Invalid network magic number', 'Recreating peers.dat']):
16- self.start_node(0, ['-checkaddrman=1'])
17- self.node.getnodeaddresses()
18+ msg = (
19+ 'Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please '
20+ 'report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file '
21+ f'("{self.datadir}/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.'
22+ )
23+ self.node.assert_start_raises_init_error(extra_args=['-checkaddrman=1'], expected_msg=msg)
Tested on Pop!_OS and everything works as expected and mentioned in PR description.
peers.dat is replaced with a new file if it is corrupt or other issues.
A. Corrupt peers.dat file
bitcoind02021-09-07T18:05:46Z init message: Loading P2P addresses…
12021-09-07T18:05:46Z Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/home/prayank/.bitcoin/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
2Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/home/prayank/.bitcoin/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
32021-09-07T18:05:46Z Shutdown: In progress...
:warning: Not sure why same error printed twice in terminal running bitcoind. I see only one error message in debug.log:
02021-09-07T18:05:46Z init message: Loading P2P addresses…
12021-09-07T18:05:46Z Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/home/prayank/.bitcoin/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
22021-09-07T18:05:46Z Shutdown: In progress...
B. peers.dat from testnet used in regtest
bitcoind02021-09-07T18:14:33Z init message: Loading P2P addresses…
12021-09-07T18:14:33Z Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/home/prayank/.bitcoin/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
2Error: Invalid or corrupt peers.dat (Invalid network magic number). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/home/prayank/.bitcoin/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.
32021-09-07T18:14:33Z Shutdown: In progress...
Fix the issue by following the things mentioned in error: Delete peers.dat and restart bitcoind. No issues. :white_check_mark:
Init should only concern itself with the initialization order, not the
detailed initialization logic of every module.
Also, inlining logic into a method that is ~800 lines of code, makes it
impossible to unit test on its own.
No need to have a function that is only called in one place
git range-diff eb1f570 fa59c6d fa55c3 and verified the new tests fail on master, except “Check mocked addrman is valid”, as expected
69- assert_equal(self.nodes[0].getnodeaddresses(), [])
70+ self.nodes[0].assert_start_raises_init_error(
71+ expected_msg=init_error(
72+ "Unsupported format of addrman database: 1. It is compatible with "
73+ "formats >=111, but the maximum supported by this version of "
74+ f"{self.config['environment']['PACKAGE_NAME']} is 3.: (.+)"
This is somewhat confusing or designates a corrupted file, not one from the future. How could the file be format=1 and only be compatible with formats larger than 111? If the above is changed as:
0-write_addrman(peers_dat, lowest_compatible=111)
1+write_addrman(peers_dat, format=115, lowest_compatible=111)
it will be a bit more realistic.
Feel free to ignore as out of the scope of this PR, given that a similar message is already in master.
Instead of “fixing” the test, I think rejecting formats that are less than lowest_compatible seems like a good sanity check. Though, this would be an unrelated fix in a separate pull:
0diff --git a/src/addrman.cpp b/src/addrman.cpp
1index 174b3a654c..bcffe64de3 100644
2--- a/src/addrman.cpp
3+++ b/src/addrman.cpp
4@@ -243,7 +243,7 @@ void CAddrMan::Unserialize(Stream& s_)
5 uint8_t compat;
6 s >> compat;
7 const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
8- if (lowest_compatible > FILE_FORMAT) {
9+ if (format < lowest_compatible || lowest_compatible > FILE_FORMAT) {
10 throw std::ios_base::failure(strprintf(
11 "Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
12 "but the maximum supported by this version of %s is %u.",
tACK https://github.com/bitcoin/bitcoin/commit/fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae
nit: Will be better if error message was printed only once #22762 (comment)
nit: Will be better if error message was printed only once #22762 (comment)
InitErrors are printed to the debug log and stderr. I think it makes sense to print them to both instead of only one.
195+ const auto path_addr{args.GetDataDirNet() / "peers.dat"};
196+ try {
197+ DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
198+ LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart);
199+ } catch (const DbNotFoundError&) {
200+ // Addrman can be in an inconsistent state after failure, reset it
Just to clarify that, strictly, this isn’t needed. DbNotFoundError means that the file wasn’t found, so addrman shouldn’t be touched at all.
Does anyone have opinions on whether to remove it or not?
I meant recreating the addrman can be skipped, because it wasn’t touched. Saves two lines of code:
0diff --git a/src/addrdb.cpp b/src/addrdb.cpp
1index 1e73750ce3..a86f4c3789 100644
2--- a/src/addrdb.cpp
3+++ b/src/addrdb.cpp
4@@ -192,8 +192,6 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
5 DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
6 LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart);
7 } catch (const DbNotFoundError&) {
8- // Addrman can be in an inconsistent state after failure, reset it
9- addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
10 LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
11 DumpPeerAddresses(args, *addrman);
12 } catch (const std::exception& e) {
DumpPeerAddresses(args, *addrman); wouldn’t work either.
The pointer is initialized in the beginning of the function
ah, well, then it is ok. ACK.
