[DB] Bitcoin 0.12-0.14 dbwrapper improvements #1629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
|
Concept ACK 👌 , nice add. |
…nstants for database keys backports bitcoin/bitcoin@14d023f
coming from bitcoin/bitcoin@42cb388 extracting the refactor and omitting the chainstate obfuscation
based on bitcoin/bitcoin@3499ce1 and bitcoin/bitcoin@0fdf8c8 excluding the obfuscation, and fixing styling. Additional edits for CZerocoinDB
Thanks @dexX7. Backports bitcoin/bitcoin@1488506 (gettxoutsetinfo functional test is already ported)
backports bitcoin/bitcoin@8587b23 [additional edits for zerocoin and sporks databases]
random-zebra
force-pushed
the
202005_backport_dbwrapper
branch
from
2eee76f
to
d420ebe
Compare
1 task
furszy
reviewed
May 22, 2020
Fuzzbawls
reviewed
May 23, 2020
|
Added 2 commits to also backport bitcoin#6290 and bitcoin#9281 following the comments. |
furszy
reviewed
May 23, 2020
furszy
previously approved these changes
May 24, 2020
random-zebra
added this to In Progress
in perpetual updating PIVX Core to BTC Core
via automation
HandleError is implementation-specific. backports bitcoin/bitcoin@869cf12
backports (partially) bitcoin/bitcoin@73f4119
random-zebra
force-pushed
the
202005_backport_dbwrapper
branch
from
1d8cbfe
to
82f9088
Compare
|
Dropped the commit |
furszy
approved these changes
May 26, 2020
perpetual updating PIVX Core to BTC Core
automation
moved this from In Progress
to Ready
May 27, 2020
Fuzzbawls
approved these changes
May 27, 2020
furszy
added a commit
that referenced
this pull request
May 29, 2020
249cc9d Avoid -Wshadow errors (random-zebra) 8e1ec9e Use fixed preallocation instead of costly GetSerializeSize (random-zebra) 9b801d0 Add optimized CSizeComputer serializers (random-zebra) 0035a54 Make CSerAction's ForRead() constexpr (random-zebra) 9730a3f Get rid of nType and nVersion (random-zebra) 25ce2bb Make GetSerializeSize a wrapper on top of CSizeComputer (random-zebra) 1b479db Make nType and nVersion private and sometimes const (random-zebra) 35f1755 Make streams' read and write return void (random-zebra) a395914 Remove unused ReadVersion and WriteVersion (random-zebra) 52e614c [WIP] Remove unused statement in serialization (random-zebra) 82a2021 Add COMPACTSIZE wrapper similar to VARINT for serialization (random-zebra) 13ad779 add bip32 pubkey serialization (random-zebra) 9e9b7b5 [QA] Update json files with sig hash type in ASM for bitcoin-util-test (random-zebra) 3383983 Resolve issue bitcoin#3166 (random-zebra) Pull request description: -Based on top of - [x] #1629 Backports the following serialization improvements from upstream and adds the required changes for the 2nd layer network and the legacy zerocoin code. - bitcoin#5264 > show scriptSig signature hash types. fixes bitcoin#3166 > > The fix basically appends the scriptSig signature hash types, within parentheses, onto the end of the signature(s) in the various "asm" json outputs. That's just the first formatting idea that came to my mind. > > Added some tests for this too. - bitcoin#6215 > CExtPubKey should be serializable like CPubKey. > This would allow storing extended private and public key to support BIP32/HD wallets. - bitcoin#8068 (only commit 5249dac) This adds COMPACTSIZE wrapper similar to VARINT for serialization - bitcoin#8658 > As the line > ``` > nVersion = this->nVersion; > ``` > seems to have no meaning in READ and also in WRITE serialization op, let's remove it and see what our tests/travis will tell us. See bitcoin#8468 for previous discussion. - bitcoin#9039 > The commits in this pull request implement a sequence of changes: > > - Simplifications: > - **Remove unused ReadVersion and WriteVersion** CDataStream and CAutoFile had a ReadVersion and WriteVersion method that was never used. Remove them. > - **Make nType and nVersion private and sometimes const** Make the various stream implementations' nType and nVersion private and const (except in CDataStream where we really need a setter). > - **Make streams' read and write return void** The stream implementations have two layers (the upper one with operator<< and operator>>, and a lower one with read and write). The lower layer's return values are never used (nor should they, as they should only be used from the higher layer), so make them void. > - **Make GetSerializeSize a wrapper on top of CSizeComputer** Given that in default GetSerializeSize implementations we're already using CSizeComputer(), get rid of the specialized GetSerializeSize methods everywhere, and just use CSizeComputer. This removes a lot of code which isn't actually used anywhere. In a few places, this removes an actually more efficient size computing algorithm, which we'll bring back in the "Add optimized CSizeComputer serializers" commit later. > - **Get rid of nType and nVersion** The big change: remove the nType and nVersion as parameters to all serialization methods and functions. There is only one place where it's read and has an impact (in CAddress), and even there it does not impact any of the member objects' serializations. Instead, the few places that need nType or nVersion read it directly from the stream, through GetType and GetVersion calls which are added to all streams. > - **Avoid -Wshadow errors** As suggested by @paveljanik, remove the few remaining cases of variable shadowing in the serialization code. > - Optimizations: > - **Make CSerAction's ForRead() constexpr** The CSerAction's ForRead() method does not depend on any runtime data, so guarantee that requests to it can be optimized out by making it constexpr (suggested by @theuni in bitcoin#8580). > - **Add optimized CSizeComputer serializers** To get the advantages of faster GetSerializeSize implementations back, reintroduce them in the few places where they actually make a difference, in the form of a specialized Serialize implementation. This actually gets us in a better state than before, as these even get used when they're nested inside the serialization of another object. > - **Use fixed preallocation instead of costly GetSerializeSize** dbwrapper uses GetSerializeSize to compute the size of the buffer to preallocate. For some cases (specifically: CCoins) this requires a costly compression call. Avoid this by just using fixed size preallocations instead. > > This will make it easier to address @TheBlueMatt's comments in bitcoin#8580, resulting is a simpler and more efficient way to simultaneously deserialize+construct objects with const members from streams. ACKs for top commit: furszy: Long and nice PR 👌 , code review ACK 249cc9d . Fuzzbawls: ACK 249cc9d furszy: tested ACK 249cc9d and merging. Tree-SHA512: 56b07634b1e18871e7c9a99d412282c83b85f77f1672ec56330a1131fc7c234cd1ba3a053bdd210cc29f1e636ee374477ff614fa9a930329a7f8f912c5006232
random-zebra
added a commit
that referenced
this pull request
Jun 2, 2020
286aa99 Remove `namespace fs=fs` (random-zebra) f16a6db torcontrol: Use fs::path instead of std::string for private key path (random-zebra) 7de1ba7 Use fsbridge for fopen and freopen (random-zebra) 5010dac Replace uses of boost::filesystem with fs (random-zebra) 4305cce Replace includes of boost/filesystem.h with fs.h (random-zebra) 1d84a5a Add fs.cpp/h (random-zebra) Pull request description: Based on top of: - [x] #1629 Backports bitcoin#9902 > Wrap boost::filesystem as the fs namespace, contained in one header. > Also add fsbridge for bridging between stdio.h fopen() and the path type. > > This allows: > > Replacing it when the time comes with std::filesystem (when standardized, and when we start using the appropriate c++ version) with a single change. > > Using custom filesystem implementations that offer the same interface. I need this in cloudabi, a sandboxed environment which has no concept of a global filesystem namespace, where paths need to be accompanied by a file descriptor which grants access to it. > > It's less typing, too. > > See individual commits for more information. ACKs for top commit: furszy: Code review ACK 286aa99. furszy: ACK 286aa99. Fuzzbawls: ACK 286aa99 Tree-SHA512: d5409bc67bb1fbb38fd65182ca36f122d81db7ec8a17747c0683602cb71e407b0e09779ca15171902fc0d2906302a993606ff6599381ef9ad3e52dbac1194952
This was referenced Aug 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This backports a series of updates and cleanups to the LevelDB wrapper from:
*]*]*]gettxoutsettinforun lock-free bitcoin/bitcoin#6290PIVX-specific edits were required to keep the sporks and zerocoin databases in line.
[
*] NOTE: excluding the obfuscation of databases by xoring data, as we might not want this feature (e.g. as zcash/zcash#2598). Otherwise it can be discussed, and added, with a separate PR.