💬 l0rinc commented on pull request "refactor: Avoid copies by using const references or by move-construction":
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2823759748)
ACK fa19d8bee350fef088f5fabca38f09610fa8d20d
The rebase had to fix a `Proxy` merge conflict but otherwise is a clean cherry-pick of the previous `readability-avoid-const-params-in-decls` commit.
Q: Any reason for not including these `const` removals in the last commit?
* `interfaces::BlockTemplate::waitNext`
* `node::GetMinimumTime`
* `blockToJSON`
* ` blockheaderToJSON`
* `GetTarget`
* `wallet::CreateDescriptor`
* `wallet::CWallet::ListAddrBookFunc`
* `DeriveTarget
...
(https://github.com/bitcoin/bitcoin/pull/31650#issuecomment-2823759748)
ACK fa19d8bee350fef088f5fabca38f09610fa8d20d
The rebase had to fix a `Proxy` merge conflict but otherwise is a clean cherry-pick of the previous `readability-avoid-const-params-in-decls` commit.
Q: Any reason for not including these `const` removals in the last commit?
* `interfaces::BlockTemplate::waitNext`
* `node::GetMinimumTime`
* `blockToJSON`
* ` blockheaderToJSON`
* `GetTarget`
* `wallet::CreateDescriptor`
* `wallet::CWallet::ListAddrBookFunc`
* `DeriveTarget
...
💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942)
Specficially → Specifically
Also, `bitcoin*d*-wallet` seems odd.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2055703942)
Specficially → Specifically
Also, `bitcoin*d*-wallet` seems odd.
💬 l0rinc commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055708651)
I think the opposite is true, this way we're forcing the reader to consider the error case when trying to understand the happy path (i.e. separation of concerns). But I'll leave it, it's not *that* important.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2055708651)
I think the opposite is true, this way we're forcing the reader to consider the error case when trying to understand the happy path (i.e. separation of concerns). But I'll leave it, it's not *that* important.
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055699711)
In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055699711)
In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055701332)
In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055701332)
In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055708434)
In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn't mention what they are, or explain why we aren't fixing the code?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055708434)
In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn't mention what they are, or explain why we aren't fixing the code?
💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055703963)
In ce950ec20fa0d246d2fd44f562a864d3ff83d399 "Fix quote issue on Windows ": This isn't in a Windows block, so I'm guessing it doesn't have any effect on other OS's? Is this alos going to be upstreamed?
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2055703963)
In ce950ec20fa0d246d2fd44f562a864d3ff83d399 "Fix quote issue on Windows ": This isn't in a Windows block, so I'm guessing it doesn't have any effect on other OS's? Is this alos going to be upstreamed?
💬 0xB10C commented on issue "ci: failure in interface_usdt_coinselection.py":
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2823781979)
Seems like a one-off event for now and not much that can be done about it?
(https://github.com/bitcoin/bitcoin/issues/32322#issuecomment-2823781979)
Seems like a one-off event for now and not much that can be done about it?
💬 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.