7014e080154f9c82e4fbe043af292a1e761cdb55 doc: remove mention of glibc 2.10+ (fanquake)
Pull request description:
We already require glibc 2.27+, so mentioning a much older version here is redundant.
ACKs for top commit:
TheCharlatan:
ACK 7014e080154f9c82e4fbe043af292a1e761cdb55
Tree-SHA512: 883a566a80cabe34bfb5d902990f3eca08d0e11438e6c128d311e558f373ec232b0934deb85d12d796baacfeae590af8c73aa1b2faef07f27ffa9011270ffd96
This is achieved by letting the index sync thread wait until
reindex-chainstate is finished.
This also disables the pruning check when reindexing the chainstate (which is
incompatible with prune mode) because there would be no chain at this point
in init.
The index sync code has logic to go back the chain to the forking point, while
also updating index-specific state, which is necessary to prevent
possible corruption of the coinstatsindex.
Also add a test for this (a reorg happens while the index is deactivated)
that would not pass before this change.
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
33e2b82a4fc990253ff77655f437c7aed336bc55 wallet, bench: Remove unused database options from WalletBenchLoading (Andrew Chow)
80ace042d8fece9be50bfef1be64c6e5720e87e6 tests: Modify records directly in wallet ckey loading test (Andrew Chow)
b3bb17d5d07f51ac2e501e4a7a3bbcd17144070f tests: Update DuplicateMockDatabase for MockableDatabase (Andrew Chow)
f0eecf5e408238c64b77b0a4974ba2b9edb17487 scripted-diff: Replace CreateMockWalletDB with CreateMockableWalletDB (Andrew Chow)
075962bc25a90661612fe4613cd50ea1cae21f52 wallet, tests: Include wallet/test/util.h (Andrew Chow)
14aa4cb1e44f089a6022a2b14a98bca4a7dd9a01 wallet: Move DummyDatabase to salvage (Andrew Chow)
f67a385556c60b2e4788a378196a395fca0539f5 wallet, tests: Replace usage of dummy db with mockable db (Andrew Chow)
33c6245ac1ecdfe25b1ee4fd9e93c43393634ae3 Introduce MockableDatabase for wallet unit tests (Andrew Chow)
Pull request description:
For the wallet's unit tests, we currently use either `DummyDatabase` or memory-only versions of either BDB or SQLite. The tests that use `DummyDatabase` just need a `WalletDatabase` so that the `CWallet` can be constructed, while the tests using the memory-only databases just need a backing data store. There is also a `FailDatabase` that is similar to `DummyDatabase` except it fails be default or can have a configured return value. Having all of these different database types can make it difficult to write tests, particularly tests that work when either BDB or SQLite is disabled.
This PR unifies all of these different unit test database classes into a single `MockableDatabase`. Like `DummyDatabase`, most functions do nothing and just return true. Like `FailDatabase`, the return value of some functions can be configured on the fly to test various failure cases. Like the memory-only databases, records can actually be written to the `MockableDatabase` and be retrieved later, but all of this is still held in memory. Using `MockableDatabase` completely removes the need for having BDB or SQLite backed wallets in the unit tests for the tests that are not actually testing specific database behaviors.
Because `MockableDatabase`s can be created by each unit test, we can also control what records are stored in the database. Records can be added and removed externally from the typical database modification functions. This will give us greater ability to test failure conditions, particularly those involving corrupted records.
Possible alternative to #26644
ACKs for top commit:
furszy:
ACK 33e2b82
TheCharlatan:
ACK 33e2b82a4fc990253ff77655f437c7aed336bc55
Tree-SHA512: c2b09eff9728d063d2d4aea28a0f0e64e40b76483e75dc53f08667df23bd25834d52656cd4eafb02e552db0b9e619cfdb1b1c65b26b5436ee2c971d804768bcc
5b3406094f2679dfb3763de4414257268565b943 net_processing: Boost inv trickle rate (Anthony Towns)
228e9201efb5574b1b96bb924de1d2e8dd1317f3 txmempool: have CompareDepthAndScore sort missing txs first (Anthony Towns)
Pull request description:
Couple of performance improvements when draining the inventory-to-send queue:
* drop txs that have already been evicted from the mempool (or included in a block) immediately, rather than at the end of processing
* marginally increase outgoing trickle rate during spikes in tx volume
ACKs for top commit:
willcl-ark:
ACK 5b34060
instagibbs:
ACK 5b3406094f
darosior:
utACK 5b3406094f2679dfb3763de4414257268565b943
glozow:
code review ACK 5b3406094f2679dfb3763de4414257268565b943
dergoegge:
utACK 5b3406094f2679dfb3763de4414257268565b943
Tree-SHA512: 155cd3b5d150ba3417c1cd126f2be734497742e85358a19c9d365f4f97c555ff9e846405bbeada13c3575b3713c3a7eb2f780879a828cbbf032ad9a6e5416b30
5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3 refactor, blockstorage: Replace stopafterblockimport arg (TheCharlatan)
18e5ba7c8002bcd473ee29ce4b5bfc56df6142a4 refactor, blockstorage: Replace blocksdir arg (TheCharlatan)
02a0899527ba3d31329e56c791c9dbf36075bb84 refactor, BlockManager: Replace fastprune from arg with options (TheCharlatan)
a498d699e3fdac5bfdb33020a1fd6c4a79989752 refactor/iwyu: Complete includes for blockmanager_args (TheCharlatan)
f0bb1021f0d60f5f19176e67a66fcf7c325f88d1 refactor: Move functions to BlockManager methods (TheCharlatan)
cfbb2124939822e95265a39242ffca3d86bac6e8 zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier (TheCharlatan)
8ed4ff8e05d61a8e954d72cebdc2e1d1ab24fb84 refactor: Declare g_zmq_notification_interface as unique_ptr (TheCharlatan)
Pull request description:
The libbitcoin_kernel library should not rely on the `ArgsManager`, but rather use option structs that can be passed to the various classes it uses. This PR removes reliance on the `ArgsManager` from the `blockstorage.*` files. Like similar prior work, it uses the options struct in the `BlockManager` that can be populated with `ArgsManager` values.
Some related prior work: https://github.com/bitcoin/bitcoin/pull/26889https://github.com/bitcoin/bitcoin/pull/25862https://github.com/bitcoin/bitcoin/pull/25527https://github.com/bitcoin/bitcoin/pull/25487
Related PR removing blockstorage globals: https://github.com/bitcoin/bitcoin/pull/25781
ACKs for top commit:
ryanofsky:
Code review ACK 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3. Since last ACK just added std::move and fixed commit title. Sorry for the noise!
mzumsande:
Code Review ACK 5ff63a09a9edd1204b2cc56cf6f48a44adab7bb3
Tree-SHA512: 4bde8fd140a40b97eca923e9016d85dcea6fad6fd199731f158376294af59c3e8b163a0725aa47b4be3519b61828044e0a042deea005e0c28de21d8b6c3e1ea7
72efc26439da9a1344a19569fb0cab01f82ae7d1 util: improve streams.h:FindByte() performance (Larry Ruane)
604df63f6c70b9692b067777ddb38d946ac0b2fc [bench] add streams findbyte (gzhao408)
Pull request description:
This PR is strictly a performance improvement; there is no functional change. The `CBufferedFile::FindByte()` method searches for the next occurrence of the given byte in the file. Currently, this is done by explicitly inspecting each byte in turn. This PR takes advantage of `std::find()` to do the same more efficiently, improving its CPU runtime by a factor of about 25 in typical use.
ACKs for top commit:
achow101:
re-ACK 72efc26439da9a1344a19569fb0cab01f82ae7d1
stickies-v:
re-ACK 72efc26439da9a1344a19569fb0cab01f82ae7d1
Tree-SHA512: ddf0bff335cc8aa34f911aa4e0558fa77ce35d963d602e4ab1c63090b4a386faf074548daf06ee829c7f2c760d06eed0125cf4c34e981c6129cea1804eb3b719
Add a stop_after_block_import field to the BlockManager options. Use
this field instead of the global gArgs.
This should allow users of the BlockManager to not rely on the global
Args.
Add a blocks_dir field to the BlockManager options. Move functions
relying on the global gArgs to get the blocks_dir into the BlockManager
class.
This should eventually allow users of the BlockManager to not rely on
the global Args and instead pass in their own options.
Remove access to the global gArgs for the fastprune argument and
replace it by adding a field to the existing BlockManager Options
struct.
When running `clang-tidy-diff` on this commit, there is a diagnostic
error: `unknown type name 'uint64_t' [clang-diagnostic-error] uint64_t
prune_target{0};`, which is fixed by including cstdint.
This should eventually allow users of the BlockManager to not rely on
the global gArgs and instead pass in their own options.
This is a commit in preparation for the next few commits. The functions
are moved to methods to avoid their re-declaration for the purpose of
passing in BlockManager options.
The functions that were now moved into the BlockManager should no longer
use the params as an argument, but instead use the member variable.
In the moved ReadBlockFromDisk and UndoReadFromDisk, change
the function signature to accept a reference to a CBlockIndex instead of
a raw pointer. The pointer is expected to be non-null, so reflect that
in the type.
To allow for the move of functions to BlockManager methods all call
sites require an instantiated BlockManager, or a callback to one.
The lambda captures a reference to the chainman unique_ptr to retrieve
block data. An assert is added on the chainman to ensure that the lambda
is not used while the chainman is uninitialized.
This is done in preparation for the following commits where blockstorage
functions are made BlockManager methods.
fa266c4bbf564308ddbc12653527226506902084 Temporarily work around gcc-13 warning bug in interfaces_tests (MarcoFalke)
fa28850562bda0c2049aaff42103dfa6f05395dc Fix clang-tidy performance-unnecessary-copy-initialization warnings (MarcoFalke)
faaa60a30e7738a1490c9c2bb8963420f53c7f2d Remove unused find_value global function (MarcoFalke)
fa422aeec2909df0151177816dc1ff5eb5a1fbab scripted-diff: Use UniValue::find_value method (MarcoFalke)
fa548ac872c094edc94c2afda5cc9b0d84f73af0 Add UniValue::find_value method (MarcoFalke)
Pull request description:
The global function has issues:
* It causes gcc-13 warnings, see https://github.com/bitcoin/bitcoin/issues/26926
* There is no rationale for it being a global function, when it acts like a member function
* `performance-unnecessary-copy-initialization` clang-tidy isn't run on it
Fix all issues by making it a member function.
ACKs for top commit:
achow101:
ACK fa266c4bbf564308ddbc12653527226506902084
hebasto:
re-ACK fa266c4bbf564308ddbc12653527226506902084
Tree-SHA512: 6c4e25da3122cd3b91c376bef73ea94fb3beb7bf8ef5cb3853c5128d95bfbacbcbfb16cc843eb7b1a7ebd350c2b6311f8085eeacf9aeeab3366987037d209e44
Ensures better memory safety for this global. This came up during
discussion of the following commit, but is not strictly required for its
implementation.
If transactions are being added to the mempool at a rate faster than 7tx/s
(INVENTORY_BROADCAST_PER_SECOND) then peers' inventory_to_send queue can
become relatively large. If this happens, increase the number of txids
we include in an INV message (normally capped at 35) by 5 for each 1000
txids in the queue.
This will tend to clear a temporary excess out reasonably quickly; an
excess of 4000 invs to send will be cleared down to 1000 in about 30
minutes, while an excess of 20000 invs would be cleared down to 1000 in
about 60 minutes.
We use CompareDepthAndScore to choose an order of txs to inv. Rather
than sorting txs that have been evicted from the mempool at the end
of the list, sort them at the beginning so they are removed from
the queue immediately.
d168458d1ff987e0d741c75ac1d4b63ae0cfb7e7 scripted-diff: Remove unused chainparamsbase includes (TheCharlatan)
e9ee8aaf3acdf6dce2b339916d4c602484570050 Add missing definitions in prep for scripted diff (TheCharlatan)
ba8fc7d788932b25864fb260ca14983aa2398c23 refactor: Replace string chain name constants with ChainTypes (TheCharlatan)
401453df419af35957ec711423ac3d93ad512fe8 refactor: Introduce ChainType getters for ArgsManager (TheCharlatan)
bfc21c31b2186f7d30fc9a9ca7d6887ab61c6fb9 refactor: Create chaintype files (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is also a follow up to #26177.
It replaces pull request https://github.com/bitcoin/bitcoin/pull/27294, which just moved the constants to a new file, but did not re-declare them as enums.
The code move of the chain name constants out of the `chainparamsbase` to their own separate header allows the kernel `chainparams` to no longer include `chainparamsbase`. The `chainparamsbase` contain references to the `ArgsManager` and networking related options that should not belong to the kernel library. Besides this move, the constants are re-declared as enums with helper functions facilitating string conversions.
ACKs for top commit:
ryanofsky:
Code review ACK d168458d1ff987e0d741c75ac1d4b63ae0cfb7e7. Just suggested changes since last review.
Tree-SHA512: ac2fbe5cbbab4f52eae1e30af1f16700b6589eb4764c328a151a712adfc37f326cc94a65c385534c57d4bc92cc1a13bf1777d92bc924a20dbb30440e7380b316
fae1d9cdede0e88cabbf8130869be31276457c34 refactor: Remove unused GetTimeMillis (MarcoFalke)
Pull request description:
The function is unused, not type-safe, and does not denote the underlying clock type. So remove it.
ACKs for top commit:
willcl-ark:
tACK fae1d9cded
Tree-SHA512: 41ea7125d1964192b85a94265be974d02bf1e79b1feb61bff11486dc0ac811745156940ec5cad2ad1f94b653936f8ae563c959c1c4142203a55645fcb83203e8
This is a follow-up to previous commits moving the chain constants out
of chainparamsbase.
The script removes the chainparamsbase header in all files where it is
included, but not used. This is done by filtering against all defined
symbols of the header as well as its respective .cpp file.
The kernel chainparams now no longer relies on chainparamsbase.
-BEGIN VERIFY SCRIPT-
sed -i '/#include <chainparamsbase.h>/d' $( git grep -l 'chainparamsbase.h' | xargs grep -L 'CBaseChainParams\|CreateBaseChainParams\|SetupChainParamsBaseOptions\|BaseParams\|SelectBaseParams\|chainparamsbase.cpp' )
-END VERIFY SCRIPT-
This commit effectively moves the definition of these constants
out of the chainparamsbase to their own file.
Using the ChainType enums provides better type safety compared to
passing around strings.
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.
These are introduced for the next commit where the usage of the
ChainType is adopted throughout the code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
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.
fe49f06c0e91b96feb8d8f1bd478c3173f14782c doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46ea10302a928fcf0f53b7aed77ad260 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
darosior:
re-ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
fa83fb31619c19a1a30b4181486601a944941b16 wallet: Use steady clock to calculate number of derive iterations (MarcoFalke)
fa2c099cec1780a498e198750be0cf5bf0ca315a wallet: Use steady clock to measure scanning duration (MarcoFalke)
fa976218044f3ff244abbd797b183a1408375c74 qt: Use steady clock to throttle GUI notifications (MarcoFalke)
fa1d8044abc2cd0f149a2d526b3b03441443cdb0 test: Use steady clock in index tests (MarcoFalke)
fa454dcb20b9e7943cc25e6eeea72912b5f1c7b5 net: Use steady clock in InterruptibleRecv (MarcoFalke)
Pull request description:
`GetTimeMillis` has multiple issues:
* It doesn't denote the underlying clock type
* It isn't type-safe
* It is used incorrectly in places that should use a steady clock
Fix all issues here.
ACKs for top commit:
willcl-ark:
ACK fa83fb3161
martinus:
Code review ACK fa83fb3161, also ran all tests. All usages of the steady_clock are just for duration measurements, so the change to a different epoch is ok.
Tree-SHA512: 5ec4fede8c7f97e2e08863c011856e8304f16ba30a68fdeb42f96a50a04961092cbe46ccf9ea6ac99ff5203c09f9e0924eb483eb38d7df0759addc85116c8a9f
Avoid use of the expensive mod operator (%) when calculating the
buffer offset. No functional difference.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
