📝 Ayush170-Future opened a pull request: "fuzz: wallet, add target for `Crypter`"
(https://github.com/bitcoin/bitcoin/pull/28074)
This PR adds fuzz coverage for `wallet/crypter`.

Motivation: Issue [27272](https://github.com/bitcoin/bitcoin/issues/27272#issue-1628327906)


I ran this for a long time with Sanitizers on and had no crashes; the average exec/sec also looks good to me. However, I would really appreciate it if some of the reviewers could try it on their machines too, and give their feedback.

💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#issuecomment-1634282656)
In addition to this and `CoinControl`, I'm working on the `FeeBumper` and `Spend` files. The majority of my work in FeeBumper is finished, and I'll open a PR for it as soon as we reach a conclusion on this PR.
I'm currently working on the Spend file, which should be finished this week or next, hopefully.

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

💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `Crypter`":
(https://github.com/bitcoin/bitcoin/pull/28074#discussion_r1262622915)
```suggestion
/*nDerivationMethod=*/ nDerivationMethod);

```

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

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