💬 ajtowns commented on pull request "net_processing: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1573150818)
> What's the rationale regarding optimizing for memory instead of privacy here? I can see how this simplifies the design, however, I fail to see this being an obvious pick given we'd be potentially even more exposed to fingerprinting than before (as is now it is trivial for every connection to probe data on the node's mempool).

This isn't trading off privacy -- that's the point of [this comment](https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1549711784). It does mean that someone
...

💬 MarcoFalke commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213968128)
> But I think I would still like to avoid overloading Serialize for std::byte spans. The whole point of std::byte is that it's supposed to be a very safe type which requires you to be deliberate and explicit about conversions.

Agree on this. For raw C-Arrays where the length is denoted in the type, it shouldn't cause any issues. Probably the same for C++20 spans that are fixed size (but our `Span` doesn't have that feature).

> No because `Span<char>` is fixed length

Are you sure? One ca
...

👍 MarcoFalke approved a pull request: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800#pullrequestreview-1456744923)
Makes sense even absent the confusion, because Span also avoids a temporary copy to a `std::vector` (in DataStream).

lgtm ACK 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 🌙

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw
...

💬 MarcoFalke commented on pull request "streams: Drop confusing DataStream::Serialize method and << operator":
(https://github.com/bitcoin/bitcoin/pull/27800#discussion_r1213984067)
`+=` removed in faa96f841fe45bc49ebb6e07ac82a129fa9c40bf

💬 kylezs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1573276009)
> If your test isn't concerned with the fee estimator's exact behavior, as @instagibbs says, you can mock it or load a placeholder fee_estimates.dat file to populate the estimator with data.

This might be the way to go, is there one that exists that can be downloaded and loaded in? I don't really mind what the exact behaviour of the estimator is.

💬 ajtowns commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1573281471)
I've cleaned up and rebased this as https://github.com/ajtowns/bitcoin/commits/202306-dropmaprelay if that's any help.

💬 ccdle12 commented on pull request "net, refactor: extract Network and BIP155Network logic to node/network":
(https://github.com/bitcoin/bitcoin/pull/27385#issuecomment-1573387265)
> Rebased 3rd_place_medal
>
> @ccdle12 Thanks! Mind re-acking? As to motivation, I think it's a mix of both reasons you mentioned, with the first one being the direction to go when doing so -- see `doc/design/libraries.md` and merged changes like [c741d74](https://github.com/bitcoin/bitcoin/commit/c741d748d4d9836940b99091cc7be09c65efcb79).

Of course! re-tACK at 8efd76b, thanks for explaining the motivation and pointing out the design/doc :)

💬 willcl-ark commented on issue "Remove Ambiguity of Script ASM Hex and Decimal Integer Representations":
(https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1573396315)
It's a little unclear to me how the ASM output is used on the consumer side, and therefore how breaking any changes to it might be. However I still think we have a few options to resolve this:

1) Fully hex-encoded output in the ASM repr:

```shell
$ /home/will/src/bitcoin/src/bitcoin-cli -regtest decodescript 0512121212120457c74942
{
"asm": "1212121212 57c74942",
```

🚀 fanquake merged a pull request: "Update .style.yapf"
(https://github.com/bitcoin/bitcoin/pull/27802)

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1573408760)
`b6fd6b525d...92fb45b5ef`: rebase and address suggestions

🚀 fanquake merged a pull request: "[25.x] build: disable boost multi index safe mode"
(https://github.com/bitcoin/bitcoin/pull/27775)

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214126425)
Added this test, but in `feature_config_args.py` because it checks interaction between some config args. That fits better than `p2p_local_tx_relay.py`:
> Test how locally submitted transactions are sent to the network when sensitive relay is used.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214127166)
Added a comment in the code in case somebody else wonders the same.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214128768)
I removed `new_p2p_index()` because it was used in just one place. Earlier it was used also for all listeners behind the SOCKS5 proxy, but I had to stop using `p2p_port()` for that because it exceeded `MAX_NODES` (12).

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1214131440)
Yes. That is a bit hidden/implicit in the creation of the grant, in `master`:

```cpp
CSemaphoreGrant grant(*semOutbound);
```

this would hang if there are too many connections, waiting for a free slot to be available (somebody to disconnect).

Added a mention to it in the description of `-maxconnections`.

🚀 fanquake merged a pull request: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800)

💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1573450367)
Using alternate distros, or the pre-compiled LLVM bins do not currently seem to be viable options. Follow up in the qa-assets repo is here: https://github.com/bitcoin-core/qa-assets/pull/129.

🚀 fanquake merged a pull request: "ci: compile Clang and compiler-rt in msan jobs"
(https://github.com/bitcoin/bitcoin/pull/27737)

🤔 glozow reviewed a pull request: "Fuzz: Mitigate timeout in CalculateTotalBumpFees"
(https://github.com/bitcoin/bitcoin/pull/27803#pullrequestreview-1457035240)
ACK 5d718f6913219d3ebe8394a17ddee81915e6f0ac

The code changes make sense to me. Reproduced the slow input from #27799, and it is dramatically faster with this change.

✅ glozow closed an issue: "fuzz: mini_miner: Timeout in mini_miner"
(https://github.com/bitcoin/bitcoin/issues/27799)