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

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2568031552)
we already have a dedicated method for testing init errors, I don't think the loop helps here, neither with the failures (we just see the loop failing) or with making it obvious what the `expected_msg` is for a give `extra_args`. We're also lacking coverage for the `-proxyrandomize=0` - what do you think of the following:
```suggestion
# -privatebroadcast init error: Tor/I2P not reachable at startup
self.nodes[0].assert_start_raises_init_error(
extra_args=["-privatebr
...

💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2568100704)
Yes, but the same logic applies to the uncovered paths - unlike in the previous case. And we might as well cover those cases, knowing now that they're uncovered :)

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2568112479)
I deliberately avoided changing any of that in this commit: https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2158456828

💬 optout21 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2568126984)
The `CTransaction::ToString()` output looks the same now, my concern is addressed.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2568141807)
Removed the txid comparison and reworded the commit message.

🤔 optout21 reviewed a pull request: "transaction: Adding script witness to ToString for CTxIn"
(https://github.com/bitcoin/bitcoin/pull/33711#pullrequestreview-3514739481)
utACK 812495a426a3ea9ef15567983ff31e22d05e9f5d

Minor extension to the `ToString()` method of the `CTxIn` class. Well localized change, minimal risk of unintended impact. Thanks for the initiative.

💬 yuvicc commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3585353671)
I agree with @sedited [comment](https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2567947291), the belt-and-suspenders check in ActivateBestChain in [validation.cpp](https://github.com/bitcoin/bitcoin/blob/38c8474d0d774b1ed5d6139a9fec9933a6cfc099/src/validation.cpp#L1287C1-L1295C6) is broken by this change. When `m_disabled` is true, `ActivateBestChain` returns false without setting the `BlockValidationState` to invalid. This causes the new `NONFATAL_UNREACHABLE()` assert in `ProcessNewB
...

💬 sedited commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2568197923)
The reason I was hesitant to submit this patch earlier is that the developer needs to associate the precomputed data with this transaction, and we don't have a straight forward semantic mechanism to enforce that. While I don't think this is particularly problematic, since the same can also be said for the outputs currently, it should be clearly communicated in the documentation for this function and I think also deserves a test case with such a mismatch.

💬 sedited commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2568068783)
It would be nice to get a test where we verify both inputs of a two-input taproot transaction with the same data. I think we currently do that implicitly in the full-chain script verification test further down, but I'd like an explicit test too.