💬 furszy commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213702653)
Could:
```c++
for (const DatabaseFormat& db_format : DATABASE_FORMATS) {
dbs.emplace_back(MakeDatabase(path_root / strprintf("%d", db_format).c_str(), options, status, error));
BOOST_ASSERT(status == DatabaseStatus::SUCCESS);
}
```

💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213849864)
> Noticed that this isn't covered by the tests.

If this is true, it seems like a bug in the test because two of the CheckPrefix lines were written specifically to cover this case.

https://github.com/bitcoin/bitcoin/blob/ba616b932cb9e9adb7eb9f1826caa62ce422a22d/src/wallet/test/db_tests.cpp#L204-L205

I think there is no need for a RawWrite method because spans of bytes should be serialized by just appending the bytes.

💬 furszy commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213883772)
oh well, I noticed while was checking the first test, and went deeper without noticing the second test existence :man_facepalming:.
What a day.. forget all what I said above. Thanks for the quick heads up.

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