🤔 stickies-v reviewed a pull request: "log: Use more severe log level (warn/err) where appropriate"
(https://github.com/bitcoin/bitcoin/pull/33960#pullrequestreview-3518589743)
Concept ACK

I think `LogError` should be rare in utility/helper functions, and reserved for the places where the actual shutdown is initiated or inevitable (e.g. right before a `assert`, `std::abort`, `Shutdown()`, ...).

We shouldn't have to inspect a function's callers in order to determine whether `LogError` is appropriate within that function. In fa0018d01102ad1d358eee20d8bae1e438ceebf8, what do you think about replacing `LogError` with `LogWarning` whenever we're not actually shutting
...

💬 stickies-v commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2571347832)
Seems to me like these should be `LogWarning`, a flush failing doesn't inherently mean the node or subsystem must be shut down?

💬 maflcko commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2571478747)
I think there will always be a possibility to argue one way or the other, when not all code paths lead to an immediate crash/shutdown. However, conceptually, even if there was a code path that would not result in a crash/shutdown on a failing `FileCommit`, it seems like it should propagate the error up as a fatal one.

Note that `flushError` results in an `AbortNode`, just like `fatalError`.

I understand that a flush failure in addrdb.cpp or mempool_persist.cpp does not lead to a fatal erro
...

👍 theStack approved a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3518745387)
Code-review ACK 53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e

I vaguely remember having seen the "stuck in pre-syncing headers loop" behavior at least once within the last years (on a machine that had frequent power failures, and no journaling file system), and this log message would have definitely helped.

Tested using the reproducer patch https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1291765534.

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2571507324)
Not a blocker but still:

https://duckduckgo.com/?q=git+commit+50+72+rule

👍 vasild approved a pull request: "precalculate SipHash constant salt XORs"
(https://github.com/bitcoin/bitcoin/pull/30442#pullrequestreview-3518895337)
ACK 1b810390fe51b79c7063966c9722c01cb8a4f7bf

I ran the benchmark locally with `Clang 19.1.7`. Results look like there is no difference between baseline and PR. Some small differences, within noise.

Base (commit 17072f7005):
```
Run1:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 48.97 | 20,418,823.09 | 0.9% | 10.96 | `SaltedOutpointHasherBench_cre
...

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2571623161)
`build/src/bench/bench_bitcoin` should be changed to `build/bin/bench_bitcoin`

> Please see the attached benchmarks in the PR description

Ok, I retrieved the benchmark tests and ran them. Would be nice to mention in the commit message how to get the tests, e.g. a link to where they can be downloaded from. Or maybe omit the bench results from the commit message altogether?

💬 ismaelsadeeq commented on pull request "Policy: Report debug message why inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2571625742)
Fixed, thanks.

💬 ismaelsadeeq commented on pull request "Policy: Report debug message why inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2571626939)
You are right, this is fixed.

💬 ismaelsadeeq commented on pull request "Policy: Report debug message why inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-3589312092)
Forced push from 9eea72d3f3647197c24329b412c7fc71895e3ea2 to 65d45acf283e7ee5ef75785b6ceaf81bac000791 [9eea72d3f3...65d45acf283](https://github.com/bitcoin/bitcoin/compare/32a5a848f266259a2baacd6ad8d5c2f33ec40775..65d45acf283e7ee5ef75785b6ceaf81bac000791)

- Fixed a nit see https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2432583813
- Remove incomplete accounting of the sigop limit because we bailout immediately the limit is reached https://github.com/bitcoin/bitcoin/pull/29060#
...

💬 pinheadmz commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3589324431)
post-merge ACK 3e01b5d0e7
instructions are clear, built mac outputs on x86 debian, my usual guix builder:

```
fe24816c78e64f0ac463eebd8dda589d6e64630527f67c10f97f4118380097dd bitcoin-3e01b5d0e7be-arm64-apple-darwin-codesigning.tar.gz
ddd416028f6858282d485f0017d8d777d3cee0bcd207f0d8e2542191425602cb bitcoin-3e01b5d0e7be-arm64-apple-darwin-unsigned.tar.gz
8088cd6ef218acc83928bd415b88bbeb082447890e7e497ee0b28e149b0bfc59 bitcoin-3e01b5d0e7be-arm64-apple-darwin-unsigned.zip
1b254bdac0893af4
...

📝 Sjors opened a pull request: "mining: fix -blockreservedweight shadows IPC option"
(https://github.com/bitcoin/bitcoin/pull/33965)
The `-blockreservedweight` startup option should only affect RPC code, because IPC clients currently do not have a way to signal their intent to use the node default (the `BlockCreateOptions` struct defaults merely document a recommendation for client software).

Before this PR however, if the user set `-blockreservedweight` then `ApplyArgsManOptions` would cause the `block_reserved_weight` option passed by IPC clients to be ignored. _Users who don't set this value were not affected._

Fix t
...

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2571728570)
I have a larger refactor coming soon that simplifies this to `node.mining_args.default_block_reserved_weight`. But this is easier to backport.

💬 sedited commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2571742382)
It would be nice if these could stay where they are. How about the following:
<details>
<summary>Protected Method Wrapper</summary>

```diff
diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
index 037d168e0e..04fbbc5473 100644
--- a/src/test/fuzz/block_index_tree.cpp
+++ b/src/test/fuzz/block_index_tree.cpp
@@ -14,0 +15 @@
+#include <test/util/validation.h>
@@ -44 +45 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
- Chainst
...

👍 dergoegge approved a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3519080814)
Code review ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

👍 sedited approved a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3519081005)
ACK 6abbb6ed6cb29b20645a7ef78bcf2e51fc7bde49

💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2571758136)
In `removeConflicts()`, we'd invoke `removeRecursive()` with a transaction that is in the mempool, and then this code would trigger. However I realize now that we already have the iterator that we're trying to remove, so I can save a map lookup by invoking the helper directly. (Prior to #33629 there were more places we'd invoke `removeRecursive()` on transactions in the mempool.)

Otherwise, I think this code will now only trigger in tests.

💬 Sjors commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2571759611)
I have a larger refactor coming soon that simplifies this to `node.mining_args.default_block_reserved_weight`. But this is easier to backport.

💬 sdaftuar commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2571763808)
I agree that this seems expensive, but an `Assume()` wouldn't save us anything, would it? I believe the code would still get invoked (unless the compiler is somehow smart enough to optimize it out).

📝 Sjors opened a pull request: "refactor: disentangle miner startup defaults from runtime options "
(https://github.com/bitcoin/bitcoin/pull/33966)
The interaction between node startup options like `-blockreservedweight` and runtime options, especially those passed via IPC, is confusing.

They're combined in `BlockAssembler::Options`, which this PR gets rid of in favour of two distinct structs:

1. `BlockCreateOptions`: used by interface clients. As before, IPC clients have access to a safe / sane subset.
2. `MiningArgs`: these are set once during node startup

Both structs have a member for the maximum block height, which is not a p
...