Replace BDB-managed addr.dat with bitcoin-managed peers.dat #1198

jgarzik commented at 7:38 AM on May 5, 2012: contributor

CAddrMan is the new address storage manager, and it already knows how to read/write its own serialized data.

Therefore, we may treat it like the block file and implement WriteToDisk() and ReadFromDisk() similarly. This eliminates the need for the BDB-managed addr.dat, replacing it with "peers.dat" containing nothing but a flat file storing addrman data (and a header and checksum).

laanwj commented at 8:03 AM on May 5, 2012: member

Nice.

Suggestion: when an error occurs while reading the address file, log it but don't escalate it. A corrupted file can easily be rebuilt, after all (and would remove one source of fatal startup errors).

sipa commented at 11:36 AM on May 5, 2012: member

I'd prefer addrman.{cpp,h} to be very self-contained and only have knowledge about the data structure and serialization, not how it's stored and certainly not where. The reason is that this may be useful to other projects (e.g. dns seeds) that don't have the same uti.h (addrman only depends on it because of the locking primitives).

Also, the IP address table is not critical, so incomplete writes in case of a crash during dumping may not be much of an issue, but it will certainly occur. I think we either want to defend against it (by making a failed read/deserialization not fatal) or by avoiding it (first writing to ipaddr.dat.new, and then atomically move it to ipaddr.dat). Or maybe both...

Edit: don't get me wrong, I definitely want this!

jgarzik commented at 8:45 PM on May 5, 2012: contributor

Yes, error diagnostic -- particularly for a corrupted or truncated file -- should be expanded, made more verbose.
Yes, the code should do something like write to temp file, then rename. I have said as much on IRC several times. Windows apparently uses ReplaceFile(), while Unix uses rename().
I/O implemented inside CAddrMan was intentionally remaining consistent with CBlock::Read/Write*Disk() etc. If we wanted to change that, that's OK. Just noting the logic behind the decision.

sipa commented at 10:23 PM on May 5, 2012: member

Or boost::filesystem::rename() ?
Ok, that's just a minor design issue; it can always be changed later on in several places to do it in a consistent way.

jgarzik commented at 12:07 AM on May 6, 2012: contributor

Googling for "boost filesystem rename atomic" seems to indicate that overwrite-via-rename is explicitly forbidden in the boost code. See e.g., http://lists.boost.org/boost-users/2008/01/33451.php

Would be happy to use boost if possible, but I think we will be forced to #ifdef WINDOWS { ReplaceFile() }

sipa commented at 12:27 AM on May 6, 2012: member

The first answer here is helpful: http://stackoverflow.com/questions/3156841/boostfilesystemrename-cannot-create-a-file-when-that-file-already-exists

Seems that in boost::filesystem v3 the behaviour is correct. The problem is that we still support v2...

luke-jr commented at 6:22 PM on May 6, 2012: member

First run with this pullreq included, I get "Error loading ipaddr.dat" and quit. How can I start it for the first time? :)

Edit: Confirmed ipaddr.dat doesn't exist yet.

laanwj commented at 6:34 PM on May 6, 2012: member

Now, that's what I meant with not escalating the error...

luke-jr commented at 7:15 PM on May 6, 2012: member

Should ipaddr.dat (451 KB) be much larger than addr.dat (7.3 KB)?

jgarzik commented at 5:47 AM on May 7, 2012: contributor

Added fix: do not die, if ipaddr.dat is missing

jgarzik commented at 5:21 AM on May 12, 2012: contributor

OK, ipaddr.dat is now renamed into place. All concerns have been addressed in the latest rebase.

Additional comments?

sipa commented at 4:30 PM on May 12, 2012: member

operator<< can fail and throw an exception if the file is not in the correct format (or there's a bug). I think you'll need a try catch in/around addrman.ReadFromDisk.

jgarzik commented at 5:10 AM on May 13, 2012: contributor

Added a new commit, re-creating CAddrDB.

in src/db.cpp:None in 500b644f3b outdated

 760 | +    CAutoFile fileout = CAutoFile(file, SER_DISK, CLIENT_VERSION);
 761 | +    if (!fileout)
 762 | +        return error("CAddrman::Write() : CAutoFile failed");
 763 | +
 764 | +    // Write and commit addrman data
 765 | +    try {

sipa commented at 1:36 PM on May 13, 2012:

Without catch block, won't the exception just get thrown further?

jgarzik commented at 6:51 PM on May 13, 2012:

I don't understand your question. The 'catch' statement immediately follows 'try'; scroll down a bit in the diff.

sipa commented at 6:55 PM on May 13, 2012:

Apologies, I misread the diff. Ignore my comment.

sipa commented at 3:25 PM on May 16, 2012: member

ACK

jgarzik commented at 7:47 PM on May 16, 2012: contributor

Added commits:

rename to "peers.dat", the consensus on IRC
add file header, which includes a magic number (pchMessageStart) and sha256 checksum

Add new utility functions FileCommit(), RenameOver() 768e5d52fb

jgarzik commented at 11:30 PM on May 16, 2012: contributor

Rebased, and fixed one final nasty bug.

CAddrDB: Replace BDB-managed addr.dat with internally managed peers.dat 928d3a011c

jgarzik commented at 2:13 AM on May 17, 2012: contributor

Collapsed multiple commits into two. A couple minor cleanups.

jgarzik referenced this in commit b56843b253 on May 17, 2012

jgarzik merged this on May 17, 2012

jgarzik closed this on May 17, 2012

coblee referenced this in commit 732452a00d on Jul 17, 2012

jgarzik deleted the branch on Aug 24, 2014

suprnurd referenced this in commit df5abf1468 on Dec 5, 2017

lateminer referenced this in commit 060e0e2556 on Jan 22, 2019

dexX7 referenced this in commit fc172d8552 on Nov 30, 2020

DrahtBot locked this on Sep 8, 2021