💬 LarryRuane commented on pull request "util: generalize accounting of system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1212354487)
Thanks, I got a slightly different result -- `malloc_usable_size() + sizeof(void*)` is perfect (note adding only `sizeof(void*)` instead of 2 times that). I'm not set up for 32-bit, can you try running this test program on 32-bit if you are able to easily?

Whichever is the most accurate, would a PR to improve [`memusage.h: MallocUsage()`](https://github.com/bitcoin/bitcoin/blob/6cf47a8f44f96099a84e5c6e628e3499045e024d/src/memusage.h#L50) be worth doing? (I would say a separate PR would be bet
...

📝 theuni opened a pull request: "depends: modernize clang flags for Darwin"
(https://github.com/bitcoin/bitcoin/pull/27798)
This is a cleaner and simpler alternative to #25098. Inspired by [this conversation](https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562543301). The diff is large but the change itself is quite small.

Fixes builds with llvm >= 11 in guix by working around the problem. As a bonus, this is much cleaner and more maintainable than what we had before.

See the updated comment for more info. At a high level: rather than playing tricks and trying to work around clang's default includes
...

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1571040493)
> > just need to decide out what to do about guix.
>
> What options are currently being considered. Is an upstream patch still possible?

See #27798 for an alternative that is imo better in every way :)

💬 mzumsande commented on pull request "p2p, rpc, test: `getpeerinfo` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1212373107)
The title and commit messages don't fit: This PR changes concerns `addnode`, not `getpeerinfo`.

💬 hebasto commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#issuecomment-1571046189)
Concept ACK.

💬 brunoerg commented on pull request "p2p, rpc, test: `addnode` improv + add test coverage for invalid command":
(https://github.com/bitcoin/bitcoin/pull/26366#discussion_r1212420718)
Ooops, I think things changed during the way, thanks for notice it! Done!

💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1571334843)
It looks like the conversion table is missing the arguments that take amounts; these also need to be converted. The test is also failing as the named only arguments are not being output in the `dump_all_command_conversions`, the regex can't handle negatives, and `psbt` has mismatched conversions.

The following diff should fix those issues:

```diff
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index be4d777169..7bd66ebbd2 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp

...

💬 torkelrogstad commented on pull request "wallet: clarify replace fields in help output":
(https://github.com/bitcoin/bitcoin/pull/27782#issuecomment-1571408030)
Thanks for pointing that out @brunoerg. Updated the commit message and PR title.

💬 MarcoFalke commented on pull request "depends: modernize clang flags for Darwin":
(https://github.com/bitcoin/bitcoin/pull/27798#discussion_r1212642042)
from CI:

```
clang: error: argument unused during compilation: '-nostdlibinc' [-Werror,-Wunused-command-line-argument]

💬 MarcoFalke commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212664168)
Is the same not true for span itself? The only reason it doesn't work is that it is missing a `std::byte` specialization?

https://github.com/bitcoin/bitcoin/blob/3a83d4417b35cb0173286b6da97315be861901bc/src/serialize.h#L202-L206

💬 MarcoFalke commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212665991)
```suggestion
DataStream value;
```

nit (feel free to ignore)

💬 MarcoFalke commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1571457996)
See for example https://github.com/achow101/wallet-fingerprinting/blob/main/fingerprints.md

💬 jonatack commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#issuecomment-1571580341)
Thank you for the review feedback, @MarcoFalke; addressed, and rebased to current master containing [`5f49cb1` (#25977)](https://github.com/bitcoin/bitcoin/pull/25977/commits/5f49cb1bc8e6ba0671c21bf6292d2d3de43fd001) to use `Result` with support for `<void>`. Invalidates previous ACK by @vasild (thanks!)

⚠️ MarcoFalke opened an issue: "fuzz: mini_miner: Timeout in mini_miner"
(https://github.com/bitcoin/bitcoin/issues/27799)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59285

_Originally posted by @MarcoFalke in https://github.com/bitcoin/bitcoin/issues/27021#issuecomment-1568242621_

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212835700)
> we should make sure that there aren't alternative clients that would ignore unrequested TXs

There is a precedent: from https://developer.bitcoin.org/reference/p2p_networking.html#tx

> [BitcoinJ](http://bitcoinj.github.io/) will send a [“tx” message](https://developer.bitcoin.org/reference/p2p_networking.html#tx) unsolicited for transactions it originates

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212837257)
For now I kept it to send `TX` right away as this is simpler and somebody blackhole-ing the transaction is ok.

💬 fanquake commented on issue "Erlay status in getpeerinfo":
(https://github.com/bitcoin/bitcoin/issues/26602#issuecomment-1571656524)
Closing for now. The comment here: https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1570503893, is a good summary of why we aren't going to do this. You can run with the patch from that PR, or look at the p2p log.

✅ fanquake closed an issue: "Erlay status in getpeerinfo"
(https://github.com/bitcoin/bitcoin/issues/26602)

💬 fanquake commented on issue "fuzz: mini_miner: Timeout in mini_miner":
(https://github.com/bitcoin/bitcoin/issues/27799#issuecomment-1571665679)
cc @Xekyo

💬 fanquake commented on pull request "rpc, net: add erlay status in `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/27797#issuecomment-1571669565)
I think you could leave a comment in #21515, about cherry-picking this change into that PR, but I don't think there's a need to leave this PR open, if we aren't going to merge it. I agree with Martins comment above; it's premature to add this, and probably not something we should add to our RPC interface just as a convenience for developers, for testing a not-yet-implemented/experimental feature.