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

💬 sedited commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2568083521)
Can you also check that spent_outputs_len is 0 here? It might be the case that the passed in pointer is not null by chance, or UB for that matter, but the intent is still clearly communicated by passing a zero size.

💬 sedited commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2568162586)
I realize you are immitating my own patterns here, but I think it would be better to not rely too heaviliy on the move constructor here, and instead do the following:
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 4102ee9305..2e86c3d8df 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -617 +617 @@ btck_PrecomputedTransactionData* btck_precomputed_transaction_data_create(
- PrecomputedTransactionData txdata{tx};
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2568229012)
Applied except the, IMO, overcrowded `if` condition. Not sure what is the difference between `Now<NodeSeconds>()` and `NodeClock::now()`. Do you suggest to use the former over the later?

💬 sedited commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2568239929)
Can you also add a case for passing in precomputed data for both the segwit and legacy transaction?

🤔 l0rinc requested changes to a pull request: "log: Use more severe log level (warn/err) where appropriate"
(https://github.com/bitcoin/bitcoin/pull/33960#pullrequestreview-3514733100)
Thanks for addressing these!
I have skimmed over the changes, I think some of the warning/error distinctions should be reconsidered.

💬 l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568175398)
Is there any particular reason for keeping the trailing newline here?

```suggestion
LogWarning("\n\n************************\n%s", message);
```

💬 l0rinc commented on pull request "log: Use more severe log level (warn/err) where appropriate":
(https://github.com/bitcoin/bitcoin/pull/33960#discussion_r2568194967)
same:

```suggestion
LogWarning("[snapshot] Failed to restart index %s on snapshot chain", index->GetName());
```