Raise InitError when peers.dat is invalid or corrupted #22762

maflcko · 2021-08-21T12:41:20Z

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 user provided the database, but picked the wrong one.
A future version of Bitcoin Core wrote the file and it can't be read.
The file was corrupted by a logic bug in Bitcoin Core.
The file was corrupted by a disk failure.

DrahtBot · 2021-08-21T21:56:37Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22937 (refactor: Add fs::PathToString, fs::PathFromString, u8string, u8path functions by ryanofsky)
#22831 (test, bugfix: addrman tried table corruption on restart with asmap by jonatack)

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.

src/addrdb.cpp

ghost · 2021-08-24T17:18:12Z

Concept ACK

maflcko

force pushed to address review comments

src/addrdb.cpp

vasild

ACK fa1704a

src/addrdb.cpp

vasild

ACK faff4fd

jonatack · 2021-09-03T10:40:17Z

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.

vasild

ACK fa7003f

jonatack

ACK fa7003f 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)

src/addrdb.h

src/init.cpp

ghost · 2021-09-07T19:05:49Z

Tested on Pop!_OS and everything works as expected and mentioned in PR description.

Master Branch:

peers.dat is replaced with a new file if it is corrupt or other issues.

Details

PR Branch:

A. Corrupt peers.dat file

Open peers.dat add random characters in beginning and save.
Run bitcoind

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.
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...

⚠️ 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

Delete regtest/peers.dat and copy testnet3/peers.dat in regtest/
Run bitcoind

2021-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. ✅

fade9a1 Remove confusing CAddrDB (MarcoFalke) fa7f77b Fix addrdb includes (MarcoFalke) fa3f5d0 Move addrman includes from .h to .cpp (MarcoFalke) Pull request description: Split out from bitcoin#22762 to avoid having to carry it around in (an)other rebase(s) ACKs for top commit: ryanofsky: Code review ACK fade9a1 lsilva01: Code Review ACK bitcoin@fade9a1 Tree-SHA512: 7615fb0b6235d0c1e6f8cd6263dd18c4d95890567c2b797fe6fce6cb12cc85ce6eacbe07dbb6d81b05d179ef03b42edfd61c940e35a1044ce6d363b54c2dae5c

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

maflcko · 2021-09-09T07:25:07Z

Rebased (only change is in tests)

jonatack · 2021-09-09T08:44:33Z

Code review re-ACK fa55c3d per git range-diff eb1f570 fa59c6d fa55c3 and verified the new tests fail on master, except "Check mocked addrman is valid", as expected

vasild

ACK fa55c3d

test/functional/feature_addrman.py

ghost · 2021-09-09T12:44:00Z

tACK fa55c3d

nit: Will be better if error message was printed only once #22762 (comment)

maflcko · 2021-09-09T12:50:19Z

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.

maflcko · 2021-09-10T11:06:50Z

src/addrdb.cpp

+        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


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.

…rupted

sipa · 2021-10-25T15:50:41Z

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.

…ers.dat d41ed32 p2p: Avoid InitError when downgrading peers.dat (junderw) Pull request description: fixes #24188 (also see bitcoin/bitcoin#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. ACKs for top commit: prayank23: reACK bitcoin/bitcoin@d41ed32 kallewoof: reACK d41ed32 Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40

d41ed32 p2p: Avoid InitError when downgrading peers.dat (junderw) Pull request description: fixes bitcoin#24188 (also see bitcoin#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. ACKs for top commit: prayank23: reACK bitcoin@d41ed32 kallewoof: reACK d41ed32 Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40

