Rewrite addrdb with less duplication using CHashVerifier by sipa · Pull Request #10248 · bitcoin/bitcoin

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

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

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

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

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

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

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

Conversation

sipa commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented May 8, 2017

Uh oh!

sipa commented May 17, 2017

Uh oh!

sipa commented Jun 1, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sipa commented Apr 21, 2017 •

edited

Loading