🤔 ryanofsky reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1526920138)
Rebased 3e6b6877645f978b4999efdfb9e616f4dbf9dd19 -> cbea21da035d9dbcd38669d780611c9a05442f3d ([`pr/stopafter.4`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.4) -> [`pr/stopafter.5`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.4-rebase..pr/stopafter.5)) fixing conflict with #28053 and also removing mentions about changed -stopafterheight behavior after Martin's comment https://github.com/bitcoin/bi
...

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1261536518)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1261480112

:man_facepalming: Thanks! I didn't notice the `StartShutdown` call was followed by a `if (m_chainman.m_interrupt) break` line. So it was already exiting the loop after the height check, and adding a new `break` would not change anything.

I think you are also right that there are race conditions where extra blocks could be attached, but I haven't looked into the details. Maybe it is worth opening an issue with your obs
...

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1262699057)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259566308

I'm not too concerned about `blockTip` notification shutting down during an `invalidateblock` call, because it seems like a corner case to worry about someone using `-stopafterheight` and `invalidblock` at the same time. If someone is doing this, I think there is only a small change of behavior not worth documenting where the `-stopatheight` option might take effect a little earlier and the node would shut down a little
...

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262751862)
ah sorry

💬 sipa commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1634494768)
@furszy I think there is only a downgrade issue, no? If someone were to import a `sh(raw(...))` as watch-only address in a descriptor wallet after allowing that, and then downgrades to older software which does not support it.

💬 Sjors commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#issuecomment-1634495424)
I think @mzumsande's suggestion is better. But the current PR, with @instagibbs's tests to document the existing weirdness, is fine too.

👍 jamesob approved a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1528767782)
reACK faebf00

Trivial rephrasing to avoid `FILE` release looking like a multiplication:

```diff
diff --git a/src/streams.h b/src/streams.h
-index 03df20b5db..fc2fe4d9f8 100644
+index 03df20b5db..5ff952be76 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -13,6 +13,7 @@
@@ -315,7 +315,7 @@
- file = nullptr;
- }
- return retval;
-+ if (std::FILE * rel{release()}) return std::fclose(rel);
++ if (auto rel{release()}) return std::fclose
...

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

👍 TheCharlatan approved a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1528801908)
Very happy with this change. Nice to see these last few PRs pushing configuration options out of the kernel and letting the user decide how to react to kernel actions.

ACK 31eca93a9eb8e54f856d3f558aa3c831ef181d37

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1634533832)
Thanks, @MarcoFalke and @brunoerg for the reviews.

- Moved `Encrypt/Decrypt` variables outside of `LIMITED_WHILE` and added a function to update them regularly.

💬 achow101 commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1634547609)
Allowing new types of descriptors only disallows downgrading of wallets that have imported them. Downgrade protection is automatic for such wallets since descriptor wallets were introduced.

💬 hebasto commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262872076)
I apologize for revisiting this old PR, but I couldn't resist asking to clarify things for me.

As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.

💬 achow101 commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1634647753)
ACK e7cf8657e1165ea5da3911a9e543837cd8938f97

🚀 achow101 merged a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)

💬 ryanofsky commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262895010)
> As far as I understand, `interfaces::MakeNodeInit` returns a `std::unique_ptr` that owns an instance of `init::BitcoindInit` or throws an exception. Given this understanding, it seems unclear how the `return exit_status;` statement could ever execute.

There are two `interfaces:MakeNodeInit` functions. The one in [`src/init/bitcoind.cpp`](https://github.com/bitcoin/bitcoin/blob/master/src/init/bitcoind.cpp#L41-L44) which is linked into `bitcoind` will leave exit_status unchanged, as you say.
...

💬 hebasto commented on pull request "multiprocess: Add basic spawn and IPC support":
(https://github.com/bitcoin/bitcoin/pull/19160#discussion_r1262899118)
Right. Thank you.

💬 recursive-rat4 commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634706063)
utACK fa4ccf15118a5e2dcd2a98283b9e6c7e1955a7f1

`CXXFLAGS` was set in https://github.com/bitcoin/bitcoin/pull/24735 without intention to disable optimizations.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262927938)
Ok adding two commits to handle these two affected areas. For the GUI I've written logic that accommodates the new Proxy features but I'll have to submit a follow-up PR to enable unix socket path configuration from the GUI as a separate element from address:port proxy configuration.

So with this only PR, the user can only set unix sockets outside the GUI, and in the network tab they will see `"Options set in this dialog are overridden by the command line..."` anyway.

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1634714444)
rfm or is something missing here?

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262930258)
Ok I'll put this statement after `SetNonBlocking()` but I think the Socks5 stuff should live in a follow-up PR?