Summary: ``` 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 user provided the database, but picked the wrong one. - A future version of Bitcoin Core wrote the file and it can't be read. - The file was corrupted by a logic bug in Bitcoin Core. - The file was corrupted by a disk failure. ``` Backport of [[bitcoin/bitcoin#22762 | core#22762]]. Also cherry-picks a commit from PR22697 that is now required for the addrman_tests to pass, and for which the rationale is still trivially correct: bitcoin/bitcoin@ed9ba8a Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D12308

maflcko mentioned this pull request Aug 21, 2021

addrman: Avoid crash on corrupt data, Force Check after deserialize #22734

Merged

DrahtBot added GUI P2P labels Aug 21, 2021

maflcko removed the GUI label Aug 21, 2021

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch 2 times, most recently from fad1965 to fa50c25 Compare August 21, 2021 14:24

DrahtBot mentioned this pull request Aug 21, 2021

refactor: Clarify and disable unused ArgsManager flags #22766

Merged

DrahtBot mentioned this pull request Aug 22, 2021

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

Merged

vasild reviewed Aug 24, 2021

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 25, 2021

init: Fix asmap/addrman initialization order bug #22791

Merged

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch from fa50c25 to fa1704a Compare August 26, 2021 09:03

maflcko commented Aug 26, 2021

View reviewed changes

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 27, 2021

refactor: Implement missing error checking for ArgsManager flags #16545

Draft

vasild approved these changes Aug 27, 2021

View reviewed changes

vasild reviewed Aug 27, 2021

View reviewed changes

src/addrdb.cpp Show resolved Hide resolved

This was referenced Aug 30, 2021

test: add addpeeraddress "tried", test addrman checks on restart with asmap #22831

Merged

Remove unused SERIALIZE_METHODS for CBanEntry #22849

Merged

MOVEONLY: Expose BanMapToJson / BanMapFromJson #22848

Merged

DrahtBot added the Needs rebase label Sep 1, 2021

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch 2 times, most recently from fa6c460 to faff4fd Compare September 1, 2021 15:57

DrahtBot removed the Needs rebase label Sep 1, 2021

DrahtBot mentioned this pull request Sep 1, 2021

Multiprocess bitcoin #10102

Draft

vasild approved these changes Sep 2, 2021

View reviewed changes

maflcko mentioned this pull request Sep 3, 2021

test: Add addrman peers.dat test #22877

Closed

DrahtBot added the Needs rebase label Sep 6, 2021

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch from faff4fd to b4cc1c5 Compare September 6, 2021 10:20

vasild approved these changes Sep 7, 2021

View reviewed changes

maflcko closed this Sep 7, 2021

maflcko reopened this Sep 7, 2021

jonatack reviewed Sep 7, 2021

View reviewed changes

src/addrdb.h Show resolved Hide resolved

src/init.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 7, 2021

net: Encapsulate asmap in NetGroupManager #22910

Merged

maflcko mentioned this pull request Sep 8, 2021

Remove confusing CAddrDB #22915

Merged

MarcoFalke added 2 commits September 9, 2021 09:11

Move LoadAddrman from init to addrdb

fa5aeec

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.

Inline ReadPeerAddresses

fa4e2cc

No need to have a function that is only called in one place

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch from fa7003f to fa59c6d Compare September 9, 2021 07:13

Raise InitError when peers.dat is invalid or corrupted

fa55c3d

maflcko force-pushed the 2108-InitErrorAddrmanCorrupt branch from fa59c6d to fa55c3d Compare September 9, 2021 07:21

vasild approved these changes Sep 9, 2021

View reviewed changes

test/functional/feature_addrman.py Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 10, 2021

refactor: Forbid calling unsafe fs::path(std::string) constructor and fs::path::string() method #22937

Merged

maflcko merged commit 053a5fc into bitcoin:master Sep 10, 2021

maflcko deleted the 2108-InitErrorAddrmanCorrupt branch September 10, 2021 09:52

maflcko commented Sep 10, 2021

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 11, 2021

Merge bitcoin#22762: Raise InitError when peers.dat is invalid or cor…

78b1e98

…rupted

maflcko mentioned this pull request Jan 28, 2022

Avoid InitError when downgrading peers.dat #24188

Closed

junderw mentioned this pull request Jan 30, 2022

p2p: Avoid InitError when downgrading peers.dat #24201

Merged

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Raise InitError when peers.dat is invalid or corrupted #22762

Raise InitError when peers.dat is invalid or corrupted #22762

Uh oh!

Conversation

maflcko commented Aug 21, 2021

Uh oh!

DrahtBot commented Aug 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 24, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 3, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Sep 7, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Master Branch:

PR Branch:

Uh oh!

maflcko commented Sep 9, 2021

Uh oh!

jonatack commented Sep 9, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Sep 9, 2021

Uh oh!

maflcko commented Sep 9, 2021

Uh oh!

maflcko Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 25, 2021

Uh oh!

Uh oh!

DrahtBot commented Aug 21, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading

ghost commented Sep 7, 2021 •

edited by ghost

Loading