✅ fanquake closed a pull request: "ubsan: add another suppression for InsecureRandomContext"
(https://github.com/bitcoin/bitcoin/pull/33949)

💬 fanquake commented on pull request "ubsan: add another suppression for InsecureRandomContext":
(https://github.com/bitcoin/bitcoin/pull/33949#issuecomment-3588691024)
I think this is the same issue in our infra.

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2571219780)
I don't think this one is better than the version on master, newcomers could still be confused by what happens if they only have two commits...

🤔 rkrux reviewed a pull request: "refactor: replace manual promise with SyncWithValidationInterfaceQueue"
(https://github.com/bitcoin/bitcoin/pull/33962#pullrequestreview-3518411840)
ACK d09e9306cab86c0917cc0ae198ef1993d764d106

Looks fine, this function doc highlights the similarity:

https://github.com/bitcoin/bitcoin/blob/808f1d972be35f4c66bdc30ab0f4064dab0c43c0/src/validationinterface.h#L208-L217

💬 rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2571218739)
Nit: The `callback_set` variable seems unnecessary now.

```diff
diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp
index 05450299e5..0b6213f7fd 100644
--- a/src/node/transaction.cpp
+++ b/src/node/transaction.cpp
@@ -45,7 +45,6 @@ TransactionError BroadcastTransaction(NodeContext& node,

Txid txid = tx->GetHash();
Wtxid wtxid = tx->GetWitnessHash();
- bool callback_set = false;

{
LOCK(cs_main);
@@ -102,22 +101,19 @@ TransactionError BroadcastTransacti
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3588830276)
`e0c9a75fc1...2d398050ee`: add unit test for the private broadcast storage. I could omit it from here and leave it for a followup if reviewers wish?

💬 vasild commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2571374248)
You are right, my bad. Also `FillShortTxIDSelector()` is called from the constructor `CBlockHeaderAndShortTxIDs(const CBlock& block, const uint64_t nonce)`.

🤔 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.