🤔 stickies-v reviewed a pull request: "http: add evhttp_connection_set_closecb to avoid g_requests hang"
(https://github.com/bitcoin/bitcoin/pull/27909#pullrequestreview-1533190394)
Nice investigative work, think I found the issue as per my comment.

I wonder if we could track connections instead of requests. A quick implementation (docstrings etc generally not updated, just a PoW) seems to indicate that it does:

<details>
<summary>git diff on d170e6c60cd9f46da1794448b8be55ba6f0b2922</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 849e9b482..8fe826f55 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -151,9 +151,9 @@ sta
...

💬 stickies-v commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1265614783)
This is the culprit of [what you're observing](https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1637992727). `req` becomes a dangling pointer as soon as a request is completed. In most cases (which is why we typically need > 1 number of iterations), this just means that `g_requests.erase(req)` removes 0 elements because it points to a non-existing request, but sometimes (and actually quite frequently as addresses seem to be reused quite aggressively) this will point to an unrelated req
...

💬 real-or-random commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638552877)
I believe that this failure is pretty sporadic. So, it may not be worth updating the subtree.

There's also still (sorry!) the sporadic `test condition failed: ((~x[m][shift]) << (64 - (1 << usebits))) == 0` failure (https://github.com/bitcoin-core/secp256k1/pull/367, https://github.com/bitcoin-core/secp256k1/issues/610). That one will be solved by https://github.com/bitcoin-core/secp256k1/pull/1298 by @sipa. Perhaps we should prioritize this PR and wait for its merge first.

💬 real-or-random commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638554188)
Concept ACK (meaning there's nothing wrong with updating the subtree right now)

🤔 stratospher reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1533297514)
tested ACK 4e5c933.

cross checked that this is consistent with poly1305-donna implementation. benchmarks look better compared to previous `poly1305_auth` code too. (also found this [blogpost](https://loup-vaillant.fr/tutorials/poly1305-design) which explains poly1305's design interesting!)

noticed this tiny difference in poly1305-donna's vs [RFC 8439](https://datatracker.ietf.org/doc/html/rfc8439#section-2.5.2) `r` - though it might also be some trick for later computation i guess.
> - r
...

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1265679362)
Thanks, done!

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1638569400)
Forced pushed updating changes in PR [28065](https://github.com/bitcoin/bitcoin/pull/28065).

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638576887)
@stratospher There are two approaches for poly1305 implementations out there; one where the key (and the accumulated hash value) are represented using 32-bit limbs, and one where it's represented using 26-bit limbs (so in one `r = sum(r[i] * 2^(32*i))` while in the other `r = sum(r[i] * 2^(26*i))`). Poly1305 is designed to make 32-bit limb designs efficient (the masking out of the bits guarantee some overflows are avoided, as explained in the blogpost you link). However, in all my benchmarks the
...

👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1533336360)
reACK 7570e2228e22c3b22e119e159c6fa07c4c891d98

👍 stickies-v approved a pull request: "validation: use noexcept instead of deprecated throw()"
(https://github.com/bitcoin/bitcoin/pull/28090#pullrequestreview-1533371791)
ACK 047daad4f59942488163c6be8516a69291646294

💬 sipa commented on pull request "validation: use noexcept instead of deprecated throw()":
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1638628672)
utACK 047daad4f59942488163c6be8516a69291646294

💬 Inatiye0911905991 commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1265745026)
> Thanks, done!

💬 Inatiye0911905991 commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1638648798)
> ACK [6c98f4c](https://github.com/bitcoin/bitcoin/commit/6c98f4c137c9d557c78ebd52379711ebbd23e24a)
>
> non blocker, just an idea: Perhaps we could have encrypt/decrypt stuff outside of `LIMITED_WHILE` and then we could have a call just to fill them. I suppose this way we would have more chance of decrypting a previous encrypted thing. Just an example:
>
> ```diff
> diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp
> index 7ba43821b..03ad97a35 100644
> ---
...

🤔 stickies-v reviewed a pull request: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078#pullrequestreview-1533381378)
Concept ACK but I think some changes are unnecessary churn

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265725273)
nit: not sure this is an improvement? I find the existing format easier to read, with less LoC change.

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265746027)
I don't see the benefit of this helper function. It doesn't really make things more readable imo (the underlying calls are short and clear enough for me). It also feels quite arbitrary: what about `IsTorOrCJDNS()`, `IsI2POrCJDNS()`, ...?

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265737690)
Entire function can be simplified to just:

```cpp
return GetLocal(peer).value_or(CService{CNetAddr(), GetListenPort()});
```

💬 stickies-v commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265733802)
I think this can be simplified further?
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 0d6b4f04a..c6c4c9a3e 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -149,7 +149,7 @@ uint16_t GetListenPort()
[[nodiscard]] static std::optional<CService> GetLocal(const CNode& peer)
{
if (!fListen) return std::nullopt;
- CService addr;
+ std::optional<CService> addr;
int nBestScore = -1;
int nBestReachability = -1;
{
@@ -167,13 +167,12 @@ uint16_t GetListenPort(
...

💬 Inatiye0911905991 commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1265749026)
https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1634533832

💬 furszy commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1224793119)
If the transaction has two outputs: index 0 not change, and index 1 change for the wallet. When `reduce_output=0`, the first round of the loop will set `destChange` to the first output script, then the second round will overwrite `destChange` with the second output script.

This will have the bad outcome of dropping the first output (the one selected by the user) and send all coins to the second one. Leaving the bumped tx with only one output.

A solution for this could be to not discard cha
...