💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055750894)
I must be missing something fundamental here, I don't understand why these lines are never triggered while running the test.
Are these triggered in some special circumstance that has happened before but isn't happening anymore - and since it's already testing other tests, there's no point in covering it with actual tests?
I find it a bit confusing, wanted to understand why e.g. the `nocleanup` branch was added here - but disabling it still passes, so that doesn't help...
```python
if se
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055750894)
I must be missing something fundamental here, I don't understand why these lines are never triggered while running the test.
Are these triggered in some special circumstance that has happened before but isn't happening anymore - and since it's already testing other tests, there's no point in covering it with actual tests?
I find it a bit confusing, wanted to understand why e.g. the `nocleanup` branch was added here - but disabling it still passes, so that doesn't help...
```python
if se
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055753450)
I think 1. and 2. are correct, but in 3. it would happen differently - it would remove the transaction because both txid and wtxid match. `stale_tx` came from the private broadcast storage, is the original transaction, not the malleated one.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055753450)
I think 1. and 2. are correct, but in 3. it would happen differently - it would remove the transaction because both txid and wtxid match. `stale_tx` came from the private broadcast storage, is the original transaction, not the malleated one.
💬 l0rinc commented on pull request "ci: switch to LLVM 20 in tidy job":
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2823865942)
ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6
Since last review `modernize-type-traits` lint rule was removed, and `RemovePrefixView` was also updated to use `starts_with`.
<details>
<summary>Diff since last review</summary>
```patch
diff --git a/src/.clang-tidy b/src/.clang-tidy
index aa171dc0ac..1cf270833a 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -9,7 +9,6 @@ bugprone-lambda-function-name,
bugprone-unhandled-self-assignment,
misc-unused-using-decls,
misc-no-rec
...
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2823865942)
ACK 08aa7fe2326391e6d633c2da50959754e3e7b8d6
Since last review `modernize-type-traits` lint rule was removed, and `RemovePrefixView` was also updated to use `starts_with`.
<details>
<summary>Diff since last review</summary>
```patch
diff --git a/src/.clang-tidy b/src/.clang-tidy
index aa171dc0ac..1cf270833a 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -9,7 +9,6 @@ bugprone-lambda-function-name,
bugprone-unhandled-self-assignment,
misc-unused-using-decls,
misc-no-rec
...
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055779247)
https://en.cppreference.com/w/cpp/algorithm/clamp states that
> If lo is greater than hi, the behavior is undefined.
(cc: @hodlinator)
Should we add an explicit `assert` somewhere that this isn't the case? Though it's implied currently from the tests...
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055779247)
https://en.cppreference.com/w/cpp/algorithm/clamp states that
> If lo is greater than hi, the behavior is undefined.
(cc: @hodlinator)
Should we add an explicit `assert` somewhere that this isn't the case? Though it's implied currently from the tests...
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801774)
Will do if I repush.
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801774)
Will do if I repush.
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801848)
you mean we usually ignore the folder when sorting? I found examples for all sorts of includes (pun, intended) - I'll adjust if I have to push again.
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055801848)
you mean we usually ignore the folder when sorting? I found examples for all sorts of includes (pun, intended) - I'll adjust if I have to push again.
💬 l0rinc commented on pull request "[IBD] flush UTXO set in batches proportional to `dbcache` size":
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055802001)
> Capped at 256 MiB, as gains are barely measurable for bigger batches (see PR 31645)
I don't mind adding, but a simple blame would immediately reveal that
> unneeded braces line 18
They may be implied, but I want to emphasize that mathematically this isn't associative, i.e. not the same as
```C++
dbcache_bytes / (DEFAULT_KERNEL_CACHE * DEFAULT_DB_CACHE_BATCH)
```
(https://github.com/bitcoin/bitcoin/pull/31645#discussion_r2055802001)
> Capped at 256 MiB, as gains are barely measurable for bigger batches (see PR 31645)
I don't mind adding, but a simple blame would immediately reveal that
> unneeded braces line 18
They may be implied, but I want to emphasize that mathematically this isn't associative, i.e. not the same as
```C++
dbcache_bytes / (DEFAULT_KERNEL_CACHE * DEFAULT_DB_CACHE_BATCH)
```
📝 juanmigdr opened a pull request: "Add rpcestimateconservativefees"
(https://github.com/bitcoin/bitcoin/pull/32329)
### Summary
This PR introduces a new configuration option: `-rpcestimateconservativefees`. When enabled, it forces the default behavior of the `estimatesmartfee` RPC to use **conservative mode** fee estimation.
### Motivation
Bitcoin Core v28.0 changed the default fee estimation mode for `estimatesmartfee` from **conservative** to **economical** ([[#30275](https://github.com/bitcoin/bitcoin/pull/30275)](https://github.com/bitcoin/bitcoin/pull/30275)). While this change reduces fee over-
...
(https://github.com/bitcoin/bitcoin/pull/32329)
### Summary
This PR introduces a new configuration option: `-rpcestimateconservativefees`. When enabled, it forces the default behavior of the `estimatesmartfee` RPC to use **conservative mode** fee estimation.
### Motivation
Bitcoin Core v28.0 changed the default fee estimation mode for `estimatesmartfee` from **conservative** to **economical** ([[#30275](https://github.com/bitcoin/bitcoin/pull/30275)](https://github.com/bitcoin/bitcoin/pull/30275)). While this change reduces fee over-
...
💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2055789608)
This is fine but seeing `pushdata1` in `tutorial/nonminpushdata1` later in the spender comment seems unnecessary then.
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2055789608)
This is fine but seeing `pushdata1` in `tutorial/nonminpushdata1` later in the spender comment seems unnecessary then.
💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2055856082)
I don't have a firm grasp on script execution yet and there are few things going on inside the "test_spenders" as well that I have not gone through in its entirety. Are following the ways these 3 tapscript scenarios executed?
1. During script execution, the empty string witness data is dropped because of "OP_DROP" and the 2 byte data is left on the stack. I'm assuming this 2 byte data counts as 1 stack item (because the stack is a vector of vector) and that's why script execution doesn't lead
...
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2055856082)
I don't have a firm grasp on script execution yet and there are few things going on inside the "test_spenders" as well that I have not gone through in its entirety. Are following the ways these 3 tapscript scenarios executed?
1. During script execution, the empty string witness data is dropped because of "OP_DROP" and the 2 byte data is left on the stack. I'm assuming this 2 byte data counts as 1 stack item (because the stack is a vector of vector) and that's why script execution doesn't lead
...
💬 Sjors commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824016734)
This PR renamed UndoReadFromDisk to ReadBlockUndo. Hopefully this comment makes this easier to find.
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824016734)
This PR renamed UndoReadFromDisk to ReadBlockUndo. Hopefully this comment makes this easier to find.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055884036)
Another objection to your approach as a general rule:
If one of the lines in the happy path is modified to call a function that raises exceptions under some circumstances, we run the risk of accidentally catching it by related (parent)type through a pre-existing `except`/`catch`-block, and treating it differently than we had if we intentionally designed the code.
Side note: I'm in favor of newer languages like Rust/Zig **not** supporting exceptions.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055884036)
Another objection to your approach as a general rule:
If one of the lines in the happy path is modified to call a function that raises exceptions under some circumstances, we run the risk of accidentally catching it by related (parent)type through a pre-existing `except`/`catch`-block, and treating it differently than we had if we intentionally designed the code.
Side note: I'm in favor of newer languages like Rust/Zig **not** supporting exceptions.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055895552)
The forwarding of `self.options` to the child process is purely to obey the expectations of someone running feature_framework_startup_failures.py or test_runner.py with those arguments on the CLI/CI.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055895552)
The forwarding of `self.options` to the child process is purely to obey the expectations of someone running feature_framework_startup_failures.py or test_runner.py with those arguments on the CLI/CI.
💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824055131)
yes, it was a leftover from the renames, suggested in https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2509562827
Did the rename cause any problems?
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2824055131)
yes, it was a leftover from the renames, suggested in https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2509562827
Did the rename cause any problems?
💬 hebasto commented on issue "PIE+LTO causes Bitcoin-Qt to segfault at startup":
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824064676)
Is anyone aware of other distros that provide shared Qt libraries built with `-reduce-relocations`?
(asking because I'm not quite familiar with Gentoo)
Fedora, Debian/Ubuntu, and Arch do not have such an issue.
(https://github.com/bitcoin-core/gui/issues/867#issuecomment-2824064676)
Is anyone aware of other distros that provide shared Qt libraries built with `-reduce-relocations`?
(asking because I'm not quite familiar with Gentoo)
Fedora, Debian/Ubuntu, and Arch do not have such an issue.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055902833)
Not sure I understand, so is it dead code? Would the above exceptions be triggered on CI? In either case I find it confusing.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055902833)
Not sure I understand, so is it dead code? Would the above exceptions be triggered on CI? In either case I find it confusing.
💬 hodlinator commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2824067436)
Happened again here:
https://github.com/hodlinator/bitcoin/actions/runs/14614533923/job/40999660639#step:6:5096
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2824067436)
Happened again here:
https://github.com/hodlinator/bitcoin/actions/runs/14614533923/job/40999660639#step:6:5096
💬 ismaelsadeeq commented on pull request "Add rpcestimateconservativefees":
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824083941)
> Bitcoin Core v28.0 changed the default fee estimation mode for `estimatesmartfee` from *conservative* to *economical* ([[#30275](https://github.com/bitcoin/bitcoin/pull/30275)](https://github.com/bitcoin/bitcoin/pull/30275)). While this change reduces fee overestimation for most users, it may not be ideal in all environments.
You can easily opt in to the `conservative` mode by passing a parameter to `estimatesmartfee`.
> In my case, I maintain a long-running node (since v24.0) with m
...
(https://github.com/bitcoin/bitcoin/pull/32329#issuecomment-2824083941)
> Bitcoin Core v28.0 changed the default fee estimation mode for `estimatesmartfee` from *conservative* to *economical* ([[#30275](https://github.com/bitcoin/bitcoin/pull/30275)](https://github.com/bitcoin/bitcoin/pull/30275)). While this change reduces fee overestimation for most users, it may not be ideal in all environments.
You can easily opt in to the `conservative` mode by passing a parameter to `estimatesmartfee`.
> In my case, I maintain a long-running node (since v24.0) with m
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055915118)
> constructing a malleated transaction wasn't as straightforward as I thought
There is `malleate_tx()` Python function in the tests. It is used by the newly added `p2p_private_broadcast.py`. It creates an invalid witness. Did you create a valid one? If yes, then maybe you want to contribute to that function, to make it possible to create valid or invalid.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2055915118)
> constructing a malleated transaction wasn't as straightforward as I thought
There is `malleate_tx()` Python function in the tests. It is used by the newly added `p2p_private_broadcast.py`. It creates an invalid witness. Did you create a valid one? If yes, then maybe you want to contribute to that function, to make it possible to create valid or invalid.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055920502)
It is sometimes useful to debug tests by looking at what is left behind on the filesystem. In those cases one could invoke the parent with "--nocleanup". Not forwarding it to child processes means they would not leave files behind.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055920502)
It is sometimes useful to debug tests by looking at what is left behind on the filesystem. In those cases one could invoke the parent with "--nocleanup". Not forwarding it to child processes means they would not leave files behind.