💬 fanquake commented on pull request "depends: update freetype and document remaining `bitcoin-qt` runtime libs":
(https://github.com/bitcoin/bitcoin/pull/33952#issuecomment-3584839998)
Guix Build (aarch64):
```bash
97ee63f3335af4ef2fb32c5f99cf8d11c7f38a908f1ed9e202a11ecbde144db0 guix-build-50eb421d9efd/output/aarch64-linux-gnu/SHA256SUMS.part
e55dd97c02a5030fec0cac92f857159bd40820959dee8213df7f918d923cb714 guix-build-50eb421d9efd/output/aarch64-linux-gnu/bitcoin-50eb421d9efd-aarch64-linux-gnu-debug.tar.gz
0131ee5e1249d7295fbe0b601c401070b3d35a1a7c93b562ebb9324122608e88 guix-build-50eb421d9efd/output/aarch64-linux-gnu/bitcoin-50eb421d9efd-aarch64-linux-gnu.tar.gz
0f7326
...

💬 vasild commented on pull request "test: add functional test for outbound connection management":
(https://github.com/bitcoin/bitcoin/pull/33954#issuecomment-3584897920)
Concept ACK, thanks!

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567563419)
Do we need sync_all here? self.generate() already performs synchronization, and this test uses a single node. What are we syncing at this point?

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567706984)
Doesn’t removing these assertions degrade the test? The original test compared RPC results against predefined expected stats.

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567593821)
Testing that all stats share the same set of keys is different from testing that the required keys are present. To preserve the original test’s intent, should we hard-code the expected keys? Other tests follow this approach as well : https://github.com/bitcoin/bitcoin/blob/85d058dc537e62258ef3b3eb73589a242b885b8d/test/functional/rpc_blockchain.py#L135

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567726713)
Maybe these are added by mistake? Same lines are already exist above.

💬 kannapoix commented on pull request "test: Refactor rpc_getblockstats.py to use MiniWallet":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2567721695)
I think this also have a similar problem I pointed out before: https://github.com/bitcoin/bitcoin/pull/33184/files#r2567593821
This changes the test from testing against predefined expected data to cross-checking one getblockstats call with another (by hash). That’s a different test which may degrade the test.

🚀 fanquake merged a pull request: "test: check for output to stdout in `TestShell` test"
(https://github.com/bitcoin/bitcoin/pull/33951)

💬 maflcko commented on issue "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed.":
(https://github.com/bitcoin/bitcoin/issues/33948#issuecomment-3584926632)
It passed with `22~++20251126081852+97732ddb5d92-1~exp1`, fwiw.

💬 hebasto commented on pull request "qa: Remove no longer needed `feature_dirsymlinks.py`":
(https://github.com/bitcoin/bitcoin/pull/33924#issuecomment-3585003301)
> Yeah, I guess there may not be a reduced path coverage inside Bitcoin Core right now, but will it always be that case in the future?

I think so, as long as Bitcoin Core relies on the C++ filesystem library.

> Is there some other benefit to removing the test?

The only reason to remove it is that it does not test any Bitcoin Core–specific functionality. It is simply a logical reversal of the principle that a test should not be added if it verifies nothing of our own.

🚀 fanquake merged a pull request: "Change Parse descriptor argument to string_view"
(https://github.com/bitcoin/bitcoin/pull/33914)

📝 maflcko opened a pull request: "log: Use more severe log level (warn/err) where appropriate"
(https://github.com/bitcoin/bitcoin/pull/33960)
Logging supports severity levels above info via the legacy `LogPrintf`. So use the more appropriate `LogError` or `LogWarning`, where it applies.

This has a few small benefits:

* It often allows to remove the manual and literal "error: ", "Warning:", ... prefixes. Instead the uniform log level formatting is used.
* It is easier to grep or glance for more severe logs, which indicate some kind of alert.
* `LogPrintf` didn't indicate any severity level, but it is an alias for `LogInfo`. So
...

💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-3585062391)
> Is this ready for review now?

No. Someone would have to pick up https://github.com/bitcoin/bitcoin/pull/29231, because most of the logs are alerts, and not info logs.

So i went ahead and did that in https://github.com/bitcoin/bitcoin/pull/33960

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3585078258)
Rebased after #33914 so the test should pass again.

👋 Sjors's pull request is ready for review: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135)

💬 sedited commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2567947291)
The comment you linked seems misleading. `bg_state` is the correct name in my view. We are not operating on the active chain here.

I think the belt-and-suspenders check in `ActivateBestChain` is broken by this change. When we return false in the case of the chain being disabled, we now run into `NONFATAL_UNREACHABLE`. It might be true that there are no similar cases in the call graph of `ActivateBestChain`, but can we guarantee that? For this reason, I think the commit should be dropped.

👍 sedited approved a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-3514478522)
Re-ACK be1af13426e0d4c7aa1da0eb78f15cf1fb166ba2

💬 optout21 commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2567999346)
nit: consistent coding style
```diff
- for (size_t s = 0; s < hashes.size(); s++) {
+ for (auto s{0}; s < hashes.size(); ++s) {
```

💬 optout21 commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568004847)
nit: Could this copy be done simpler? (`leaves = hashes`, `leaves{hashes}`)

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2568023745)
Done with the `return tie <=> tie` even though, for the coverage, it sweeps under the carpet the fact that this is not executed with `this == other` or `this < other` during the tests.