1d1ae6f0c4 wallet, test: Remove DuplicateMockDatabase (Ava Chow)
57820c472b bench: Utilize setup() for WalletLoading and use a real database (Ava Chow)
9a7604fd25 bench: Use setup() in WalletMigration to prepare the legacy wallet (Ava Chow)
426a94e7bd bench: Utilize setup() in WalletEncrypt to create the encryption wallet (Ava Chow)
d672455d20 bench: Utilitze setup() in WalletBalance for marking caches dirty (Ava Chow)
61412ef887 bench: Utilize setup() in WalletCreate to cleanup previous wallets (Ava Chow)
Pull request description:
Several of the wallet benchmarks have some setup or cleanup that needs to be done per run. Now that #34208 is merged, these can use `setup()`. Additionally, this allows for removing `DuplicateMockDatabase` in `WalletEncryptDescriptors`.
This PR also removes `DuplicateMockDatabase` in `WalletLoadingDescriptors`. `DuplicateMockDatabase` was added here in #24924 as part of benchmark performance improvements. However, it does not appear to make a significant difference today.
Removing `DuplicateMockDatabase` makes future database changes easier. In particular it should simplify #33032 and #33034, and any future changes that introduce sqlite features.
ACKs for top commit:
l0rinc:
code review ACK 1d1ae6f0c4
furszy:
Other than that, ACK 1d1ae6f0c4
sedited:
ACK 1d1ae6f0c4
Tree-SHA512: 41130144972b759b401f990820eaf524d1f17f47d81bf1afea4a529d15a21d253521838a9e31df8f424996582b718a92634ab255204c6fce703b7e47a1d23670
758f208cc1 contrib: override system locale in gen-manpages.py (Sjors Provoost)
Pull request description:
`bitcoin-qt --help` emits a translation of "version", which creates a diff when updating or verifying man pages.
The script aborts earlier however, because `bitcoin-qt --version` also emits a localized output, which triggers the `Copyright (C)` assertion on a translated term like "Auteursrecht".
Fix this by passing `--lang=en` to both `bitcoin-qt` invocations.
None of the actual command options are translated, so this commit does not affect the actual manual page.
Noticed while verifying the manual updates in #34800 on macOS with Dutch system locale.
See also https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-locale-dependence.py notes about localization (though the issue here is translation).
ACKs for top commit:
achow101:
ACK 758f208cc1
sedited:
ACK 758f208cc1
hebasto:
ACK 758f208cc1.
Tree-SHA512: 0947391b85a637ae6d1d0cf4b8de4ab74b42e772bf7d70991cf2b4e035c2cb99f0f46c6cb2cd8aac9cc1be22a9e8329594536688622d3bacab22289c8151e89d
938312d7a6 docs: clarify RPC credentials security boundary (crStiv)
Pull request description:
Explicitly states that RPC credentials grant full administrative access to the node and filesystem resources accessible by bitcoind. Adds a new section in JSON-RPC-interface.md to address issue https://github.com/bitcoin/bitcoin/issues/32274 by documenting that providing RPC credentials to untrusted clients
reopened#32424
P.S. I've tried to somehow squash all the commits from the previous pr but accidentally closed the pr and had no idea how to return back, therefore created a new pr, I'm really sorry for the inconvenience
ACKs for top commit:
achow101:
ACK 938312d7a6
janb84:
re ACK 938312d7a6
sedited:
ACK 938312d7a6
Tree-SHA512: 54db0651cfe4a92d700d09c822db5cb68f60f17a4634eb8f132939294e7a0ca2aea15ddc4d581610976158f7546e9c4463cfe113de9500162a0f107e168833cd
2104282ddd fuzz: Add tests for CCoinControl methods (Chandra Pratap)
43b09b993d fuzz: Improve oracle for existing CCoinControl tests (Chandra Pratap)
Pull request description:
The `ccoincontrol` fuzzer misses tests for a number of `CCoinControl` operations. Add them.
While at it, improve the oracle for the existing tests.
ACKs for top commit:
l0rinc:
Lightly tested code review ACK 2104282ddd
brunoerg:
reACK 2104282ddd
sedited:
Re-ACK 2104282ddd
Tree-SHA512: bfc8c9a51fca94437332056c476840d841a5b42dd6749cb34105b7ae78215ec9c3eb0f407e1a5f51b3ac20d7abb97cae7c21ad2146d5be9409edbc2cd2c568ee
1950da94fc test: enable `rpc_bind` on macOS and BSD (Lőrinc)
7236a05503 test: enable `feature_bind_extra` on macOS and BSD (Lőrinc)
Pull request description:
### Problem
Some functional tests are shown as skipped when running on macOS & BSD because `test_framework/netutil.py` only implemented the Linux-specific logic for checking which TCP sockets a node is listening on.
### Fix
Add macOS and BSD implementations in `test/functional/test_framework/netutil.py` so tests can query:
* which TCP sockets a node is listening on (`get_bind_addrs()`, via `lsof`)
* a non-loopback interface address (`all_interfaces()`, via `ifconfig`)
Then enable the previously Linux-only tests by switching to a shared POSIX platform guard.
### Commands
<details>
<summary><code>get_bind_addrs()</code> (<code>lsof</code> + regex)</summary>
> Command used
```bash
lsof -nP -a -p <pid> -iTCP -sTCP:LISTEN -Ftn
```
> Flags
- -D: device cache warnings
- -n: no hostname resolution
- -P: no service/port-name resolution
- -a: AND all conditions
- -p <pid>: filter by process ID
- -iTCP: TCP sockets only
- -sTCP:LISTEN: listening sockets only
- -Ftn: machine-readable output (fields: type `t`, name `n`)
> Regex parser
```regex
t(IPv[46])\nn(\*|\[.+?]|[^:]+):(\d+)
```
> Captured groups
- group 1: IPv4 / IPv6 (used to disambiguate `*`)
- group 2: host (`*`, `[::1]`, `127.0.0.1`, ...)
- group 3: port
</details>
<details>
<summary><code>all_interfaces()</code> (<code>ifconfig</code> + regex)</summary>
> Command used
```bash
ifconfig -au
```
> Regex parsing
Interface blocks:
```regex
(?m)^(?P<iface>\S+):(?P<block>[^\n]*(?:\n[ \t]+[^\n]*)*)
```
IPv4 extraction within each block:
```regex
inet (\S+)
```
</details>
### Notes
The only remaining platform skips on macOS are the USDT/BPF tracing tests (`interface_usdt_*.py`).
ACKs for top commit:
Sjors:
ACK 1950da94fc
achow101:
ACK 1950da94fc
willcl-ark:
tACK 1950da94fc
Tree-SHA512: 4cecc88852623f3fe3a7dccceb0e71932824c1ed7f1d4ab89b953ff6b7991afbd0b016c819c17e966bed53082dd623a832752b8847711861009cd5ffc4677367
1aa78cdab6 clusterlin: adopt STL ranges algorithms (refactor) (Pieter Wuille)
747da25360 feefrac: drop comparison and operator{<<,>>} for sorted wrappers (Pieter Wuille)
Pull request description:
Instead of having an unintuitive but strong implicit sort order on `FeeFrac` (first increasing feerate, then decreasing size), and separate overloaded `operator<<` and `operator>>` that implement a weak ordering that only looks at feerate, replace these with explicit wrapper classes which make the behavior more explicit (`ByRatio` and `ByRatioNegSize`).
This allows for things like `ByRatio{a} <= ByRatio{b}`, instead of the earlier `!(a >> b)`. It also supports usage inside `std::min`/`std::max`/`std::less`, and `std::greater`, so one can use:
* `std::max<ByRatioNegSize<FeeFrac>>(a, b)` to get the highest-feerate `FeeFrac`, tie-breaking by smallest size.
* `std::ranges::sort(v, std::greater<ByRatioNegSize<FeeFrac>>{});` to sort a vector that way.
Suggested in https://github.com/bitcoin/bitcoin/pull/34257#discussion_r2780475893.
ACKs for top commit:
achow101:
ACK 1aa78cdab6
sedited:
ACK 1aa78cdab6
ajtowns:
ACK 1aa78cdab6
Tree-SHA512: d76657b15f6d745e5ca01c67fd5b101fdc418e6301646d14e575b6564bfa2fe0eb40a95a7ff95a4420624ef6b67224d35e4713aa5bbc0d293e08fe44c0cc6db0
08925d5ee7 test: add coverage for loading a wallet in a non-writable directory (furszy)
0218966c0d test: add coverage for wallet creation in non-writable directory (furszy)
bc0090f1d6 wallet: handle non-writable db directories (furszy)
Pull request description:
Make wallet creation and load fail with a clear error when the db directory isn’t writable.
#### 1) For Wallet Creation
Before: creating a wallet would return a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"
After: creating a wallet returns:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory is not writable"
#### 2) For Wallet Loading
We currently allow loading wallets located on non-writable directories. This is problematic
because the node crashes on any subsequent write; generating a block is enough to trigger it.
Can be verified just by running the following test on master: 85fa4e2910
Also, to check directory writability, this creates a tmp file rather than relying on the
`permissions()` functions, since perms bits alone may not reliably reflect actual writability
in some systems.
Testing Note:
Pushed the tests in separate commits so they can be cherry-picked on master for comparison.
ACKs for top commit:
rkrux:
re-ACK 08925d5ee7
achow101:
ACK 08925d5ee7
seduless:
Tested ACK 08925d5ee7
Tree-SHA512: e480eab329a1d595fe0b191e83c97956e3ff1d1e335ada8ac6fe72bc4b2bb9b13b0d49db0254d34ad75f816db06d9cd0c21d3063d7d8ee6687a7ea2324c36288
DuplicateMockDatabase is no longer used. Furthermore, as SQLite gets
used more as a database and less as a key value store, this function
gets more complicated and more bug prone. As the benchmarks now run
equivalently quickly with a real database, retaining this duplication
function is no longer necessary.
Instead of making a mock database and duplicating it for the benchmark,
use a real database. Also use setup() to avoid measuring the overhead in
the benchmark.
WalletMigration needs a new wallet with legacy records for each run of
the benchmark. This can be done in setup() rather than duplicating the
records of an initial wallet.
WalletEncrypt needs an unencrypted wallet in order for the benchmark to
encrypt a wallet. This was previously achieved by duplicating the
contents of an initial wallet for each run of the benchmark. We can
instead use setup() to unload the previously loaded wallet and then
create a new wallet with unencrypted keys.
WalletBalance benchmarks the balance computation function and should
exclude the setup step of (optionally) marking caches as dirty. Instead,
that is moved into setup().
The WalletCreate benchmark should only be for creating a wallet and
exclude the unloading of the newly created wallet. Instead, unloading
can be done in setup() and after the benchmark completes.
0bc9d354df multi_index: fix compilation failure with boost >= 1.91 (Cory Fields)
Pull request description:
This effectively reverts a3cb309e7c from PR #30194.
That PR reduced the `multi_index` type signatures as recommended upstream, but this is no longer supported as of boost 1.91 because it is no longer necessary. 1.91 drops support for the pre-c++11 work-arounds that bloated the type signatures to begin with.
The upstream `BOOST_MULTI_INDEX_ENABLE_MPL_SUPPORT` define is meant to provide compatibility with removed features, but it does not work for this case. Using `indexed_by` directly when defining the `multi_index` (as opposed to inheriting from it) works with all versions, and avoids the use of the back-compat define.
This is a slight regression when building against boost < 1.91 because the bloated type signatures are reintroduced in that case, but it's not significant enough to go to the trouble of introducing version detection and ifdefs.
ACKs for top commit:
maflcko:
review ACK 0bc9d354df🎶
hebasto:
ACK 0bc9d354df.
w0xlt:
ACK 0bc9d354df
Tree-SHA512: 883ee998efd16d944628653ca204e3d2acaf2554b2eced40556143a66d6072a3625861d961d1a4a194a7b8d4d448562581e5d11a09380754a5635a871d2a0aa1
eab72d14d7 refactor: use SignOptions for MutableTransactionSignatureCreator (Sjors Provoost)
5ed41752c5 refactor: use SignOptions for SignTransaction (Sjors Provoost)
dc4a5d1270 refactor: use PSBTFillOptions for filling and signing (Sjors Provoost)
Pull request description:
Replace the `sign`, `finalize` , `bip32derivs` and `sighash_type` arguments that are passed to `FillPSBT()` and `SignPSBTInput()` with a `PSBTFillOptions` struct.
```cpp
struct PSBTFillOptions {
bool sign{true};
std::optional<int> sighash_type{std::nullopt};
bool finalize{true};
bool bip32_derivs{true};
};
```
Additionally this PR introduces:
```cpp
struct SignOptions {
int sighash_type{SIGHASH_DEFAULT};
};
```
This is used by `SignTransaction` and the `MutableTransactionSignatureCreator` constructor.
These changes make it easier to add additional options later without large code churn, such as `avoid_script_path` proposed in #32857. It also makes the use of default boolean options safer compared to positional arguments that can easily get mixed up.
ACKs for top commit:
w0xlt:
reACK eab72d14d7
optout21:
ACK eab72d14d7
sedited:
ACK eab72d14d7
Tree-SHA512: 097e3d042e794c9f47d03e1aafad184a4525aa765d274f6497122d4f41603e902191df6fbf9ce846dbcd7372a159b67e2234da7341ec6a6776be5685e3e6e6ff
This effectively reverts a3cb309e7c from PR #30194.
That PR reduced the multi_index type signatures as recommended upstream, but
this is no longer supported as of boost 1.91 because it is no longer necessary.
1.91 drops support for the pre-c++11 work-arounds that bloated the type
signatures to begin with.
The upstream `BOOST_MULTI_INDEX_ENABLE_MPL_SUPPORT` define is meant to provide
compatibility with removed features, but it does not work for this case. Using
`indexed_by` directly when defining the `multi_index` (as opposed to inheriting
from it) works with all versions, and avoids the use of the back-compat define.
This is a slight regression when building against boost < 1.91 because the
bloated type signatures are reintroduced in that case, but it's not significant
enough to go to the trouble of introducing version detection and ifdefs.
Replace the sign, finalize , bip32derivs and sighash_type arguments which
are passed to FillPSBT() and SignPSBTInput() with a PSBTFillOptions struct.
This makes it easier to add additional options later without large code
churn, such as avoid_script_path proposed in #32857. It also makes the
use of default boolean options safer compared to positional arguments
that can easily get mixed up.
8deed0df06 doc: add release notes for PR 34911 (rkrux)
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses (rkrux)
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response (rkrux)
Pull request description:
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently.
The getmempoolinfo RPC need not return the deprecated `fullrbf` key in the
response anymore unless the user requests it via -deprecatedrpc=fullrbf node
argument.
Also, remove the `bip125-replaceable` key from the mempool RPCs
responses (because it, too, has been deprecated since v29) unless the user
requests it via -deprecatedrpc=bip125 node argument.
ACKs for top commit:
optout21:
ACK 8deed0df06
sedited:
ACK 8deed0df06
instagibbs:
ACK 8deed0df06
Tree-SHA512: dfbbdf04ae25dafe81758bd85715af80d736e21bb913ab09ff8fd8225587b7a1bf1dd6b0f7ea05135504e11d43371d98e5f5fc6e3717d581e870e8c2bf97811b
a1e534bbf0 doc: clarify clang-tidy in developer notes (rkrux)
Pull request description:
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
ACKs for top commit:
sedited:
ACK a1e534bbf0
Tree-SHA512: e95cd44cdad9a9011c18136ec7f1db4e73f78f11c517f94c14cf128beaaea150881868c27e7c5c710c9cab94d285d61dc5e5278f347c229f5df495cd49c532ab
ef21e29298 doc: update llvm based coverage example (Max Edwards)
Pull request description:
The old example was working on my Mac but when run on Linux I was finding that I had missing coverage information.
On some systems the old example of using xargs to pass list of profraw files can exceed xargs's argument limit resulting in multiple executions of llvm-profdata merge command which overwrites it's output file losing coverage information. This PR switches the example to using a file with a list of filenames.
There are other ways to solve this like using bash's globbing or a one liner with temporary file but I felt like this was the most simple, robust and easy to understand.
ACKs for top commit:
vasild:
ACK ef21e29298
sedited:
ACK ef21e29298
Tree-SHA512: 2726d9b38088c1c0b6f04d4622bdc4ea28e16b13a962a3afc842798aa5e0eee4682752c73707c5580a43bdb9d0d84d542afd357f166e4dbb7f6beb7afc0522c8
14547eb489 kernel: guard btck::Handle move-assignment against self-move (Thomas)
Pull request description:
The move-assignment operator for `btck::Handle<>` in `src/kernel/bitcoinkernel_wrapper.h` unconditionally called `DestroyFunc(m_ptr)` before reading the source pointer. On a self-move (`h = std::move(h)`), this destroys the held resource and then restores the now-dangling pointer via `std::exchange(other.m_ptr, nullptr)` (since `&other == this`), which leaves `m_ptr` pointing at freed memory. The destructor then calls `DestroyFunc` again on it, resulting in a double-free.
Trace of `h = std::move(h)` with the old code, where `h.m_ptr == P`:
1. `DestroyFunc(m_ptr)` -> `delete P`. `this->m_ptr` still literally stores the now-dangling value `P`.
2. `std::exchange(other.m_ptr, nullptr)` — because `&other == this`, this reads the dangling `P` back, writes `nullptr` to `m_ptr`, and returns `P`.
3. `m_ptr = P` restores the dangling pointer.
4. `~Handle()` later runs `DestroyFunc(P)` -> double free, UB.
The copy-assignment operator already guards against self-assignment with `if (this != &other)`; the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.
`Handle<>` is the base class of 16 public types in the kernel C++ API wrapper (`Transaction`, `Block`, `BlockHeader`, `ChainParams`, `Context`, `Coin`, `BlockValidationState`, `ScriptPubkey`, `TransactionOutput`, `Txid`, `OutPoint`, `TransactionInput`, `PrecomputedTransactionData`, `BlockHash`, `BlockSpentOutputs`, `TransactionSpentOutputs`), so self-move can arise from generic algorithms operating on containers of these types (`std::sort`, `std::remove`, erase-remove idioms, etc.).
Fix: mirror the copy-assignment pattern by guarding the move-assignment body with `if (this != &other)`, making a self-move a no-op.
Also extend `CheckHandle` in `src/test/kernel/test_kernel.cpp` to exercise self-move-assignment for every `Handle`-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.
ACKs for top commit:
sedited:
Thanks, ACK 14547eb489
alexanderwiederin:
ACK 14547eb489
yuvicc:
lgtm ACK 14547eb489
Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa
1) For wallet load, this fixes a crash.
We currently allow loading wallets located on non-writable directories.
This is problematic because the node crashes on any subsequent write.
E.g. generating a block is enough to trigger it.
2) For wallet creation, this improves the returned error msg.
Before: creating a wallet would return a generic error:
"SQLiteDatabase: Failed to open database: unable to open database file"
After: creating a wallet returns:
"SQLiteDatabase: Failed to open database in directory <dir_path>: directory
is not writable"
fa204100e1 streams: Remove confusing DataStream::in_avail() (MarcoFalke)
fa5ab0220e move-only: Extract ProcessPong() helper (MarcoFalke)
Pull request description:
The `in_avail` alias is confusing (see commit message), so remove it. Also extract the `ProcessPong()` helper, as the only place that called `in_avail`.
ACKs for top commit:
l0rinc:
code review ACK fa204100e1
sedited:
ACK fa204100e1
Tree-SHA512: b5a069abb471c42d39e91a3c55fc7f18fb9ccf8591408b3dd43210d0287785369b6a5d6b59b847d85247a494f208dbbe4a7e88b24caf9b3e6c8fea2157cd0ee0
2a90b6132a Add release notes for exportasmap (Fabian Jahr)
8cb2d926b4 rpc: Add exportasmap RPC (Fabian Jahr)
Pull request description:
This depends on the embedded data PR itself (#28792), until merge this will remain in draft status. All commit but the last one are from there.
There has been interest in exporting the embedded data back into a file. This is implemented here with a simple `exportasmap` RPC which provides this functionality. The exported file can be used to verify the data integrity or statistical analysis using e.g. `contrib/asmap-tool.py`.
ACKs for top commit:
achow101:
ACK 2a90b6132a
sedited:
ACK 2a90b6132a
seduless:
Tested ACK 2a90b6132a
Tree-SHA512: c8453078efe916ed95bff0695aabd9123d805970e037fad5e9317f4d43e2a4b21970030265bc82b00f3a222ee3db11346eef1fb6fc9393429b9bc6449f1830f1
fa43da21f1 refactor: Run ShouldWarnOversizedDbCache calculation in u64 (MarcoFalke)
fa5801762e util: Return uint64_t from _MiB and _GiB operators (MarcoFalke)
fa9ddb01c9 test: Use MiB operator directly in cuckoocache_tests (MarcoFalke)
Pull request description:
Currently, the `_MiB` literal operator returns a value of type `size_t`. This is brittle and confusing:
* It is inherently impossible to represent larger values, like storage device usage.
* Similarly, it is not possible to even type an upper cap on the memory, see the failure in https://github.com/bitcoin/bitcoin/pull/34692#issuecomment-3973281952
* Using `size_t` isn't required here. The function is evaluated at compile time, and even 32-bit compilers can evaluate an `uint64_t` at compile time.
* Using `size_t` here encourages it to be used in more places, which will likely lead to more bugs and CVEs, such as https://github.com/bitcoin/bitcoin/pull/34109, https://github.com/bitcoin/bitcoin/pull/33724, etc.
Fix all issues, by:
* Marking the operator `consteval`, to ensure it is really only called at compile-time.
* Returning an `uint64_t` value.
* Using it in the place where it was previously not possible.
Review note:
This should have no downside, because the C++11 narrowing checks continue to work as expected. For example, typing `uint8_t{1_MiB};` will continue to fail with the correct error message `error: constant expression evaluates to 1048576 which cannot be narrowed to type 'uint8_t' (aka 'unsigned char') [-Wc++11-narrowing]` (like it does on current master).
ACKs for top commit:
l0rinc:
ACK fa43da21f1
achow101:
ACK fa43da21f1
hebasto:
ACK fa43da21f1, I have reviewed the code and it looks OK.
hodlinator:
ACK fa43da21f1
Tree-SHA512: e19f6bd18c519c2a56d44fbc9e65b02e630bc71170f02a8a8955bf5c9bb2924b55ec2f810de2bbc961162aa668b7383c9fe9b5261fa1de7d9e8413003c32fa87
On some systems the old example of using xargs to pass list of profraw
files can exceed xargs's argument limit resulting in multiple executions
of llvm-profdata merge command which overwrites it's output file losing
coverage information. Switch to using a file with a list of filenames.
Recently, I ran clang-tidy in my system and had to figure out setting it up
correcty on my system (macOS) & understanding its output while tweaking
few arguments that made the output easier to parse.
This patch documents those in the developer notes.
The alias of the size() method is confusing, because:
* It claims to be part of the Bitcoin Core stream subset (streams
interface), but this is not used by any other stream interface. Mostly
the `write(std::span)` and `read(std::span)` define the stream
interface.
* It casts the size_t to i32, but the only place that calls the function
casts that back to size_t.
* Providing this alias for size() without a proper reason is confusing.
Fix all issues by removing it and using the size() method.
RPCs: getrawmempool, getmempoolancestors, getmempooldescendants, getmempoolentry
This key has been marked deprecated since v29, it can be
removed from the RPC response now unless the user requests it via the
-deprecatedrpc=bip125 node argument.
mempoolfullrbf=1 behaviour has been the default since v28 and the argument has
been removed since v29 subsequently. The getmempoolinfo RPC need not return
the deprecated `fullrbf` key in the response anymore unless the user requests
it via -deprecatedrpc=fullrbf node argument.
e6430b2773 bench: make `setup()` use single-iteration epochs (Lőrinc)
ba0078e3bf bench: fix ephemeral spend inputs (Lőrinc)
b8b7f896e8 bench: drop duplicate balance benchmark (Lőrinc)
Pull request description:
### Context
Found while reviewing https://github.com/bitcoin/bitcoin/pull/35018, which led to a few additional benchmark fixes.
### Problem
`MempoolCheckEphemeralSpends` populated every child input by writing to `vin[0]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
`WalletBalanceMine` duplicated `WalletBalanceClean` exactly.
The same review also showed that nanobench `setup()` can be misused as though it runs once per timed call.
It actually runs once per epoch, so using it with multiple epoch iterations can silently leave later iterations with different preconditions.
### Fix
Write each ephemeral-spend prevout to the matching child input and assert that the last input spends the last parent output.
Remove the duplicate wallet balance benchmark registration.
Add a nanobench assertion that `setup()` is only used with `epochIterations(1)`.
> [!NOTE]
> This PR included a few benchmark adjustments originally which made the tests unstable and were reverted.
ACKs for top commit:
davidgumberg:
crACK e6430b2773
sedited:
ACK e6430b2773
Tree-SHA512: 667c67975d7a3e21333f6c8a4e037ad700bc6dcbd7039e002534e4f2a9328195a618b78accf097e7c43402a1f1b25e32b0c39642fd465729ab4c337abe4f23f1
df44afdc98 kernel: expose btck_block_tree_entry_get_ancestor (Peter Zafonte)
Pull request description:
Callers building a block locator from a stale tip currently have to walk back one entry at a time using btck_block_tree_entry_get_previous. For large step sizes this means hundreds of C API calls per locator entry.
This exposes GetAncestor via btck_block_tree_entry_get_ancestor, which uses the existing CBlockIndex skiplist to reach any ancestor height in O(log N) and works on any chain, not just the active chain.
Includes a C++ convenience wrapper and tests ~~covering same-height, genesis, and out-of-range cases~~ in btck_block_tree_entry_tests.
See previous discussion here #34843
ACKs for top commit:
alexanderwiederin:
ACK df44afdc98
sedited:
ACK df44afdc98
Tree-SHA512: e953b634235129fa972e7f148016e8aee7bad61e16d50191d6724d11c71fe0f7623152ff61aa4c4ec5c8a5b500aaeb36613b5b2ff1e404d01ed7b65aa861f7aa
`setup()` in nanobench runs once per epoch, not once per timed call.
If an epoch executes the benchmark body multiple times, `setup()` can silently leave later iterations with different preconditions.
Make `setup()` force `epochIterations(1)` itself: keep rejecting incompatible larger explicit epoch sizes, but allow existing single-iteration callers such as `-sanity-check`.
With `setup()` handling this centrally, remove the redundant `epochIterations(1)` calls from the benchmarks that use it.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
`MempoolCheckEphemeralSpends` wrote every prevout to `tx2.vin[0]` instead of `tx2.vin[i]`.
That left only one child input pointing at the parent transaction, while the remaining inputs kept default prevouts.
Write each prevout to `vin[i]` instead.
Add an assertion that the last child input spends the last parent output.
Co-authored-by: David Gumberg <davidzgumberg@gmail.com>
3bf3b6d59a net: log SOCKS5 auth before sending (takeshikurosawaa)
b2debc9276 net: cleanup SOCKS5 auth logging (takeshikurosawaa)
Pull request description:
`Socks5()` logs the supplied `username` and `password` in `BCLog::PROXY`
when the server selects `USER_PASS`.
Keep the log entry for the auth path, but omit the credentials.
Log the message before sending the authentication data.
No functional behavior change intended.
ACKs for top commit:
davidgumberg:
crACK 3bf3b6d59a
theStack:
ACK 3bf3b6d59a
sedited:
ACK 3bf3b6d59a
Tree-SHA512: ef90fca297b954c8659d108259b7f25216c0c0ddb7305539154f14a7bfdad807b70b7ddd0cf6e4ad0127d0a1ccfdfbb3f4785d7662b134409375ff1830b9df9b
Previously, they were using a pattern of defininig a constant megabytes
symbol, and then multiplying that by 1_MiB.
It is easier to use the _MiB operator directly.
The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.
Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.
Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.
Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
c5ec2d5313 logging: replace FormatLogStrInPlace with Format (stickies-v)
3b92ec2036 logging: replace BufferedLog with log::Entry (stickies-v)
8115001cd4 logging: pass log::Entry through to logging functions (stickies-v)
b414913c73 util: add timestamp and thread_name to log::Entry (stickies-v)
8a55b17751 util: make SourceLocation constructor explicit (stickies-v)
Pull request description:
Preparatory work to enable struct-based logging for kernel (#34374), but I think it's a mild improvement by itself too.
#34465 introduced `util::log::Entry`. This PR updates some internal functions and structs to make better use of that new plumbing.
Mostly a refactor, except for very minor timing differences for `timestamp` and `mocktime` which are now uniformly captured at `Entry` generation.
ACKs for top commit:
maflcko:
review ACK c5ec2d5313🚐
ryanofsky:
Code review ACK c5ec2d5313. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](https://github.com/bitcoin/bitcoin/pull/34865#discussion_r3054379230) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options.
sedited:
ACK c5ec2d5313
Tree-SHA512: 3943f380a426b2c1b405226f231db51548c737467a278936b36a4cc738e01a790258f0886817d7caa1dbba7874f2e7f9ad93c36a137fa35f721f2d988b9863aa
