Rewrite addrdb with less duplication using CHashVerifier #10248

sipa · 2017-04-21T12:00:07Z

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

TheBlueMatt

utACK d76b7ac079e140948d589f5c53a4af15ddcad6f5

TheBlueMatt · 2017-04-21T21:04:35Z

src/validation.cpp

@@ -1455,19 +1455,18 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CDiskBlockPos& pos, const uin

    // Read block
    uint256 hashChecksum;
+    CHashVerifier<CAutoFile> verifier(&filein); // We need a CHashVerifier as reserializing may lose data


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

Indeed; fixed.

ryanofsky

utACK 0e8d567209ba198f64c7657acb0e05018bf0c5be

ryanofsky · 2017-05-03T21:36:23Z

src/hash.h

+        char data[1024];
+        while (nSize > 0) {
+            size_t now = std::min<size_t>(nSize, 1024);
+            source->read(data, now);


In commit "Introduce CHashVerifier to hash read data"

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

ryanofsky · 2017-05-03T21:51:02Z

src/addrdb.cpp

-    }
-    catch (const std::exception& e) {
-        // de-serialization has failed, ensure addrman is left in a clean state
+    bool ret = DeserializeDB(ssPeers, addr, false);


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.

I've made it static.

ryanofsky · 2017-05-03T22:02:11Z

src/addrdb.cpp

-        return error("%s: Failed to open file %s", __func__, pathAddr.string());
-
-    // use file size to size memory buffer
-    uint64_t fileSize = fs::file_size(pathAddr);


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?

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.

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

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

theuni · 2017-05-08T20:11:04Z

src/hash.h

+    CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}
+
+    void read(char* pch, size_t nSize)
+    {


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

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

theuni · 2017-05-08T20:12:02Z

src/hash.h

+    Source* source;
+
+public:
+    CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}


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

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

theuni · 2017-05-08T20:21:02Z

utACK, other than the nits

sipa · 2017-05-17T21:13:52Z

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

sipa · 2017-06-01T23:47:16Z

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.

TheBlueMatt

Feel free to leave comment for future PR.

utACK cf68a48

TheBlueMatt · 2017-06-07T17:37:00Z

src/addrdb.cpp

-    pathBanlist = GetDataDir() / "banlist.dat";
+    // Write and commit header, data
+    try {
+        CHashWriter hasher(SER_DISK, CLIENT_VERSION);


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?

laanwj · 2017-06-22T17:54:39Z

utACK cf68a48

cf68a48 Deduplicate addrdb.cpp and use CHashWriter/Verifier (Pieter Wuille) Tree-SHA512: 0301332e797f64da3a1588c9ebaf533af58da41e38f8a64206bff20102c5e82c2a7c630ca3150cf451b2ccf4acb3dd45e44259b6ba15e92786e9e9a2b225bd2f

…Verifier cf68a48 Deduplicate addrdb.cpp and use CHashWriter/Verifier (Pieter Wuille) Tree-SHA512: 0301332e797f64da3a1588c9ebaf533af58da41e38f8a64206bff20102c5e82c2a7c630ca3150cf451b2ccf4acb3dd45e44259b6ba15e92786e9e9a2b225bd2f

…+ Add file syncing logging and error handling d593e7e Migrate remaining FLATDATA serialization, we are natively supporting it now. (furszy) 051970d addrdb: Remove temporary files created in SerializeFileDB. Fixes non-determinism in unit tests. (practicalswift) 4be426f Add logging and error handling for file syncing (Wladimir J. van der Laan) 8662fb3 Remove unused double_safe_addition & double_safe_multiplication functions. (furszy) a7c3885 Add native support for serializing char arrays without FLATDATA (Pieter Wuille) 459ecb9 Deduplicate addrdb.cpp and use CHashWriter/Verifier (Pieter Wuille) a5d2f8a Fix copypasted comment. (Pavel Janík) Pull request description: Digging down the peers and ban databases commits path in upstream found some good stuff, back ported the following PRs: * bitcoin#9216. * bitcoin#10248. * bitcoin#12740. * bitcoin#13039. * bitcoin#16212. ACKs for top commit: random-zebra: ACK d593e7e Fuzzbawls: ACK d593e7e Tree-SHA512: 8d567c68a2c36f43c749d5faa4b7f8a299dbfdda133495df434ac642c1a2ac96dc2259123ad85decc9039c62c46602665f6be4aadf6d5bf729344c51692ec60e

sipa mentioned this pull request Apr 21, 2017

Switch chainstate db and cache to per-txout model #10195

Merged

laanwj added the Utils/log/libs label Apr 21, 2017

TheBlueMatt reviewed Apr 21, 2017

View reviewed changes

sipa force-pushed the chashverifier branch from d76b7ac to 0e8d567 Compare May 3, 2017 03:20

ryanofsky reviewed May 3, 2017

View reviewed changes

theuni reviewed May 8, 2017

View reviewed changes

sipa force-pushed the chashverifier branch from 0e8d567 to 60f464c Compare May 17, 2017 21:13

Deduplicate addrdb.cpp and use CHashWriter/Verifier

cf68a48

sipa force-pushed the chashverifier branch from 60f464c to cf68a48 Compare June 1, 2017 23:44

sipa changed the title ~~Introduce CHashVerifier to hash while deserializing~~ Rewrite addrdb with less duplication using CHashVerifier Jun 1, 2017

TheBlueMatt reviewed Jun 7, 2017

View reviewed changes

laanwj merged commit cf68a48 into bitcoin:master Jun 22, 2017

furszy mentioned this pull request Apr 9, 2021

Addrdb code deduplication + Native support for FLATDATA + Add file syncing logging and error handling PIVX-Project/PIVX#2304

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite addrdb with less duplication using CHashVerifier #10248

Rewrite addrdb with less duplication using CHashVerifier #10248

sipa commented Apr 21, 2017 •

edited

TheBlueMatt left a comment

TheBlueMatt Apr 21, 2017

sipa May 3, 2017

ryanofsky left a comment

ryanofsky May 3, 2017

sipa May 17, 2017

ryanofsky May 3, 2017

sipa May 17, 2017

ryanofsky May 3, 2017

sipa May 17, 2017

laanwj Jun 6, 2017

sipa Jun 7, 2017

theuni May 8, 2017

sipa May 17, 2017

theuni May 8, 2017

sipa May 17, 2017

theuni commented May 8, 2017

sipa commented May 17, 2017

sipa commented Jun 1, 2017

TheBlueMatt left a comment

TheBlueMatt Jun 7, 2017

laanwj commented Jun 22, 2017

Rewrite addrdb with less duplication using CHashVerifier #10248

Rewrite addrdb with less duplication using CHashVerifier #10248

Conversation

sipa commented Apr 21, 2017 • edited

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theuni commented May 8, 2017

sipa commented May 17, 2017

sipa commented Jun 1, 2017

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jun 22, 2017

sipa commented Apr 21, 2017 •

edited