👍 theStack approved a pull request: "test: check P2SH sigop count for coinbase tx"
(https://github.com/bitcoin/bitcoin/pull/32850#pullrequestreview-2976370364)
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

Good catch. Verified that on master, indeed all unit and functional tests pass if the tested code part in `GetP2SHSigOpCount` is changed (or removed), whereas unit tests fail with this PR.

💬 maflcko commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2178187277)
seems easier and less brittle to modify `GetWalletNameFromJSONRPCRequest` to:

```
const std::string wallet_name{GetWalletNameFromJSONRPCRequest(request, self.MaybeArg<std::string>("wallet_name")};
```

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#issuecomment-3024957772)
Concept ACK. @SomberNight it's still useful if you open an issue to track the reason you need this, just in case this implementation PR doesn't make.

👋 achow101's pull request is ready for review: "wallet: Track no-longer-spendable TXOs separately"
(https://github.com/bitcoin/bitcoin/pull/27865)

🤔 w0xlt reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2976415024)
ACK https://github.com/bitcoin/bitcoin/pull/32593/commits/6135e0553e6e58fcf506700991fa178f2c50a266

Moving to (Un)LockCoin, the responsibility logic of creating the `WalletBatch` instance and persisting the data improves code readability and separation of concerns.

💬 instagibbs commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3024981580)
@sipa one order of magnitude more for me on my wimpy laptop but well within acceptable bounds I suspect for a reorg. IIUC this is one of the more pessimistic mempool topologies for this since you're going to have to go through the ~entire mempool before finishing

🤔 Sjors reviewed a pull request: "RPC/txoutproof: Support including (and verifying) proofs of wtxid"
(https://github.com/bitcoin/bitcoin/pull/32844#pullrequestreview-2976408858)
Took an initial look at the code, but got a bit confused. Will check again later.

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178198327)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: to avoid confusion between an _empty_ block and a pre-segwit block, could comment:

```
// Block without coinbase
```

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178194140)
605b372b1f62c4cd59d8056f0a6a1baf17c24014: `*Assert(block)`

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178192051)
605b372b1f62c4cd59d8056f0a6a1baf17c24014 nit: maybe rename `tx` to `coinbase`

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178215585)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036:

```cpp
if (prove_witness) {
if (i == 0) {
// generation tx has null wtxid
wtxids.emplace_back();
} else {
wtxids.push_back(block.vtx[i]->GetWitnessHash());
}
```

💬 Sjors commented on pull request "RPC/txoutproof: Support including (and verifying) proofs of wtxid":
(https://github.com/bitcoin/bitcoin/pull/32844#discussion_r2178222894)
3abfd6d3d99bbcceb09162e83d8f9f9769bd4036: what does this do?

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238384)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178238458)
Thanks, fixed in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178244200)
done in https://github.com/bitcoin/bitcoin/pull/24539/commits/d5980f39f934a8e251ddaf6801d23bd5a4bcb3ef. Moving this does not work (maybe a conflict with the mempool lock ?) I'll investigate.

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178248747)
Correct but I wanted to have 2 explicit tests: one when a spending tx is replaced, the other when it is cancelled.

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178249989)
Sorry I don't understand this ?

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-3025042997)
> In the pull request description you are saying that the keys are 64 bytes, but I guess you mean 64 bits?
Yes 64 bits.
>
> Did you split this work into two commits to gather feedback on tradeoffs between the two approaches? The space savings and the ability to immediately return a transaction is nice, but it is also not compatible with pruning (and still requires notices about that in init.cpp). Some lightning implementations do support pruned nodes, I guess they won't be able to leverage t
...

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178267285)
It may help user figure out they've made a mistake in their setup/configuration ?

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2178272887)
It did help when the index returned a tx id, it does not anymore.