💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2128930669)
In [BIP 327](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki), the paragraph starting from [here](https://github.com/bitcoin/bips/blob/master/bip-0327.mediawiki#identifying-disruptive-signers:~:text=The%20same%20individual%20public%20key%20is%20allowed%20to%20occur%20more%20than%20once%20in%20the%20input%20of%20KeyAgg%20and%20KeySort.) makes the case for allowing duplicate keys in the protocol. It's also recommended to omit such checks.

> In fact, applications are recommended
...

🤔 vasild reviewed a pull request: "Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061#pullrequestreview-2899730748)
Posting review midway. Reviewed up to and including b90f808e30 `http: disconnect after idle timeout (-rpcservertimeout)`

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128494930)
Would be good to have tests to exercise the chunked transfer encoding from d7778f426c `http: support "chunked" Transfer-Encoding`.

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128594514)
The `CloseConnectionInternal()` method can and should take just a reference, no need for shared pointer:

```diff
- void HTTPServer::CloseConnectionInternal(std::shared_ptr<HTTPClient>& client)
+ void HTTPServer::CloseConnectionInternal(const HTTPClient& client)
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128627105)
Would be good to note that this overrides `m_disconnect`:

```cpp
// If set, then the client will not be disconnected even if `m_disconnect` is true.
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128628047)
nit:
```suggestion
// Client requested to keep the connection open after all requests have been responded to.
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128770318)
nit:
```suggestion
if (!InitHTTPAllowList()) {
return false;
}
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128778896)
`LogPrintf()` is using the `Info` severity.

```suggestion
LogWarning("The RPC server is not safe to expose to untrusted networks such as the public internet\n");
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128776391)
```suggestion
for (std::vector<std::pair<std::string, uint16_t>>::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128815408)
nit:
```suggestion
std::this_thread::sleep_for(50ms);
```

💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2128818962)
Could a new client sneak in after the loop `while (!g_http_server->m_no_clients)` exits? Shouldn't that loop be after `interruptNet()` so that new clients cannot be accepted?

💬 Zeegaths commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2944606071)
okay. Thankyou. I appreciate a review

On Thu, 5 Jun 2025 at 16:32, Muniru0 ***@***.***> wrote:

> *Muniru0* left a comment (bitcoin/bitcoin#32565)
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2944359326>
>
> I wanted to do it but I am currently trying to review a very interesting
> PR 😊.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2944359326>,
> or unsubscribe
> <https://github.com/noti
...

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2128980010)
```cpp
prov->GetPubKey(0, arg,
```

Ah I see now the SigningProvider being passed here. I notice that `const SigningProvider& arg` is quite prevalent in this file, but I don't prefer calling the signing provider here just `arg` - it's easy to miss. Fine to stick with it in this PR, can be updated separately for all occurrences in one go if it makes sense to others as well.

🤔 furszy reviewed a pull request: "wallet: Fix wallet interface detection of encrypted wallets"
(https://github.com/bitcoin/bitcoin/pull/32620#pullrequestreview-2900562541)
utACK 130a922980778b293b22169d5e5649afde3ba33b

💬 brunoerg commented on pull request "test: wallet: cover wallet passphrase with a null char":
(https://github.com/bitcoin/bitcoin/pull/32675#discussion_r2128990277)
Yes, make sense, thanks. I will just keep as-is to not invalidate the reviews.

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2129019547)
Had a thought: this harness could be relaxed to where the fuzzer input selects the protected peers, which could be any subset of all the peers. This would allow coverage of a scenario where no peers exceed reservation limits, and possibly stronger assertions in that case.

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

💬 Sjors commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2129029421)
This isn't loading anything so the name is a bit odd. Perhaps instead of splitting:

```diff
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 6a40cfe97e..a633d7966e 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2617,19 +2617,13 @@ util::Result<void> CWallet::DisplayAddress(const CTxDestination& dest)
return util::Error{_("There is no ScriptPubKeyManager for this address")};
}

-void CWallet::LoadLockedCoin(const COutPoint& coin, bool persiste
...

👍 willcl-ark approved a pull request: "init: Configure reachable networks before we start the RPC server"
(https://github.com/bitcoin/bitcoin/pull/32539#pullrequestreview-2900716660)
ACK 12ff4be9c724c752fe67835419be3ff4d0e65990

Tested the new behaviour manually and using the new test, checked the moved code block with `--color-moved=dimmed-zebra`.

💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2945101861)
https://github.com/capnproto/capnproto/pull/2308 - has landed, so you can pull that in here.