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

💬 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_r1264078873)
c31e0db7f4263096d503f07707af9d0a733e50da The original `< 170000` made it more clear that this applies to _all_ old versions. Not a big deal though, since it's unlikely we'll add even older nodes.

💬 achow101 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_r1264119254)
0.18 is still tested, just not explicitly with its own special casing and variables.

💬 achow101 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_r1264136631)
No, this is more correct. This behavior doesn't apply to all older versions, only to 0.16.x. Versions prior to this kept the wallet files in the main datadir. 0.16 added the wallet directory but the wallets are still per file. 0.17 moved wallets into the current one directory per wallet structure.