💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1263941686)
This resulted in `netbase.cpp:415:27: runtime error: implicit conversion from type 'int' of value 210 (32-bit, signed) to type 'char' changed the value to -46 (8-bit, signed)`. This is ok in practice because the data is transferred as bytes anyway over the socket (not as integers). Anyway, to silence the sanitizer:

<details>
<summary>[patch] sock: introduce a generic Sock::SendComplete(const void*, size_t, ...)</summary>

```diff
commit 8467080a08327aaf0d853a73412456c44558a9b3
Parent: af
...

🚀 achow101 merged a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048)

🤔 mzumsande reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1530681852)
ACK 31eca93a9eb8e54f856d3f558aa3c831ef181d37

The PR description is outdated - its last sentence can be removed.

🤔 jonatack reviewed a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411#pullrequestreview-1530682192)
Post-merge ACK

💬 jonatack commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263967598)
f4754b9dfb84859166843fb2a1888fb3cfebf73c It would be nice to avoid low-level `Network` enum value comparisons when we have built-in higher level helpers we can use.

💬 jonatack commented on pull request "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting":
(https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263969104)
While touching this, the `GetLocal()` getter helper is unused outside the class and could be

- moved from the header to the implementation
- made static and nodiscard
- converted from a bool with an out-param to a std::optional without an out-param

📝 jonatack opened a pull request: "net, refactor: remove unneeded exports, improve separation, use std::optional"
(https://github.com/bitcoin/bitcoin/pull/28078)
and other improvements noticed while reviewing #27411. See commit messages for details.

⚠️ jonatack opened an issue: "libsecp CI failure [no wallet, libbitcoinkernel] [focal]"
(https://github.com/bitcoin/bitcoin/issues/28079)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

CI failure: https://cirrus-ci.com/task/5453591138271232


### Expected behaviour

Expect CI task to pass for a pull with unrelated changes.

### Steps to reproduce

See https://cirrus-ci.com/task/5453591138271232


### Relevant log output

```
FAIL: tests
===========

test count = 64
random seed = 81af32fd7ab8c9cbc2e62a689f642106
src/modules/ellswift/tests_impl.h:396: test conditio
...

💬 kristapsk commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1636198809)
> Because I don't think it is a good use of developer and reviewer time to write not type-safe code, only to later (after review or merge) discover that it crashes with a `TypeError`.

Why not just use C++ then? (I'm not a Rust expert, maybe there are good reasons)

> I can't recall the last time I've run any linter outside of testing one after writing the code for it.

I try always to remember running linters locally before submitting a new PR (sometimes I fail and forget, need to admit)
...

💬 jonatack commented on issue "libsecp CI failure [no wallet, libbitcoinkernel] [focal]":
(https://github.com/bitcoin/bitcoin/issues/28079#issuecomment-1636218979)
Possibly spurious. The CI task passed after re-running it: https://cirrus-ci.com/task/5885691087814656

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1264030502)
Now its ` ERROR: Error during SOCKS5 proxy handshake: send(): Transport endpoint is not connected (107)` I'm still digging in to this...

💬 furszy commented on pull request "[WIP] descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#discussion_r1264064042)
fixed

👋 jonatack's pull request is ready for review: "net, refactor: remove unneeded exports, use helpers over low-level code, use optional"
(https://github.com/bitcoin/bitcoin/pull/28078)

💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1636339566)
Initial code review ACK (didn't review the test changes yet), will test.

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1636342363)
This is rebased after #27607, which allowed making a lot of simplifications and improvements. Also, I think the PR is better organized now, so I think I could split off the first commit or first few commits into a separate PR if reviewers would find that helpful.

💬 achow101 commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1636345229)
ACK 47abfe1525cc2700699ba0605747f1ad36a34a1b

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1636352480)
> The PR description is outdated - its last sentence can be removed.

Thanks, struck it out since PR was merged and the old description did get added to git history.

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1636359937)
0ffa0f1c370494b6b99a9cdf911b20ee655ac829 (and later) breaks at line line 96 when you run `wallet_backwards_compatibility.py --descriptors` without BDB:

```
JSONRPCException: Wallet file verification failed. Failed to open database path '/tmp/bitcoin_func_test_x3n9_0vn/node1/regtest/wallets/w1_v19'. Build does not support Berkeley DB database format. (-18)
```

Otherwise it works, just some stylistic notes inline.

> 0.16 creating new wallets rather than testing the target wallets

Is
...

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264084847)
6a5d245b89c51a53989b984d31d7bb4d807837c3: if a node is node is not used, we should reduce the number of nodes and `node_v19` should be `self.num_nodes - 3`.

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264088086)
72362ed1fbbab9dfbaf224f930ed9b328f29c55f: maybe keep all the `node_v..` definitions in one place at the top of the test?