fixes #24188 (also see #22762 (comment)) When downgrading, a peers.dat with a future version that has a minimum required version larger than the downgraded Bitcoin Core version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with a new empty one.
Part of the reason why I am using strncmp is because AddrMan (AddrManImpl) -> Unserialize might be called somewhere else, so I didn’t want to touch it.
I should probably re-work AddrManImpl to use a new exception class that wraps around std::ios_base::failure and only throw exception in the case of version mismatch… but I wanted to get feedback first.
Fixed.
Also, 2 jobs on the first commit’s CI have been running for 10 hours… is this caused by my strncmp?
LogPrintf the e.what() right before we LogPrintf "Creating new peers.dat..."?
195@@ -196,6 +196,11 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
196 addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
197 LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
198 DumpPeerAddresses(args, *addrman);
199+ } catch (const InvalidAddrManVersionError&) {
200+ // Addrman can be in an inconsistent state after failure, reset it
201+ addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
202+ LogPrintf("Creating new peers.dat because the file version was not compatible (%s)\n", fs::quoted(fs::PathToString(path_addr)));
203+ DumpPeerAddresses(args, *addrman);
Would be nice to deduplicate the common code here somehow.
Maybe just a single common catch of std::exception, with if(DbNotFoundError) else if (InvalidAddrManVersionError) else return then the common re-init logic?
195@@ -196,6 +196,11 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
196 addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
197 LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
198 DumpPeerAddresses(args, *addrman);
199+ } catch (const InvalidAddrManVersionError&) {
200+ // Addrman can be in an inconsistent state after failure, reset it
201+ addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
202+ LogPrintf("Creating new peers.dat because the file version was not compatible (%s)\n", fs::quoted(fs::PathToString(path_addr)));
I just saw your issue on knots.
I will make a backup at peers.dat.bak.
196@@ -197,9 +197,12 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
197 LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
198 DumpPeerAddresses(args, *addrman);
199 } catch (const InvalidAddrManVersionError&) {
200+ if (!RenameOver(path_addr, (fs::path)path_addr + ".bak")) {
201+ throw std::exception();
0throw std::runtime_error{"Failed to rename invalid peer.dat file. Please move or delete it and try again."};
67@@ -68,17 +68,16 @@ def run_test(self):
68 self.start_node(0, extra_args=["-checkaddrman=1"])
69 assert_equal(self.nodes[0].getnodeaddresses(), [])
70
71- self.log.info("Check that addrman from future cannot be read")
72+ self.log.info("Check that addrman from future is overwritten with new addrman")
73 self.stop_node(0)
74 write_addrman(peers_dat, lowest_compatible=111)
0write_addrman(peers_dat, lowest_compatible=111)
Is it possible to do this using some hidden RPC?
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.
195@@ -196,6 +196,15 @@ std::optional<bilingual_str> LoadAddrman(const std::vector<bool>& asmap, const A
196 addrman = std::make_unique<AddrMan>(asmap, /* deterministic */ false, /* consistency_check_ratio */ check_addrman);
197 LogPrintf("Creating peers.dat because the file was not found (%s)\n", fs::quoted(fs::PathToString(path_addr)));
198 DumpPeerAddresses(args, *addrman);
199+ } catch (const InvalidAddrManVersionError&) {
200+ if (!RenameOver(path_addr, (fs::path)path_addr + ".bak")) {
201+ addrman = nullptr;
202+ return strprintf(_("Failed to rename invalid peer.dat file. Please move or delete it and try again."));
s.dat
Sorry, I misunderstood. I made it WIP because I thought I still hadn’t finished all the outstanding issues with the PR.
After reviewing my own commit I understand I was just confused.
There’s a spelling error I’ll fix right now.
tACK https://github.com/bitcoin/bitcoin/pull/24201/commits/772c9ba8ccfa7247042be10771cca3c8de7e8568
nit:
peer to peers
Unrelated test failed for some reason.
I don’t think that’s an issue
Maybe you can follow 1 or 2 or both:
01st commit: net (src/addrman.h , src/addrman.cpp and src/addrdb.cpp)
02nd commit: test (test/functional/feature_addrman.py)
fixes #24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.