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

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638783636)
@MarcoFalke Thanks for the comments; I think I'm going to create a separate PR with some further code refactoring related to this as well, outside of the critical path of #25361.

💬 jonatack 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_r1265859575)
Very nice, done.

💬 jonatack 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_r1265859783)
Much better, thank you!

💬 jonatack 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_r1265860411)
I made the declaration on one line for the smaller diff, and I don't mind reading the parameter list left-to-right. But sure, kept it multi-line.

🤔 mzumsande reviewed a pull request: "[NO MERGE] BIP331 Ancestor Package Relay"
(https://github.com/bitcoin/bitcoin/pull/27742#pullrequestreview-1531801228)
Some initial comments on part 2 (Negotiate Package Relay, Request Ancestor Package Info For Orphans) - will move on to part 3 soon.