💬 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
...

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

For MSVC build suggesting the diff as follows:
```diff
--- a/build_msvc/common.init.vcxproj.in
+++ b/build_msvc/common.init.vcxproj.in
@@ -90,7 +90,7 @@
<AdditionalOptions>/utf-8 /Zc:__cplusplus /std:c++20 %(AdditionalOptions)</AdditionalOptions>
<DisableSpecificWarnings>4018;4244;4267;4715;4805</DisableSpecificWarnings>
<TreatWarningAsError>true</TreatWarningAsError>
-
...

💬 hebasto commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1638692761)
Concept ACK.

💬 sipa commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1638767118)
utACK c7db88af71b3204171f33399aa4f33b40a4f7cd9

Longer term, and not for this PR, I think it would make sense to make a clearer separation between "miniscripts (and descriptors in general) which have undesirable properties so they shouldn't be importable without warning", and "miniscripts that are so deficient we cannot reason about them at all". For example, if one has a malleable (or non-IsSane(), for certain reasons) miniscript, it'd be a bad idea to import it, as assessment of its properti
...