Rewrite addrdb with less duplication using CHashVerifier

sipa commented at 12:00 pm on April 21, 2017: member

This rewrites addrdb.cpp to use CHashVerifier (from #10195), and combines the code for addrdb and bandb to the extent possible.

laanwj added the label Utils and libraries on Apr 21, 2017

in src/validation.cpp:1252 in d76b7ac079 outdated

1454@@ -1455,19 +1455,18 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uin
1455 
1456     // Read block
1457     uint256 hashChecksum;
1458+    CHashVerifier<CAutoFile> verifier(&filein); // We need a CHashVerifier as reserializing may lose data

TheBlueMatt commented at 9:04 pm on April 21, 2017:

I believe this comment is wrong if this is pulled out of #10195.

sipa commented at 3:21 am on May 3, 2017:

Indeed; fixed.

TheBlueMatt commented at 9:34 pm on April 21, 2017: member

utACK d76b7ac079e140948d589f5c53a4af15ddcad6f5

sipa force-pushed on May 3, 2017

in src/hash.h:184 in d016b2c79c outdated

179+    void ignore(size_t nSize)
180+    {
181+        char data[1024];
182+        while (nSize > 0) {
183+            size_t now = std::min<size_t>(nSize, 1024);
184+            source->read(data, now);

ryanofsky commented at 9:36 pm on May 3, 2017:

In commit “Introduce CHashVerifier to hash read data”

Could replace these two lines with just read(data, now);

sipa commented at 9:15 pm on May 17, 2017:

Done.

in src/addrdb.cpp:140 in 0e8d567209 outdated

216-        // de-serialize address data into one CAddrMan object
217-        ssPeers >> addr;
218-    }
219-    catch (const std::exception& e) {
220-        // de-serialization has failed, ensure addrman is left in a clean state
221+    bool ret = DeserializeDB(ssPeers, addr, false);

ryanofsky commented at 9:51 pm on May 3, 2017:

In commit “Deduplicate addrdb.cpp and use CHashWriter/Verifier”

Not your change, but it isn’t clear to me why this why this function is a part of the CAddrDB class and is non-static. It doesn’t seem to access any state or be directly related to the class.

sipa commented at 9:13 pm on May 17, 2017:

I’ve made it static.

in src/addrdb.cpp:168 in 0e8d567209 outdated

170-    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);
171-    if (filein.IsNull())
172-        return error("%s: Failed to open file %s", __func__, pathAddr.string());
173-
174-    // use file size to size memory buffer
175-    uint64_t fileSize = fs::file_size(pathAddr);

ryanofsky commented at 10:02 pm on May 3, 2017:

In commit “Deduplicate addrdb.cpp and use CHashWriter/Verifier”

It seems this logic to read the file into a memory buffer before deserializing it has gone away. Any idea why it was written that way in the first place?

sipa commented at 9:05 pm on May 17, 2017:

I believe it’s to protect against the case where the data is unusually expensive to deserialize. I’m not particularly worried about (non-adverserial) corruption that causes expensive deserialization without causing an error somewhere along the way.

laanwj commented at 10:24 am on June 6, 2017:

IIRC the point was to be able to check the checksum before starting any deserialization work

sipa commented at 7:53 pm on June 7, 2017:

Indeed. I don’t think that’s particularly interesting, but if that remains a requirement, I’ll drop this PR.

ryanofsky commented at 10:03 pm on May 3, 2017: member

utACK 0e8d567209ba198f64c7657acb0e05018bf0c5be

in src/hash.h:174 in d016b2c79c outdated

169+
170+public:
171+    CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}
172+
173+    void read(char* pch, size_t nSize)
174+    {

theuni commented at 8:11 pm on May 8, 2017:

please check for nSize == 0 to avoid problems like #10250

sipa commented at 8:31 pm on May 17, 2017:

It seems that the invoked read and write method already need to deal with nSize==0 anyway?

in src/hash.h:171 in d016b2c79c outdated

166+{
167+private:
168+    Source* source;
169+
170+public:
171+    CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}

theuni commented at 8:12 pm on May 8, 2017:

Pass a reference instead, so that it can’t be null?

sipa commented at 8:30 pm on May 17, 2017:

I don’t like using references for objects that persist. This risks calling CHashVerifier(function_that_returns_a_Source()).

theuni commented at 8:21 pm on May 8, 2017: member

utACK, other than the nits

sipa force-pushed on May 17, 2017

sipa commented at 9:13 pm on May 17, 2017: member

I think I’ve addressed or responded to all comments.

Deduplicate addrdb.cpp and use CHashWriter/Verifier cf68a488a4

sipa force-pushed on Jun 1, 2017

sipa commented at 11:47 pm on June 1, 2017: member

The first commits of this PR are merged as part of #10195. I’m going to change the title to just reflect the effect of the last remaining commit.

sipa renamed this:
~~Introduce CHashVerifier to hash while deserializing~~
Rewrite addrdb with less duplication using CHashVerifier
on Jun 1, 2017

in src/addrdb.cpp:25 in cf68a488a4

22+bool SerializeDB(Stream& stream, const Data& data)
23 {
24-    pathBanlist = GetDataDir() / "banlist.dat";
25+    // Write and commit header, data
26+    try {
27+        CHashWriter hasher(SER_DISK, CLIENT_VERSION);

TheBlueMatt commented at 5:37 pm on June 7, 2017:

Seems it may be nicer to add a more analygous hasher to CHashVerifyer here - why not hash while writing the data instead of serializing twice?

TheBlueMatt commented at 5:50 pm on June 7, 2017: member

Feel free to leave comment for future PR.

utACK cf68a488a4cc78d711ca6e5e3236c6d89d689079

laanwj commented at 5:54 pm on June 22, 2017: member

utACK cf68a48

laanwj merged this on Jun 22, 2017

laanwj closed this on Jun 22, 2017

laanwj referenced this in commit 01c4b143a8 on Jun 22, 2017

PastaPastaPasta referenced this in commit 7261b741f6 on Jul 6, 2019

PastaPastaPasta referenced this in commit 97515afe31 on Jul 8, 2019

PastaPastaPasta referenced this in commit 620e15f873 on Jul 9, 2019

PastaPastaPasta referenced this in commit 305fd06796 on Jul 11, 2019

barrystyle referenced this in commit 296e58d0d9 on Jan 22, 2020

random-zebra referenced this in commit 116bb50765 on Apr 20, 2021

DrahtBot locked this on Sep 8, 2021

Rewrite addrdb with less duplication using CHashVerifier #10248