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

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

💬 kristapsk commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#issuecomment-1634333616)
Now it builds, but backwards compatibility with passing number instead of object as a third parameter is broken, will look at it later.

👍 brunoerg approved a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074#pullrequestreview-1528586326)
ACK 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'm 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
--- a/src/wallet/test/fuzz/crypter.cpp
+++ b/src/wallet/test/fuzz/cry
...

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262645801)
ah, thanks. I had a hard time with this one for some reason because casting to `sockaddr` kept truncating the unix socket path to 14 bytes before it got to `Connect()`. Anyway this works and I don't know why I didn't try it like this at first.

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262653604)
Ok thanks I added the preprocessor logic to the function, I'll look into adding afunix.h for windows, maybe in compat.h? also add it to the first commit to test for AF_UNIX in configure.ac

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262664857)
great catch thanks yeah I saw that in your commit but I was confused. makes sense now

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262667486)
got it thanks, fixed this when changing sockaddr_storage to sockaddr_un and it's now `addrun`

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1262690201)
ok fixed this, just using default (with auth) for the test

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