Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] [kernelheaders 1/n] Cleave LevelDB headers from our header tree #24676

Closed

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Mar 25, 2022

This PR is not based on anything other than master.

We #include LevelDB headers in dbwrapper.h, which means that anything that #include's or transitively #include's dbwrapper.h will also pull in the LevelDB headers:

bitcoin/src/dbwrapper.h

Lines 16 to 17 in 2f0f056

#include <leveldb/db.h>
#include <leveldb/write_batch.h>

This PR's changes allow us to remove these LevelDB header #include's in dbwrapper.h and have them be in dbwrapper.cpp instead. We achieve this mainly by:

  • Using private non-virtual interfaces (private doFoo's) to:
    • Move LevelDB-specific code out of the header,
    • While keeping templatized serialization logic in the header
  • Using the pimpl idiom to move LevelDB-specific member variables out of the header

Coincidentally, this PR also makes obfuscation an implementation detail of the database, as described in this code comment:

bitcoin/src/dbwrapper.h

Lines 39 to 42 in 2f0f056

* Database obfuscation should be considered an implementation detail of the
* specific database.
*/
const std::vector<unsigned char>& GetObfuscateKey(const CDBWrapper &w);

@dongcarl dongcarl force-pushed the 2022-03-kernelheaders-dbwrapper branch from cf1701a to 47ccacd Compare March 25, 2022 21:38
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 27, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25108 (tidy: add modernize-use-default-member-init by fanquake)
  • #24922 (Isolate the storage abstraction layer from the application/serialization layer by TheQuantumPhysicist)
  • #24232 (assumeutxo: add init and completion logic by jamesob)
  • #22663 (dbwrapper: properly destroy objects in case CDBWrapper ctor throws by Crypt-iQ)

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.

@laanwj
Copy link
Member

laanwj commented Apr 6, 2022

Concept ACK.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@dongcarl
Copy link
Contributor Author

dongcarl commented Sep 26, 2022

Closing, @TheCharlatan will follow up with a PR that's simpler

@dongcarl dongcarl closed this Sep 26, 2022
Copy link

@8498549767 8498549767 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excente

@bitcoin bitcoin locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done or Closed or Rethinking
Development

Successfully merging this pull request may close these issues.

4 participants