👍 MarcoFalke approved a pull request: "test: LLVM/Clang 16 for MSAN jobs"
(https://github.com/bitcoin/bitcoin/pull/27436#pullrequestreview-1376328538)
lgtm

💬 MarcoFalke commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1160787798)
any reason to change this here and elsewhere? Either you use `update-alternatives`, or hardcode the version, but doing both seems not needed?

💬 fanquake commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1160790846)
Right, no need to change these with update-alternatives. Dropped.

💬 fanquake commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1160790999)
Moved to 16.0.1.

🤔 Sjors reviewed a pull request: "[Draft / POC] Silent Payments"
(https://github.com/bitcoin/bitcoin/pull/24897#pullrequestreview-1376326898)
Don't forget to rebase. The bech32 hrp stuff moved to `kernel/chainparams.{h,cpp}`.

> The silentpayment index has been removed from the codebase. … A new PR, with the index, will be added soon.

That's nice. It's more important to prove functionality, can improve performance later.

The `rawtr()` functionality has been merged, so the dependency can be removed from the PR description.

Enabling the ECDH module in `libsecp256k1` might be a reason to put silent payments behind a configure
...

💬 Sjors commented on pull request "[Draft / POC] Silent Payments":
(https://github.com/bitcoin/bitcoin/pull/24897#discussion_r1160786689)
3742cc75571a60bda52a41923c8244535af9ff23: maybe move that above `UNKNOWN`?

💬 Sjors commented on pull request "[Draft / POC] Silent Payments":
(https://github.com/bitcoin/bitcoin/pull/24897#discussion_r1160804856)
9143c7e1f1710f123072cc272b66ab2cad0eb57a: this commit won't compile, probably needs something from a later commit

💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1500433830)
`e42ce20c65...ebfe47e594`: rebase due to conflicts

💬 jessebarton commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1500477935)
@vasild Thanks that helped a ton!

Looks like I have it down to one now.

💬 jessebarton commented on pull request "doc: FreeBSD DataDirectoryGroupReadable Setting":
(https://github.com/bitcoin/bitcoin/pull/26741#issuecomment-1500484059)
@achow101 Did you mean to push to this branch?

💬 pinheadmz commented on issue "RPC wont bind without an IP address on a non-localhost interface":
(https://github.com/bitcoin/bitcoin/issues/13155#issuecomment-1500485576)
I know this is an old one, but can you offer steps to reproduce? I down'ed all interfaces except `lo` on macOS and rpc bound fine:

```sh
--> ifconfig | grep UP -A 5
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 16384
options=1203<RXCSUM,TXCSUM,TXSTATUS,SW_TIMESTAMP>
inet 127.0.0.1 netmask 0xff000000
inet6 ::1 prefixlen 128
inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1
nd6 options=201<PERFORMNUD,DAD>
```

```
2023-04-07T17:19:42Z Binding RPC on address ::1 port 18443
2023
...

👍 vasild approved a pull request: "httpserver, rest: fix segmentation fault on evhttp_uri_get_query"
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1376409023)
ACK 4f68c30b2d52f39c7c7465d49ba41c2e70f486ba

Would be nice to split the actual fix from the other changes. I guess it would be easier for other reviewers too.

I would have liked to remove the `replySent` argument, but that's secondary to this PR which fixes a serious bug. Let's get the bug fixed and if I feel strong enough about it, then maybe I will open a followup to remove that parameter.

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1160843481)
Remove this unrelated whitespace change.

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1160845066)
This will always be `true` because if `uri_parsed` is `nullptr` we would have returned 2 lines above.

🤔 Xekyo reviewed a pull request: "bumpfee: avoid making bumped transactions with too low fee when replacing outputs"
(https://github.com/bitcoin/bitcoin/pull/27308#pullrequestreview-1376422479)
ACK 26b4c66418f97b4d13989a5c6cf2a7827af4fc1e

💬 Xekyo commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1160852657)
If I understand this test right, it checks whether the absolute fee of a smaller replacement transaction sufficiently surpasses that of a larger original transaction.

I looked around in `wallet_bumpfee.py`, and had the impression that we generally seem to assume that the replacement is the same size as the original. If we don’t check it yet, we should perhaps also add a test case where a larger replacement transaction supersedes a smaller original’s feerate sufficiently (although I might have
...

🤔 mzumsande reviewed a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374#pullrequestreview-1376434945)
ACK b5585ba5f97a19d1b435d9ab69b5a55cfd45dd70

I reviewed the code and verified that we now make outgoing connections to peers with lexicographically adjacent onion addresses.

💬 Sjors commented on pull request "[Draft / POC] Silent Payments":
(https://github.com/bitcoin/bitcoin/pull/24897#discussion_r1160888305)
The term "identity" is confusing here. It's also confusing that you get this error when you generate a new silent payment address, but not when you request an existing one (by reusing a label).

I guess what you're trying to say here is that an outside observer can tell that two silent payment addresses belong to the same person? Maybe just say that:

`The silent payment addresses are unique for accounting purposes only. They can be linked to the same wallet. For privacy a new wallet is requ
...

👍 ryanofsky approved a pull request: "refactor: Move chain names to the util library"
(https://github.com/bitcoin/bitcoin/pull/27294#pullrequestreview-1376476610)
Code review ACK f3c57bf4654599a4343cee3a1d2ffa7026a8ade7. This breaks dependency between server parameters (CChainParams in libbitcoin_kernel) and client parameters (CBaseChainParams in libbitcoin_common), so client parameters aren't dragged into the kernel.

However I think as long as every usage of the constants is changing anyway, it would be better to turn this into to enum and not introduce an odd namespace with string symbols. Suggestion would be to add a new `src/util/chain_types.h` fil
...

🤔 Xekyo reviewed a pull request: "wallet: Avoid underpaying transaction fees when signing taproot spends"
(https://github.com/bitcoin/bitcoin/pull/23502#pullrequestreview-1376476830)
Concept ACK

How would this behave in the context of changeless transactions? Would we be overestimating the target and dropping an excess to the fees? Does that get recognized when we compare coin selection solutions?