💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2178059683)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2176634257

> Maybe comment that the `!` is for gcc

Makes sense, added comment

🤔 furszy reviewed a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-2976212991)
I think this worth a general RPC util function like (haven't tested it):

```c++
template <typename... Value>
bool AreParamsNullOrEmpty(const Value&... params) {
return ((params.isNull() || params.empty()) && ...);
}
```

Which, for example should work fine for `getdescriptoractivity `:
```c++
if (AreParamsNullOrEmpty(request.params[0], request.params[1])) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "At least 1 blockhash or 1 descriptor must be specified.");
}
```

💬 maflcko commented on issue "[BTC signet v22.0] websocket not working as expect":
(https://github.com/bitcoin/bitcoin/issues/32848#issuecomment-3024849863)
> It think it works on bitcoin testnet & mainnet network with version v22.0.0.
>
> I am wondering why it's not working with signet.

What are the exact steps to reproduce the working case and the failing case. What is the error message?

Without any details there is nothing that can be done here.

👍 brunoerg approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976319365)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d

🤔 w0xlt reviewed a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2976354289)
ACK https://github.com/bitcoin/bitcoin/pull/32846/commits/0e9f409db3b7b08aef75ce39765b018b69cc8e9d

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#issuecomment-3024909015)
@instagibbs @ismaelsadeeq See https://github.com/sipa/bitcoin/commit/0a8e1d6e39bd1ba6d3df149945b8dfd78e1fb7e6

👍 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