💬 maflcko commented on pull request "docs: remove passing CI section in guidelines for PR merging as it's common sense":
(https://github.com/bitcoin/bitcoin/pull/32006#issuecomment-2818560151)
lgtm ACK 17a1630f226e1c8eb13eb7c7425290e933ad9368

Makes sense to remove this, because it is common sense, and also redundant with the readme sections https://github.com/bitcoin/bitcoin/blob/master/README.md#automated-testing and https://github.com/bitcoin/bitcoin/blob/master/README.md#manual-quality-assurance-qa-testing.

📝 maflcko reopened a pull request: "docs: remove passing CI section in guidelines for PR merging as it's common sense"
(https://github.com/bitcoin/bitcoin/pull/32006)
Removing passing CI section in guidelines for PR merging as it's common sens

💬 saikiran57 commented on issue "signrawtransactionwithkey command shouldn't output the "Witness program was passed an empty witness" error for a TapRoot transaction":
(https://github.com/bitcoin/bitcoin/issues/27017#issuecomment-2818575632)
Its not related to bug https://github.com/bitcoin/bitcoin/issues/31589,

you can see this line https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L477 it is consider the XOnlyPubKey{vSolutions[0]} instead of tweaked public key. So that's why when signing https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L67 it returns false because output is locked to tweaked public key where as we are searching for x-only-public key.

👋 mzumsande's pull request is ready for review: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290)

💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818686077)
Concept & Approach ACK

Code nits:
```diff
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -930,11 +930,11 @@ BOOST_AUTO_TEST_CASE(script_json_test)
auto element = test[pos][i].get_str();
// We use #SCRIPT# to flag a non-hex script that we can read using ParseScript
// Taproot script must be third from the last element in witness stack
- std::string scriptFlag = std::string("#SCRIPT#");
-
...

💬 EniRox001 commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2818701974)
> Code change looks good to me. The commit message doesn't seem to say anything about the typo fix though. It should be mentioned there and that the rest of the change is fixing a secondary issue caused by this. You could also split this in two separate commits.

Thanks for the review! I've split the changes into two logical commits as suggested:

1. First commit fixes the nTimes -> nTime typo and explains its impact on the test behavior
2. Second commit addresses the test failure that was
...

📝 VolodymyrBg opened a pull request: "Fix failing util_time_GetTime test on Windows"
(https://github.com/bitcoin/bitcoin/pull/32318)
Remove unreliable steady clock time checking from the test that was causing CI failures primarily on Windows. The test previously tried to verify that steady_clock time increases after a 1ms sleep, but this approach is not reliable on all platforms where such a short sleep interval may not consistently result in observable clock changes.

This addresses issue #32197 where the test was reporting failures in the cross-built Windows CI environment. As noted in the discussion, the test is not cr
...

👍 ryanofsky approved a pull request: "doc: Add deps install notes for multiprocess"
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2781680324)
Code review ACK 7f5a35cf4b31f176ff9138d21f3ec677ae7d1bcf just dropping dependencies.md update since last review

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052036849)
nit: consider using `string_view` instead of `string` here and below.

💬 1440000bytes commented on pull request "rpc: Allow fullrbf fee bump in (psbt)bumpfee":
(https://github.com/bitcoin/bitcoin/pull/31953#issuecomment-2818913559)
reACK https://github.com/bitcoin/bitcoin/commit/fa86190e6ed2aeda7bcceaf96f52403816bcd751

👍 TheCharlatan approved a pull request: "test: test MAX_SCRIPT_SIZE for block validity"
(https://github.com/bitcoin/bitcoin/pull/32304#pullrequestreview-2781694117)
ACK b1ea542ae651cec433910d8c727abc41f17a7678

💬 romanz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2052664896)
nit: maybe the following would be simpler?
```c++
for (const auto& [k, v] : m_map) {
out += k + ": " + v + "\r\n";
}
```

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052668744)
Yes, that is the gist of it. Will add a commit at the end to add some documentation in the header.

🤔 shahsb reviewed a pull request: "bench: close wallets after migration"
(https://github.com/bitcoin/bitcoin/pull/32309#pullrequestreview-2781713855)
Concept ACK

💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2818997734)
@sipa Addressed

👍 ryanofsky approved a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273#pullrequestreview-2781756291)
Code review ACK 6b4b7ac2113ed969bd31ce42190b96bd2e1b9c97. Main change is expansion of test to cover restoring from the backup.

IIUC, PR description seems helpful for describing part of the problem, but contains inaccuracies and would be good to revise to avoid being misleading. Here are things I might add or correct:

1. PR describes there being a problems with relative paths, but not all relative paths have a problem, and absolute paths have problems too. Things only currently work correct
...

🤔 janb84 reviewed a pull request: "tests: improves tapscript unit tests"
(https://github.com/bitcoin/bitcoin/pull/31640#pullrequestreview-2781803719)
Re ACK [e3d7533](https://github.com/bitcoin/bitcoin/pull/31640/commits/e3d7533ac954516d87a09b230c2898f5c9b1e213)

Changes since last ACK:
- code cleanup

💬 darosior commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#discussion_r2052733269)
I think it would be clearer to take a vector of spent `Coin`s directly? Or even a span?

💬 darosior commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2052746710)
So i looked into this and i don't think there is a clean way of doing it that would help here. What this function is doing is really a hack, creating a block assembler for a specific tip and mining a block with `prev` that does not correspond to it. However one thing is the `nSequence` doesn't change from one block to another. I added here for clarity but i now think it's unnecessarily confusing, so i removed it.

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2781869977)
Code review ACK 9e0fede6a6d608ff307b48b9a326c6d467b410cf. Main change since last review were a lot of `feature_framework_startup_failures.py` cleanups. I do think the drop or squash commit 9e0fede6a6d608ff307b48b9a326c6d467b410cf is an improvement so would vote for squash, but no strong opinion