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

✅ fanquake closed a pull request: "rpc, net: add erlay status in `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/27797)

💬 MarnixCroes commented on pull request "doc: Tor: fix link & generalize onion getnodeaddresses RPC":
(https://github.com/bitcoin/bitcoin/pull/27719#issuecomment-1571677493)
@jonatack thank you! I've applied your suggestions

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1571690382)
`>>` This is ready for review and testing `<<`

> ... if the transaction is sent along with the potentially uniquely identifying UA comment, or other fingerprints in the version message. Maybe it makes sense to define a static version message ...

@MarcoFalke, I went with a [completely static `VERSION` message](https://github.com/vasild/bitcoin/blob/b73810f80917dd882d6161f6892f10f8c46a8193/src/net_processing.cpp#L1439-L1446)

> ... Perhaps it's worth mimicking `bitcoin-submittx`

@sipa,
...

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1212891740)
This, together with the other change from commit 0ec7b979e7a9d4c30a97c39c2a64768d7c5662b1 `net_processing: omit unbroadcast txs from replies to GETDATA and MEMPOOL` alter the behavior even if sensitive relay is not used (e.g. disabled or Tor and I2P not reachable).

I think it is beneficial in that case too, but is not the purpose of this PR to improve that. Should this be guarded by `if (UseSensitiveRelay())`?

👋 vasild's pull request is ready for review: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509)