1. It didn't actually disable asm usage in our code. Regardless of the setting,
asm is used in random.cpp and support/cleanse.cpp.
2. The value wasn't forwarded to libsecp as a user might have reasonably
expected.
3. We now have the DISABLE_OPTIMIZED_SHA256 define which is what disable-asm
actually did in practice.
If there is any desire, we can hook DISABLE_OPTIMIZED_SHA256 up to a new
configure option that actually does what it says.
29029df5c7 [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea795e [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5c62 [functional test] v3 transaction submission (glozow)
27c8786ba9 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea55b2 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2e7d [policy] add v3 policy rules (glozow)
9a29d470fb [rpc] return full string for package_msg and package-error (glozow)
158623b8e0 [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)
Pull request description:
See #27463 for overall package relay tracking.
Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418
Rationale:
- There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see #28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
- Switching to a cluster-based mempool (see #27677 and #28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.
V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.
Immediate benefits:
- You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
- Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.
This also enables some other cool things (again see #27463 for overall roadmap):
- Ephemeral Anchors
- Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
- We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
- We can switch to a cluster-based mempool [5] (#27677#28676), which removes CPFP carve out [6].
[1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
[2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
[3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
[4]: Original PR #25038 also contains a lot of the discussion
[5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
[6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12
ACKs for top commit:
sdaftuar:
ACK 29029df5c7
achow101:
ACK 29029df5c7
instagibbs:
ACK 29029df5c7 modulo that
Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
bbf218d061 crypto: remove sha256_sse4 from the base crypto helper lib (Cory Fields)
4dbd0475d8 crypto: remove use of BUILD_BITCOIN_INTERNAL macro in sha256 (Cory Fields)
Pull request description:
Replace it with a more explicit `DISABLE_OPTIMIZED_SHA256` and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of optimized sha256 for the sake of simplicity.
Also remove the `BUILD_BITCOIN_INTERNAL` define from libbitcoinkernel for now as it does not export an api. When it does we can pick a less confusing define to control its exports.
Removing the define should have the effect of enabling sha256 optimizations for the kernel.
ACKs for top commit:
TheCharlatan:
Re-ACK bbf218d061
hebasto:
re-ACK bbf218d061
Tree-SHA512: 7c17592bb2d3e671779f96903cb36887c5785408213bffbda1ae37b66e6bcfaffaefd0c1bf2d1a407060cd377e3d4881cde3a73c429a1aacb677f370314a066a
00c1e2aa44 build: fix optimisation flags used for --coverage (fanquake)
1dc2c9b385 ci: cleanup C*FLAG usage in Valgrind jobs (fanquake)
6cc2a38c13 build: add sanitizer flags to configure output (fanquake)
08cd5aca18 build: always set -g -O2 in CORE_CXXFLAGS (fanquake)
Pull request description:
Rather than trying to sporadically rely on / override Autoconf default behaviour. Just always override (if unset), and always set the flags we want (which are the same as the Autoconf defaults).
Removes the need for duplicate code to clear (if not overridden) `CXXFLAGS`.
Fixes cases of "missing" `-O2`. i.e this PR when running a Valgrind CI job with changes here:
```bash
CXXFLAGS = -g -O2 -fdebug-prefix-map=$(abs_top_srcdir)=. -Wstack-protector -fstack-protector-all -mbranch-protection=bti -Werror -fsanitize=fuzzer -gdwarf-4
```
Fixes configure output to reflect actual compilation flag ordering, so it's useful.
Note that if we do still end up with a duplicate "-g -O2" when compiling, that has no effect, and I don't really thinks it's something worth trying to optimize.
ACKs for top commit:
TheCharlatan:
lgtm ACK 00c1e2aa44
hebasto:
ACK 00c1e2aa44, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.
theuni:
ACK 00c1e2aa44
Tree-SHA512: cf6c7acf813ba10b198561e83eb72e9b2532a39cb1767c452d031e82921dcd42a47b129735b24c4e36131fd0c8fe7457f7cae870c1e011cdfdd430bdc4d4912b
6acec6b9ff multiprocess: Add type conversion code for UniValue types (Ryan Ofsky)
0cc74fce72 multiprocess: Add type conversion code for serializable types (Ryan Ofsky)
4aaee23921 test: add ipc test to test multiprocess type conversion code (Ryan Ofsky)
Pull request description:
Add type conversion hooks to allow `UniValue` objects, and objects that have `CDataStream` `Serialize` and `Unserialize` methods to be used as arguments and return values in Cap'nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.
The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 6acec6b9ff
dergoegge:
reACK 6acec6b9ff
Tree-SHA512: 5d2cbc5215d488b876d34420adf91205dabf09b736183dcc85aa86255e3804c2bac5bab6792dacd585ef99a1d92cf29c8afb3eb65e4d953abc7ffe41994340c6
This avoids cases of missing -O2, when *FLAGS has been overriden.
Removes the need for duplicate code to clear autoconf defaults.
Also, move CORE_CXXFLAGS before DEBUG_CXXFLAGS, so that -O2 is always
overriden if debugging etc.
This has outlived its usefulness, doesn't gel well with
newer compilers & `-flto` related options, i.e thin vs full, or `=auto`,
and having `-flto` as the only option means that sometimes this just
needs to be worked around, i.e in oss-fuzz:
https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh.
While it was convenient when `-flto` was newer, support for `-flto` is now
in all compilers we use, and there's also no-longer any real need
for us to treat `-flto` different to any other optimization option.
Remove it, to remove build complexity, and so there's no need
to port a similar option to CMake.
Note that the LTO option remains in depends, because we still a way to
build packages that have LTO specific patches/options.
If we decide to merge this, I'll follow up downstream in oss-fuzz first,
to make sure we don't break the build.
Replace it with a more explicit DISABLE_OPTIMIZED_SHA256 and clean up some.
The macro was originally used by libbitcoinconsensus which opts out of
optimized sha256 for the sake of simplicity.
Also remove the BUILD_BITCOIN_INTERNAL define from libbitcoinkernel for now
as it does not export an api. When it does we can pick a less confusing define
to control its exports.
Removing the define should have the effect of enabling sha256 optimizations
for the kernel.
6db04be102 Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly (Ryan Ofsky)
213542b625 refactor: Add InitContext function to initialize NodeContext with global pointers (Ryan Ofsky)
feeb7b816a refactor: Remove calls to StartShutdown from KernelNotifications (Ryan Ofsky)
6824eecaf1 refactor: Remove call to StartShutdown from stop RPC (Ryan Ofsky)
1d92d89edb util: Get rid of uncaught exceptions thrown by SignalInterrupt class (Ryan Ofsky)
ba93966368 refactor: Remove call to ShutdownRequested from IndexWaitSynced (Ryan Ofsky)
42e5829d97 refactor: Remove call to ShutdownRequested from HTTPRequest (Ryan Ofsky)
73133c36aa refactor: Add NodeContext::shutdown member (Ryan Ofsky)
f4a8bd6e2f refactor: Remove call to StartShutdown from qt (Ryan Ofsky)
f0c73c1336 refactor: Remove call to ShutdownRequested from rpc/mining (Ryan Ofsky)
263b23f008 refactor: Remove call to ShutdownRequested from chainstate init (Ryan Ofsky)
Pull request description:
This change drops `shutdown.h` and `shutdown.cpp` files, replacing them with a `NodeContext::shutdown` member which is used to trigger shutdowns directly. This gets rid of an unnecessary layer of indirection, and allows getting rid of the `kernel::g_context` global.
Additionally, this PR tries to improve error handling of `SignalInterrupt` code by marking relevant methods `[[nodiscard]]` to avoid the possibility of uncaught exceptions mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
Behavior is changing In a few cases which are noted in individual commit messages. Particularly: GUI code more consistently interrupts RPCs when it is shutting down, shutdown state no longer persists between unit tests, the stop RPC now returns an RPC error if requesting shutdown fails instead of aborting, and other failed shutdown calls now log errors instead of aborting.
This PR is a net reduction in lines of code, but in some cases the explicit error handling and lack of global shutdown functions do make it more verbose. The verbosity can be seen as good thing if it discourages more code from directly triggering shutdowns, and instead encourages code to return errors or send notifications that could be translated into shutdowns. Probably a number of existing shutdown calls could just be replaced by better error handling.
ACKs for top commit:
achow101:
ACK 6db04be102
TheCharlatan:
Re-ACK 6db04be102
maflcko:
ACK 6db04be102👗
stickies-v:
re-ACK 6db04be102
Tree-SHA512: 7a34cb69085f37e813c43bdaded1a0cbf6c53bd95fdde96f0cb45346127fc934604c43bccd3328231ca2f1faf712a7418d047ceabd22ef2dca3c32ebb659e634
fa3da629a1 Remove DirIsWritable, GetUniquePath (MarcoFalke)
fad3a9793b Return LockResult::ErrorWrite in LockDirectory (MarcoFalke)
fa0afe7408 refactor: Return enum in LockDirectory (MarcoFalke)
Pull request description:
`GetUniquePath` is only used in tests and in `DirIsWritable`. The check by `DirIsWritable` is redundant with the check done in `LockDirectory`.
Fix the redundancy by removing everything, except `LockDirectory`.
ACKs for top commit:
TheCharlatan:
Re-ACK fa3da629a1
hebasto:
ACK fa3da629a1, I have reviewed the code and it looks OK.
Tree-SHA512: e95f18cd586de7582e9c08ac7ddb860bfcfcbc8963804f45c5784c5e4c0598dc59ae7e45dd4daf30a5020dbf8433f5db2ad06e46a8676371982003790043c6c9
This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after #27861
But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
91504cbe0d rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918b tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b99 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd382 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0d
TheCharlatan:
Re-ACK 91504cbe0d
willcl-ark:
ACK 91504cbe0d
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
Allow any C++ object that has Serialize and Unserialize methods and can be
serialized to a bitcoin CDataStream to be converted to a capnproto Data field
and passed as arguments or return values to capnproto methods using the Data
type.
Extend IPC unit test to cover this and verify the serialization happens
correctly.
`CBlockPolicyEstimator` will implement `CValidationInterface` and
subscribe to its notification to process transactions added and removed
from the mempool.
Re-delegate calculation of `validForFeeEstimation` from validation to fee estimator.
Also clean up the validForFeeEstimation arg thats no longer needed in `CTxMempool`.
Co-authored-by: Matt Corallo <git@bluematt.me>
9b3da70bd0 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447964 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2098 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c12 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea7 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd0 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd0
glozow:
ACK 9b3da70bd0
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
4313c77400 make DisconnectedBlockTransactions responsible for its own memory management (glozow)
cf5f1faa03 MOVEONLY: DisconnectedBlockTransactions to its own file (glozow)
2765d6f343 rewrite DisconnectedBlockTransactions as a list + map (glozow)
79ce9f0aa4 add std::list to memusage (glozow)
59a35a7398 [bench] DisconnectedBlockTransactions (glozow)
925bb723ca [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 4313c77400
stickies-v:
ACK 4313c77400
TheCharlatan:
ACK 4313c77400
Tree-SHA512: 273c80866bf3acd39b2a039dc082b7719d2d82e0940e1eb6c402f1c0992e997256722b85c7e310c9811238a770cfbdeb122ea4babbc23835d17128f214a1ef9e
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>
91d924ede1 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow)
bacdb2e208 Clean up script/standard.{h/cpp} includes (Andrew Chow)
f3c9078b4c Clean up things that include script/standard.h (Andrew Chow)
8bbe257bac MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow)
7a172c76d2 Move CTxDestination to its own file (Andrew Chow)
145f36ec81 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow)
86ea8bed54 Move CScriptID to script.{h/cpp} (Andrew Chow)
b81ebff0d9 Remove ScriptHash from CScriptID constructor (Andrew Chow)
cba69dda3d 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 91d924ede1
ajtowns:
ACK 91d924ede1
MarcoFalke:
ACK 91d924ede1😇
murchandamus:
ACK 91d924ede1
darosior:
Code review ACK 91d924ede1.
theStack:
Code-review ACK 91d924ede1
Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
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.
d8f1222ac5 refactor: Correct dbwrapper key naming (TheCharlatan)
be8f159ac5 build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan)
c95b37d641 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan)
c534a615e9 refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan)
586448888b refactor: Move HandleError to dbwrapper implementation (TheCharlatan)
dede0eef7a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan)
a5c2eb5748 refactor: Fix logging.h includes (TheCharlatan)
84058e0eed refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan)
e4af2408f2 refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan)
ef941ff128 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan)
b7a1ab5cb4 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan)
d7437908cd refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan)
ea8135de7e refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan)
b9870c920d refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan)
532ee812a4 refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan)
afc534df9a 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 d8f1222ac5
MarcoFalke:
ACK d8f1222ac5🔠
Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
fa633aa690 streams: Teach AutoFile how to XOR (MarcoFalke)
000019e158 Add AutoFile::detail_fread member function (MarcoFalke)
fa7724bc9d refactor: Modernize AutoFile (MarcoFalke)
fa8d227d58 doc: Remove comments that just repeat what the code does (MarcoFalke)
fafe2ca0ce refactor: Remove redundant file check from AutoFile shift operators (MarcoFalke)
9999a49b32 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 fa633aa690 ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tree-SHA512: 6d66cad0a089a096d3f95e4f2b28bce80b349d4b76f53d09dc9a0bea4fc1b7c0652724469c37971ba27728c7d46398a4c1d289c252af4c0f83bb2fcbc6f8e90b
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.
6960c81cbf kernel: Remove Univalue from kernel library (TheCharlatan)
10eb3a9faa 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 6960c81cbf
theuni:
Re-ACK 6960c81cbf
stickies-v:
re-ACK 6960c81cbf
Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
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.
