💬 brunoerg commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2665882208)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959636120, https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959639872 and https://github.com/bitcoin/bitcoin/pull/31603#discussion_r1959643968.

It adds 2 more test cases and fix a comment. Thanks, @rkrux.

💬 maflcko commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665883746)
Thank you for your contribution. While this stylistic change can make sense on its own, it comes at a cost and risk for the project as a whole. The weak motivation for the change does not justify the burden that it places on the project. A burden could be any of the following:

* Time spent on review
* Accidental introduction of bugs
* (Silent) merge conflicts, either in the branch or a backport branch. Those conflicts demand further developer and reviewer time or introduce bugs.

Generall
...

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2665894022)
rebased

💬 hebasto commented on pull request "doc: update dev note examples for CMake":
(https://github.com/bitcoin/bitcoin/pull/30739#issuecomment-2665898730)
> On the command line, invoking the underlying commands explicitly is both easier to understand and faster to type:
>
> ```shell
> mkdir build
> cd build
> cmake -GNinja ..
> ninja
> ```

I would advise against this approach, as it may inadvertently lead to the use of an incorrect build tool.

For instance, on macOS, when GNU Make is installed via Homebrew, two build tools become available:
```
% which -a make
/usr/bin/make
hebasto@mini bitcoin % make --version
...

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665909290)
> The burden to explain and motivate a change is on the pull request author. If there was a need to have changes without motivation, I am sure an LLM can generate them en mass.

The author was paged above and just thought he had justification or maybe it was just an oversight.

👍 TheCharlatan approved a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379#pullrequestreview-2623814063)
ACK c4c5cf174883cb53256e869f0d1673e29576a97c

I think I prefer this over passing through `SECP256K1_APPEND_FLAGS`. This is a distinct project after all, and having to configure all its subtrees/subprojects with separate flags seems cumbersome. It would also be inconsistent, since the method introduced here for libsecp already works fine for e.g. leveldb.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959897031)
Added

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959897532)
I flipped it.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2665925797)
Addressed @vasild's feedback (and rebased).

💬 sipa commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665932107)
> The author was paged

You are the author of this PR. You should have a justification for having it open.

💬 sipa commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2665950978)
> would guess there is a livelock or deadlock or similar issue.

Is there progress at all? As in, is the number of validation blocks going not going up at all? That would be a bug. If it's just (very) slow, that may be due to low I/O speed of your disk + pruning + low dbcache. #28280 may speed things up somewhat (which will be in 29.0, not in any release yet).

💬 maflcko commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1959923000)
As the pull title is "CLI cleanups", just a random comment: It seems odd to treat `-getinfo` different than `-netinfo` (and everything else). It would be good for consistency to treat them the same:


```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index defe5e4c4e..18bd6b1f91 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -1267,7 +1267,7 @@ static int CommandLineRPC(int argc, char *argv[])
gArgs.CheckMultipleCLIArgs();
std::unique_ptr<Ba
...

💬 hebasto commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665963150)
@purpleKarrot

> Concept NACK.
>
> I am not a big fan of simple utility functions that group a small set of builtin cmake commands. I think they increase the cognitive load and impede knowledge transfer from one cmake project to another. (I remember a time when builtin functions like `add_library` had to be wrapped and I authored many such wrappers, because there was no way of propagating interface requirements from one target to another. Those times are thankfully over since CMake version
...

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2665963156)
> You are the author of this PR. You should have a justification for having it open.

The justification is that the for loops are redundant and confusing for contributors trying to understand the test. There is no good reason to do such a thing is my justification for opening this PR.

💬 hebasto commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1959928953)
@theuni

> Looking at this again, is there any reason [this needs to be an object library](https://github.com/bitcoin/bitcoin/blob/master/cmake/minisketch.cmake#L43)? It triggers the bug that necessitates the workaround here. I'm wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?

Please see https://github.com/bitcoin/bitcoin/pull/31880.

💬 purpleKarrot commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665972886)
ACK. lgtm.

💬 fjahr commented on issue "rpc: dumptxoutset as sqlite file":
(https://github.com/bitcoin/bitcoin/issues/24628#issuecomment-2665978440)
I think this is resolved with the merged of #27432?

🚀 fanquake merged a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893)

💬 hodlinator commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2666012863)
```
[shutoff] [...\httpserver.cpp:536] [StopHTTPServer] [http] Waiting for 1 connections to stop HTTP server
[http] [...\httpserver.cpp:355] [ThreadHTTP] [http] Exited http event loop
[net] [...\util\thread.cpp:22] [TraceThread] net thread exit
```
The fact that we don't get "Stopped HTTP server" or even "Waiting for HTTP event thread to exit" after these indicates that we are stuck in the lower part of `StopHTTPServer()`:

https://github.com/bitcoin/bitcoin/blob/790c509a4796ec47be2275d86b35370f
...

💬 maflcko commented on pull request "ci: switch MSAN to use prebuilt Clang binaries":
(https://github.com/bitcoin/bitcoin/pull/31850#issuecomment-2666017905)
> Needs a fixup for MSAN fuzz.

What is the error?