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:
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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):
std::unique_ptr<CAddrMan> LoadAddrman(const ArgsManager& args, bilingual_str& error);
bilingual_str error;
node.addrman = LoadAddrman(args, error);
if (!node.addrman) {
return InitError(error);
}
This would make the code more verbose. In general I rather standardize error handling how it is done in rust, which the current code is closer to than yours, I think.
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:
LogPrintf("Creating peers.dat because it was not found\n");
or just "Creating peers.dat" or "peers.dat not found, creating an empty one"
Thanks, fixed
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:
try {
throw std::system_error(std::make_error_code(std::errc::no_such_file_or_directory));
} catch (const std::exception& e) {
auto se = dynamic_cast<const std::system_error*>(&e);
if (se != nullptr && se->code() == std::errc::no_such_file_or_directory) {
std::cout << "no peers.dat" << std::endl;
} else {
std::cout << e.what() << std::endl;
}
}
Thanks, removed "".
Concept ACK
force pushed to address review comments
ACK fa1704a2f49eaedf5788d82294485f3a8e17a6c8
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{};
nit: {} is not necessary
Not for me:
addrdb.cpp:174:15: error: 'DbNotFoundError' does not refer to a value
throw DbNotFoundError;
^
ah, oh, yes of course, sorry for the noise
ACK faff4fd4e00621a02554233effa8f144990f3dd8
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.
ACK b4cc1c52e103f6fc3969daefc1ba11ac4af4d606
ACK faa75278d076874194073a6cb3974f45de051ffc
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>
fa9aed72 while fixing the addrman includes may as well do these too: #22740 (comment)
Thanks, done
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
--- a/src/test/addrman_tests.cpp
+++ b/src/test/addrman_tests.cpp
@@ -1002,7 +1002,7 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
}
-BOOST_AUTO_TEST_CASE(caddrdb_read)
+BOOST_AUTO_TEST_CASE(load_addrman)
{
CAddrManUncorrupted addrmanUncorrupted;
@@ -1047,7 +1047,7 @@ BOOST_AUTO_TEST_CASE(caddrdb_read)
}
-BOOST_AUTO_TEST_CASE(caddrdb_read_corrupted)
+BOOST_AUTO_TEST_CASE(load_corrupted_addrman)
{
CAddrManCorrupted addrmanCorrupted;
Thanks, done
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
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."),
Thanks, done
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");
faa75278d076874194073a6cb3974f45de051ffc Perhaps also log to the user the location where the file was expected to be (as is currently the case).
Good catch, done
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:
@@ -106,7 +106,7 @@ class AddrmanAsmapTest(BitcoinTestFramework):
self.log.info('Test bitcoind with missing peers.dat and addrman checks')
self.stop_node(0)
os.remove(os.path.join(self.datadir, 'peers.dat'))
- with self.node.assert_debug_log([f'Missing or invalid file {self.datadir}/peers.dat', 'Recreating peers.dat']):
+ with self.node.assert_debug_log(['Creating peers.dat because it was not found']):
self.start_node(0, ['-checkaddrman=1'])
self.node.getnodeaddresses()
@@ -114,9 +114,12 @@ class AddrmanAsmapTest(BitcoinTestFramework):
self.log.info('Test bitcoind with peers.dat having invalid network magic and addrman checks')
self.stop_node(0)
shutil.copyfile(self.asmap_raw, os.path.join(self.datadir, 'peers.dat'))
- with self.node.assert_debug_log(['Invalid network magic number', 'Recreating peers.dat']):
- self.start_node(0, ['-checkaddrman=1'])
- self.node.getnodeaddresses()
+ msg = (
+ 'Error: Addrman invalid or corrupt (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 database '
+ f'("{self.datadir}/peers.dat") out of the way (rename, move, or delete) to use a fresh one on the next start.'
+ )
+ self.node.assert_start_raises_init_error(extra_args=['-checkaddrman=1'], expected_msg=msg)
def run_test(self):
self.node = self.nodes[0]
ACK fa7003f949f21a0ba81388caff1ff986d8b43eed
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
/** Load addresses from peers.dat into addrman. Returns an error string on failure */
I don't think other modules care much about the exact file name, so I didn't mention it, as it is self-documented in the addrdb implementation file (cpp) already. And the "Load addresses" part should already be clear from the function name.
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
fa035c1 perhaps keep this comment
I don't think it is important to document the file name in init.cpp, so I removed it
ACK fa7003f949f21a0ba81388caff1ff986d8b43eed review / debug build clean on each commit, verified the test coverage of #22831 passes on this branch with the following changes
--- a/test/functional/feature_addrman_asmap.py
+++ b/test/functional/feature_addrman_asmap.py
@@ -106,17 +106,21 @@ class AddrmanAsmapTest(BitcoinTestFramework):
self.log.info('Test bitcoind with missing peers.dat and addrman checks')
self.stop_node(0)
os.remove(os.path.join(self.datadir, 'peers.dat'))
- with self.node.assert_debug_log([f'Missing or invalid file {self.datadir}/peers.dat', 'Recreating peers.dat']):
+ msg = f'Creating peers.dat because the file was not found ("{self.datadir}/peers.dat")'
+ with self.node.assert_debug_log(expected_msgs=[msg]):
self.start_node(0, ['-checkaddrman=1'])
def test_peers_dat_with_invalid_network_magic(self):
self.log.info('Test bitcoind with peers.dat having invalid network magic and addrman checks')
self.stop_node(0)
shutil.copyfile(self.asmap_raw, os.path.join(self.datadir, 'peers.dat'))
- with self.node.assert_debug_log(['Invalid network magic number', 'Recreating peers.dat']):
- self.start_node(0, ['-checkaddrman=1'])
- self.node.getnodeaddresses()
+ msg = (
+ '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 '
+ f'("{self.datadir}/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start.'
+ )
+ 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.
<details> <summary>Details</summary>
</details>
A. Corrupt peers.dat file
bitcoind2021-09-07T18:05:46Z init message: Loading P2P addresses…
2021-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.
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.
2021-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:
2021-09-07T18:05:46Z init message: Loading P2P addresses…
2021-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.
2021-09-07T18:05:46Z Shutdown: In progress...
B. peers.dat from testnet used in regtest
bitcoind2021-09-07T18:14:33Z init message: Loading P2P addresses…
2021-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.
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.
2021-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
Rebased (only change is in tests)
Code review re-ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae per 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:
-write_addrman(peers_dat, lowest_compatible=111)
+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:
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 174b3a654c..bcffe64de3 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -243,7 +243,7 @@ void CAddrMan::Unserialize(Stream& s_)
uint8_t compat;
s >> compat;
const uint8_t lowest_compatible = compat - INCOMPATIBILITY_BASE;
- if (lowest_compatible > FILE_FORMAT) {
+ if (format < lowest_compatible || lowest_compatible > FILE_FORMAT) {
throw std::ios_base::failure(strprintf(
"Unsupported format of addrman database: %u. It is compatible with formats >=%u, "
"but the maximum supported by this version of %s is %u.",
+1, maybe each condition deserves its own error message.
ACK fa55c3dc1b4bbdc6a53bd11fa6c0b2ec6bbb64ae
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?
But we need to distinguish somehow this from the other error case, so that we don't signal an error to the caller in this case?
I meant recreating the addrman can be skipped, because it wasn't touched. Saves two lines of code:
diff --git a/src/addrdb.cpp b/src/addrdb.cpp
index 1e73750ce3..a86f4c3789 100644
--- a/src/addrdb.cpp
+++ b/src/addrdb.cpp
@@ -192,8 +192,6 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
DeserializeFileDB(path_addr, *addrman, CLIENT_VERSION);
LogPrintf("Loaded %i addresses from peers.dat %dms\n", addrman->size(), GetTimeMillis() - nStart);
} catch (const DbNotFoundError&) {
- // Addrman can be in an inconsistent state after failure, reset it
- addrman = std::make_unique<CAddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
LogPrintf("Creating peers.dat because the file was not found (%s)\n", path_addr);
DumpPeerAddresses(args, *addrman);
} catch (const std::exception& e) {
This would leave the unique_ptr empty and not signal error to the caller. So the caller will continue, possibly dereferencing the empty unique_ptr, leading to undefined behavior?
The pointer is initialized in the beginning of the function. Otherwise DumpPeerAddresses(args, *addrman); wouldn't work either.
Removal LGTM. Built with the suggested diff and tested.
The pointer is initialized in the beginning of the function
ah, well, then it is ok. ACK.
I'm not sure it's entirely desirable to require user intervention when downgrading. When there is actual corruption, this may make sense as it's a sign of a deeper problem, but wiping peers.dat when downgrading to an incompatible version seems entirely expected to me.