Age | Commit message (Collapse) | Author |
|
DisconnectedBlockTransactions to not use boost
4313c77400eb8eaa8586db39a7e29a861772ea80 make DisconnectedBlockTransactions responsible for its own memory management (glozow)
cf5f1faa037e9a40a5029cc7dd4ee61454b62466 MOVEONLY: DisconnectedBlockTransactions to its own file (glozow)
2765d6f3434c101fe2d46e9313e540aa680fbd77 rewrite DisconnectedBlockTransactions as a list + map (glozow)
79ce9f0aa46de8ff742be83fd6f68eab40e073ec add std::list to memusage (glozow)
59a35a7398f5bcb3e3805d1e4f363e4c2fb336b3 [bench] DisconnectedBlockTransactions (glozow)
925bb723ca71aa76380b769d8926c7c2ad9bbb7b [refactor] batch-add transactions to DisconnectedBlockTransactions (glozow)
Pull request description:
Motivation
- I think it's preferable to use stdlib data structures instead of depending on boost if we can achieve the same thing.
- Also see #28335 for further context/motivation. This PR simplifies that one.
Things done in this PR:
- Add a bench for `DisconnectedBlockTransactions` where we reorg and the new chain has {100%, 90%, 10%} of the same transactions. AFAIU in practice, it's usually close to 100%.
- Rewrite `DisconnectedBlockTransactions` as a `std::list` + `unordered_map` instead of a boost multi index container.
- On my machine, the bench suggests the performance is very similar.
- Move `DisconnectedBlockTransactions` from txmempool.h to its own kernel/disconnected_transactions.h. This struct isn't used by txmempool and doesn't have much to do with txmempool. My guess is that it's been living there for convenience since the boost includes are there.
ACKs for top commit:
ismaelsadeeq:
Tested ACK 4313c77400eb8eaa8586db39a7e29a861772ea80
stickies-v:
ACK 4313c77400eb8eaa8586db39a7e29a861772ea80
TheCharlatan:
ACK 4313c77400eb8eaa8586db39a7e29a861772ea80
Tree-SHA512: 273c80866bf3acd39b2a039dc082b7719d2d82e0940e1eb6c402f1c0992e997256722b85c7e310c9811238a770cfbdeb122ea4babbc23835d17128f214a1ef9e
|
|
This struct is only used in validation + tests and has very little to do
with txmempool.
|
|
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
|
|
This is needed for a future commit. Can be reviewed with:
--color-moved=dimmed-zebra
|
|
91d924ede1b421df31c895f4f43359e453a09ca5 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow)
bacdb2e208531124e85ed2d4ea2a4b508fbb5088 Clean up script/standard.{h/cpp} includes (Andrew Chow)
f3c9078b4cddec5581e52de5c216ae53984ec130 Clean up things that include script/standard.h (Andrew Chow)
8bbe257bac751859a272ddf52dc0328c1b5a1ede MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow)
7a172c76d2361fc3cdf6345590e26c79a7821672 Move CTxDestination to its own file (Andrew Chow)
145f36ec81e79d2e391847520364c2420ef0e0e8 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow)
86ea8bed5473f400f7a93fcc455393a574a2f319 Move CScriptID to script.{h/cpp} (Andrew Chow)
b81ebff0d99c45c071b999796b8ae3f0f2517b22 Remove ScriptHash from CScriptID constructor (Andrew Chow)
cba69dda3da0e4fa39cff5ce4dc81d1242fe651b Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h (Anthony Towns)
Pull request description:
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to `SigningProvider` as these are used only during signing.
* `CScriptID` is moved to `script/script.h` to be next to `CScript`.
* `MANDATORY_SCRIPT_VERIFY_FLAGS` is moved to `interpreter.h`
* The parameters `DEFAULT_ACCEPT_DATACARRIER` and `MAX_OP_RETURN_RELAY` are moved to `policy.h`
* `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above moves
ACKs for top commit:
Sjors:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
ajtowns:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
MarcoFalke:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇
murchandamus:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
darosior:
Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.
theStack:
Code-review ACK 91d924ede1b421df31c895f4f43359e453a09ca5
Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
|
|
Since script/standard only contains things that are used by the Solver
and its callers, rename the files to script/solver.
|
|
CTxDestination is really our internal representation of an address and
doesn't really have anything to do with standard script types, so move
them to their own file.
|
|
1c7582ead6e1119899922041c1af2b4169b0bc74 tests: add decryption test to bip324_tests (Pieter Wuille)
990f0f8da92a2d11828a7c05ca93bf0520b2a95e Add BIP324Cipher, encapsulating key agreement, derivation, and stream/AEAD ciphers (Pieter Wuille)
c91cedf281e5207fb5fd2ca81feec9760f7c2ed0 crypto: support split plaintext in ChaCha20Poly1305 Encrypt/Decrypt (Pieter Wuille)
af2b44c76e5de8ce880381e5535ead37ab0b3ba9 bench: add benchmark for FSChaCha20Poly1305 (Pieter Wuille)
aa8cee93342ee857931afec9af3ff5dbd8ce7749 crypto: add FSChaCha20Poly1305, rekeying wrapper around ChaCha20Poly1305 (Pieter Wuille)
0fee267792eb8cbdd48ad78f1712420b5d8d905b crypto: add FSChaCha20, a rekeying wrapper around ChaCha20 (Pieter Wuille)
9ff0768bdcca06836ccc673eacfa648e801930cb crypto: add the ChaCha20Poly1305 AEAD as specified in RFC8439 (Pieter Wuille)
9fd085a1a49d317abcaf1492b71c48bf1a1b3007 crypto: remove outdated variant of ChaCha20Poly1305 AEAD (Pieter Wuille)
Pull request description:
Depends on #27985 and #27993, based on and partially replaces #25361, part of #27634. Draft while dependencies are not merged.
This adds implementations of:
* The ChaCha20Poly1305 AEAD from [RFC8439 section 2.8](https://datatracker.ietf.org/doc/html/rfc8439#section-2.8), including test vectors.
* The FSChaCha20 stream cipher as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20.
* The FSChaCha20Poly1305 AEAD as specified in [BIP324](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#rekeying-wrappers-fschacha20poly1305-and-fschacha20), a rekeying wrapper around ChaCha20Poly1305.
* A BIP324Cipher class that encapsulates key agreement, key derivation, and stream ciphers and AEADs for [BIP324 packet encoding](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#overall-packet-encryption-and-decryption-pseudocode).
The ChaCha20Poly1305 and FSChaCha20Poly1305 implementations are new, taking advance of the improvements in #27993.
ACKs for top commit:
jamesob:
reACK 1c7582e
theStack:
ACK 1c7582ead6e1119899922041c1af2b4169b0bc74
stratospher:
tested ACK 1c7582e.
Tree-SHA512: 06728b4b95b21c5b732ed08faf40e94d0583f9d86ff4db3b92dd519dcd9fbfa0f310bc66ef1e59c9e49dd844ba8c5ac06e2001762a804fb5aa97027816045a46
|
|
d8f1222ac50f089a0af29eaf8ce0555bad8366ef refactor: Correct dbwrapper key naming (TheCharlatan)
be8f159ac59b9e700cbd3314ed71ebf39bd5b67a build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan)
c95b37d641b1eed4a62d55ca5342a6ed8c7a1ce7 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan)
c534a615e93452a5f509aaf5f68c600391a98d6a refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan)
586448888b72f7c87db4dcd30fc4e4044afae13b refactor: Move HandleError to dbwrapper implementation (TheCharlatan)
dede0eef7adb7413f62f5abd68cac8e01635ba4a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan)
a5c2eb57484314b04ec94523d14e0ef0c6c46d4f refactor: Fix logging.h includes (TheCharlatan)
84058e0eed9c05bc30984b39131e88ad1425628f refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan)
e4af2408f2ac59788567b6fc8cb3a68fc43da9fe refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan)
ef941ff1281e76308c3e746e592375bec023e9e4 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan)
b7a1ab5cb4e60230f62c94efb3a10d07c9af4883 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan)
d7437908cdf242626263ba9d5541addcddadc594 refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan)
ea8135de7e617259cda3fc7b1c8e7569d454fd57 refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan)
b9870c920dc475ec759eaf7339ea42aecba92138 refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan)
532ee812a499e13b123af6b8415d8de1f3804f0f refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan)
afc534df9adbf5599b286b5dc3531a4b9ac2d056 refactor: Wrap DestroyDB in dbwrapper helper (TheCharlatan)
Pull request description:
Leveldb headers are currently included in the `dbwrapper.h` file and thus available to many of Bitcoin Core's source files. However, leveldb-specific functionality should be abstracted by the `dbwrapper` and does not need to be available to the rest of the code. Having leveldb included in a widely-used header such as `dbwrapper.h` bloats the entire project's header tree.
The `dbwrapper` is a key component of the libbitcoinkernel library. Future users of this library would not want to contend with having the leveldb headers exposed and potentially polluting their project's namespace.
For these reasons, the leveldb headers are removed from the `dbwrapper` by moving leveldb-specific code to the implementation file and creating a [pimpl](https://en.cppreference.com/w/cpp/language/pimpl) where leveldb member variables are indispensable. As a final step, the leveldb include flags are removed from the `BITCOIN_INCLUDES` and moved to places where the dbwrapper is compiled.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), and more specifically its stage 1 step 3 "Decouple most non-consensus headers from libbitcoinkernel".
ACKs for top commit:
stickies-v:
re-ACK https://github.com/bitcoin/bitcoin/commit/d8f1222ac50f089a0af29eaf8ce0555bad8366ef
MarcoFalke:
ACK d8f1222ac50f089a0af29eaf8ce0555bad8366ef 🔠
Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
|
|
Since leveldb is no longer in our header tree, move its include flags to
whereever dbwrapper.cpp is built.
|
|
fa633aa6906f3b130b691568bcd20b2b76bb1cbb streams: Teach AutoFile how to XOR (MarcoFalke)
000019e158ef01f2bedc3fc1589f95e106e817ea Add AutoFile::detail_fread member function (MarcoFalke)
fa7724bc9d94c08d8facccd0a067d6a3b27fbbc6 refactor: Modernize AutoFile (MarcoFalke)
fa8d227d58f7baa5a9be1b88930f4813bf6eedb1 doc: Remove comments that just repeat what the code does (MarcoFalke)
fafe2ca0ce842cd8f0d8135fa8c8bac9b2c72da6 refactor: Remove redundant file check from AutoFile shift operators (MarcoFalke)
9999a49b3299bd25dde4805f5c68adef3876057f Extract util::Xor, Add key_offset option, Add bench (MarcoFalke)
Pull request description:
This allows `AutoFile` to roll an XOR pattern while reading or writing to the underlying file.
This is needed for https://github.com/bitcoin/bitcoin/pull/28052, but can also be used in any other place.
Also, there are tests, so I've split this up from the larger pull to make review easier, hopefully.
ACKs for top commit:
Crypt-iQ:
crACK fa633aa
willcl-ark:
Lightly tested ACK fa633aa690
jamesob:
reACK fa633aa6906f3b130b691568bcd20b2b76bb1cbb ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tree-SHA512: 6d66cad0a089a096d3f95e4f2b28bce80b349d4b76f53d09dc9a0bea4fc1b7c0652724469c37971ba27728c7d46398a4c1d289c252af4c0f83bb2fcbc6f8e90b
|
|
ciphers
Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
|
|
This adds an implementation of the ChaCha20Poly1305 AEAD exactly matching
the version specified in RFC8439 section 2.8, including tests and official
test vectors.
|
|
Remove the variant of ChaCha20Poly1305 AEAD that was previously added in
anticipation of BIP324 using it. BIP324 was updated to instead use rekeying
wrappers around otherwise unmodified versions of the ChaCha20 stream cipher
and the ChaCha20Poly1305 AEAD as specified in RFC8439.
|
|
6960c81cbfa6208d4098353e53b313e13a21cb49 kernel: Remove Univalue from kernel library (TheCharlatan)
10eb3a9faa977371facacee937b2e6dc26f008e0 kernel: Split ParseSighashString (TheCharlatan)
Pull request description:
Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files.
UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.
ACKs for top commit:
achow101:
ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
theuni:
Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49
stickies-v:
re-ACK https://github.com/bitcoin/bitcoin/commit/6960c81cbfa6208d4098353e53b313e13a21cb49
Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
|
|
It is not required by any of the kernel components.
A JSON library should not need to be part of a consensus library.
|
|
|
|
New code can call the method without having first to retrieve the raw
FILE* pointer via Get().
Also, move implementation to the cpp file. Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to interrupt kernel
operations when the chain tip changes.
This is a refactoring that does not affect behavior. (Looking at the code it
can appear like the new break statement in the ActivateBestChain function is a
change in behavior, but actually the previous StartShutdown call was indirectly
triggering a break before, because it was causing m_chainman.m_interrupt to be
true. The new code just makes the break more obvious.)
|
|
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
|
|
This change helps generalize shutdown code so an interrupt can be
provided to libbitcoinkernel callers. This may also be useful to
eventually de-globalize all of the shutdown code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
|
|
After initially being merged in #20487, it's no-longer clear that an
internal syscall sandboxing mechanism is something that Bitcoin Core
should have/maintain, especially when compared to better
maintained/supported alterantives, i.e firejail.
Note that given where it's used, the sandbox also gets dragged into the
kernel.
There is some related discussion in #24771.
This should not require any sort of deprecation, as this was only ever
an opt-in, experimental feature.
Closes #24771.
|
|
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from code that is not strictly required by it.
The settings code belongs into the common library and namespace, since
the kernel library should not depend on it. See doc/design/libraries.md
for more information on this rationale.
Changing the namespace of the moved functions is scripted in the
following commit.
|
|
|
|
Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.
|
|
|
|
|
|
With the previous move of AlertNotify out of the validation file, and
thus out of the kernel library, ScheduleBatchPriority is the last
remaining function used by the kernel library from util/system. Move it
to its own file, such that util/system can be moved out of the util
library in the following few commits.
Moving util/system out of the kernel library removes further networking
as well as shell related code from it.
|
|
This commit is part of the libbitcoinkernel project and seeks to remove
the ChainstateManager's and, more generally, the kernel library's
dependency on interface_ui with options methods in this and the following
few commits. By removing interface_ui from the kernel library, its
dependency on boost is reduced to just boost::multi_index.
Define a new kernel notification class with virtual methods for
notifying about internal kernel events. Create a new file in the node
library for defining a function creating the default set of notification
methods such that these do not need to be re-defined all over the
codebase. As a first step, add a `blockTip` method, wrapping
`uiInterface.NotifyBlockTip`.
|
|
calculate mining scores
6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965 [fuzz] Add MiniMiner target + diff fuzz against BlockAssembler (glozow)
3f3f2d59ea2946a7b7cc8cb0222fb602d62645d0 [unit test] GatherClusters and MiniMiner unit tests (glozow)
59afcc83548ea67a863dac7b75d000bc8f6a7023 Implement Mini version of BlockAssembler to calculate mining scores (glozow)
56484f0fdc44261e723563f59df886d5acdd851f [mempool] find connected mempool entries with GatherClusters(…) (glozow)
Pull request description:
Implement Mini version of BlockAssembler to calculate mining scores
Run the mining algorithm on a subset of the mempool, only disturbing the
mempool to copy out fee information for relevant entries. Intended to be
used by wallet to calculate amounts needed for fee-bumping unconfirmed
transactions.
From comments of sipa and glozow below:
> > In what way does the code added here differ from the real block assembly code?
>
> * Only operates on the relevant transactions rather than full mempool
> * Has the ability to remove transactions that will be replaced so they don't impact their ancestors
> * Does not hold mempool lock outside of the constructor, makes copies of the entries it needs instead (though I'm not sure if this has an effect in practice)
> * Doesn't do the sanity checks like keeping weight within max block weight and `IsFinalTx()`
> * After the block template is built, additionally calculates fees to bump remaining ancestor packages to target feerate
ACKs for top commit:
achow101:
ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Xekyo:
> ACK [6b605b9](https://github.com/bitcoin/bitcoin/commit/6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965) modulo `miniminer_overlap` test.
furszy:
ACK 6b605b91 modulo `miniminer_overlap` test.
theStack:
Code-review ACK 6b605b91c1faf2c7f7cc0c9d39b4fcfd66dc2965
Tree-SHA512: f86a8b4ae0506858a7b15d90f417ebceea5038b395c05c825e3796123ad3b6cb8a98ebb948521316802a4c6d60ebd7041093356b1e2c2922a06b3b96b3b8acb6
|
|
in Makefiles
1b1ffbd014b931afb9435ec10911b9a7c130d3e5 Build: Log when test -f fails in Makefile (TheCharlatan)
541012e621386cd824eed81295206a34ba3ba497 Build: Use AM_V_GEN in Makefiles where appropriate (TheCharlatan)
Pull request description:
This PR triages some behavior around Makefile recipe echoing suppression.
When generating new files as part of the Makefile the recipe is sometimes suppressed with $(AM_V_GEN) and sometimes with `@`. We should prefer $(AM_V_GEN), since this also prints the lines in silent mode. This is arguably more in style with the current recipe echoing.
Before:
`Generated test/data/script_tests.json.h`
Now:
` GEN test/data/script_tests.json.h`
A side effect of this change is that the recipe for generating build.h is now echoed on each make run. Arguably this makes its generation more transparent.
Sometimes the error emitted by `test -f` is currently thrown without any logging. This makes it a bit harder to debug. Instead, print a helpful log message to point the developer in the right direction.
Alternatively this could have been implemented by just removing the recipe echo suppression (@), but the subsequent make output became too noisy.
ACKs for top commit:
fanquake:
ACK 1b1ffbd014b931afb9435ec10911b9a7c130d3e5
Tree-SHA512: e31869fab25e72802b692ce6735f9561912caea903c1577101b64c9cb115c98de01a59300e8ffe7b05b998345c1b64a79226231d7d1453236ac338c62dc9fbb3
|
|
This is the first of a number of commits with the goal of moving the
chain type definitions out of chainparamsbase to their own file and
implementing them as enums instead of constant strings. The goal is to
allow the kernel chainparams to no longer include chainparamsbase.
The commit is part of an ongoing effort to decouple the libbitcoinkernel
library from the ArgsManager and other functionality that should not be
part of the kernel library.
|
|
be55f545d53d44fdcf2d8ae802e9eae551d120c6 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲
ryanofsky:
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.
hebasto:
ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
|
|
9f947fc3d4b779f017332135323b34e8f216f613 Use PoolAllocator for CCoinsMap (Martin Leitner-Ankerl)
5e4ac5abf54f8e6d6330df0c73119aa0cca4c103 Call ReallocateCache() on each Flush() (Martin Leitner-Ankerl)
1afca6b663bb54022afff193fd9d83856606b189 Add PoolResource fuzzer (Martin Leitner-Ankerl)
e19943f049ed8aa4f32a1d8440a9fbf160367f0f Calculate memory usage correctly for unordered_maps that use PoolAllocator (Martin Leitner-Ankerl)
b8401c3281978beed6198b2f9782b6a8dd35cbd7 Add pool based memory resource & allocator (Martin Leitner-Ankerl)
Pull request description:
A memory resource similar to `std::pmr::unsynchronized_pool_resource`, but optimized for node-based containers. The goal is to be able to cache more coins with the same memory usage, and allocate/deallocate faster.
This is a reimplementation of #22702. The goal was to implement it in a way that is simpler to review & test
* There is now a generic `PoolResource` for allocating/deallocating memory. This has practically the same API as `std::pmr::memory_resource`. (Unfortunately I cannot use std::pmr because libc++ simply doesn't implement that API).
* Thanks to sipa there is now a fuzzer for PoolResource! On a fast machine I ran it for ~770 million executions without finding any issue.
* The estimation of the correct node size is now gone, PoolResource now has multiple pools and just needs to be created large enough to have space for the unordered_map nodes.
I ran benchmarks with #22702, mergebase, and this PR. Frequency locked Intel i7-8700, clang++ 13.0.1 to reindex up to block 690000.
```sh
bitcoind -dbcache=5000 -assumevalid=00000000000000000002a23d6df20eecec15b21d32c75833cce28f113de888b7 -reindex-chainstate -printtoconsole=0 -stopatheight=690000
```
The performance is practically identical with #22702, just 0.4% slower. It's ~21% faster than master:
![Progress in Million Transactions over Time(2)](https://user-images.githubusercontent.com/14386/173288685-91952ade-f304-4825-8bfb-0725a71ca17b.png)
![Size of Cache in MiB over Time](https://user-images.githubusercontent.com/14386/173291421-e6b410be-ac77-479b-ad24-5fafcebf81eb.png)
Note that on cache drops mergebase's memory doesnt go so far down because it does not free the `CCoinsMap` bucket array.
![Size of Cache in Million tx over Time(1)](https://user-images.githubusercontent.com/14386/173288703-a80c9c9e-93c8-4a16-9df8-610c89c61cc4.png)
ACKs for top commit:
LarryRuane:
ACK 9f947fc3d4b779f017332135323b34e8f216f613
achow101:
re-ACK 9f947fc3d4b779f017332135323b34e8f216f613
john-moffett:
ACK 9f947fc3d4b779f017332135323b34e8f216f613
jonatack:
re-ACK 9f947fc3d4b779f017332135323b34e8f216f613
Tree-SHA512: 48caf57d1775875a612b54388ef64c53952cd48741cacfe20d89049f2fb35301b5c28e69264b7d659a3ca33d4c714d47bafad6fd547c4075f08b45acc87c0f45
|
|
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
|
|
Move isminetype and isminefilter there this commit, add WalletPurpose type next
commit.
|
|
Rewrite the same algo instead of reusing BlockAssembler because we have
a few extra requirements that would make the changes invasive and
difficult to review:
- Only operate on the relevant transactions rather than full mempool
- Remove transactions that will be replaced so they can't bump their ancestors
- Don't hold mempool lock outside of the constructor
- Skip things like max block weight and IsFinalTx
- Additionally calculate fees to bump remaining ancestor packages to target feerate
Co-authored-by: Murch <murch@murch.one>
|
|
A memory resource similar to std::pmr::unsynchronized_pool_resource, but
optimized for node-based containers.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
|
|
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
|
|
This is an extraction of filesystem related functions from util/system
into their own utility file.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving these functions out of system.h allows including them from a
separate source file without including the ArgsManager definitions from
system.h.
|
|
functionality to kernel
b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6 refactor: Don't use global chainparams in chainstatemanager method (TheCharlatan)
382b692a503355df7347efd9c128aff465b5583e Split non/kernel chainparams (Carl Dong)
edabbc78a3bc272b2b802e1dbab73d6ed8e31e96 Add factory functions for Main/Test/Sig/Reg chainparams (Carl Dong)
d938098398814f37fed9b018b44716179cfa4b03 Remove UpdateVersionBitsParameters (Carl Dong)
84b85786f0f5cb23cc257a4464ae345e1d372313 Decouple RegTestChainParams from ArgsManager (Carl Dong)
76cd4e7c96242398172989609f1b9a8843c404b4 Decouple SigNetChainParams from ArgsManager (Carl Dong)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". dongcarl is the original author of this patchset, these commits were taken from https://github.com/dongcarl/bitcoin/tree/2022-03-libbitcoinkernel-chainparams-args-only.
#### Context
The bitcoin kernel library currently relies on code containing user configurations through the `ArgsManager`. This is not optimal, since as a stand-alone library it should not rely on bitcoind's argument parsing logic. Instead, its interfaces should accept control and options structs that control the kernel library's desired configuration.
Similar work towards decoupling the `ArgsManager` from the kernel has been done in
https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527 and https://github.com/bitcoin/bitcoin/pull/25862.
#### Changes
By moving the `CChainParams` class definition into the kernel and giving it new factory functions `CChainParams::{RegTest,SigNet,Main,TestNet}`it can be constructed without an `ArgsManager` reference, unlike the current factory function `CreateChainParams`.
The first few commits remove uses of `ArgsManager` within `CChainParams`. Then the `CChainParams` definition is moved to a new file in the `kernel/` subdirectory.
ACKs for top commit:
MarcoFalke:
re-ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6 🛁
ryanofsky:
Code review ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6. Only changes since last review were recent review suggestions.
ajtowns:
ACK b3e78dc91d01e364b77aacd9fb9a2f88688ab8a6
Tree-SHA512: 3835aca1d3e3c75cc3303dd584bab3a77e58f6c678724a5e359fe4b0e17e0763a00931ee6191f516b9fde50496f59cc691f0709c0254206db3863bbf7ab2cacd
|
|
Moves chainparams code not using the ArgsManager to the kernel.
Subsequently use the kernel chainparams header now where possible in
order to further decouple chainparams call sites from gArgs.
|
|
|
|
This is a minimal extraction of a single function, but also the only use
of std::exception in util/system.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving the function out of system.h allows including it from a separate
source file without including the ArgsManager definitions from system.h.
|
|
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code
reading config files and creating the datadir.
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the
GUI error text has changed from "Error: Cannot parse configuration file:" to
"Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the
error text has changed from "Failed loading settings file" to "Settings
file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the
error text has changed from "Failed saving settings file" to "Settings file
could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read),
there is an normal error dialog showing "Error: filesystem error: status:
Permission denied [.../settings.json]", instead of an uncaught exception
|
|
Symbol visibility issues are not actually fixed yet because we have not yet
defined an api and exported symbols, but everything is now in place for that.
|
|
Building binaries against our uninstalled shared libs is impractical. Instead,
to test them, we'll need to work on a runtime shared-lib execution harness.
|
|
Libtool is unable to calculate dependencies correctly so give it some help.
|
|
Add CoinsViewOptions struct to remove ArgsManager uses from txdb.
To reduce size of this commit, this moves references to gArgs variable out of
txdb.cpp to calling code in validation.cpp. But these moves are temporary. The
gArgs references in validation.cpp are moved out to calling code in init.cpp in
later commits.
This commit does not change behavior.
|
|
Add DBParams and DBOptions structs to remove ArgsManager uses from dbwrapper.
To reduce size of this commit, this moves references to gArgs variable out of
dbwrapper.cpp to calling code in txdb.cpp. But these moves are temporary. The
gArgs references in txdb.cpp are moved out to calling code in init.cpp in later
commits.
This commit does not change behavior.
|