💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1265607620)
I wouldn't say it's controversial - I have no problem with adding this.

I just want to question if duplicating lots of things (if here, I suspect there will be several others too) is the right way to go.

📝 fanquake opened a pull request: "subtree: update libsecp256k1 to latest master"
(https://github.com/bitcoin/bitcoin/pull/28093)
Includes https://github.com/bitcoin-core/secp256k1/pull/1378. Which fixes #28079.

💬 fanquake commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638514173)
cc @real-or-random @jonasnick

💬 darosior commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1638517435)
> No, the rationale was definitely about transaction malleability

Malleability is orthogonal. I was responding to the definition given above of "does the apparent policy of this miniscript match its actual semantics". An `older(42)` script can well be malleated, its apparent policy will match its semantics.

But anyways, let's switch to ##miniscript if we want to continue discussing this? It's not what this PR is implementing anymore.

💬 sipa commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#issuecomment-1638520565)
utACK 7d92b1430a6fd42c4438810640576830d0ff8d13

💬 fanquake commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638522424)
Looks like we'll need to adapt to Windows DLL/symbol export related changes. Will fixup.

🤔 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