💬 hebasto commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1651688197)
> Or maybe recommend that in the org (or repo) settings the permission is lowered to `restricted` just to be safe?

Here:

- Check "Read repository contents and packages permissions
- Uncheck "Allow GitHub Actions to create and approve pull requests

💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1651744555)
> Concept ACK, but I'm not too sure about the Wtxid type.

Could you clarify your objections about the `Wtxid` type? This PR aims to catch type confusion bugs (confusion between txids and wtxids) at compile-time and I don't see how to do that without these explicit types.

💬 MarcoFalke commented on pull request "test: Avoid intermittent issues due to async events in validationinterface_tests":
(https://github.com/bitcoin/bitcoin/pull/28150#issuecomment-1651767856)
Interestingly, I can *not* reproduce this easily with `rr`. I tried:

```
while rr record --chaos ./src/test/test_bitcoin -t validationinterface_tests/unregister_all_during_call ; do true ; done

💬 MarcoFalke commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1651776726)
Ok, closing for now. Let us know if this should be reopened.

✅ MarcoFalke closed an issue: "Bitcoin Core v25.0 Crashes"
(https://github.com/bitcoin/bitcoin/issues/28119)

💬 MarcoFalke commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1274946418)
Agree as well.

I think it is fine to only add `[[nodiscard]]` to functions where it is *harmful* to drop the result. Otherwise one ends up adding the attribute to *every* function that isn't `void`, which seems bloaty and unreadable?

If that was the goal, it could be easier achieved with a clang-tidy plugin, without touching or bloating any code at all.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1651836376)
> To get an idea about numbers: FSChaCha20Poly1305 on my Ryzen 5950X takes ~1.6 ns/byte, which would be 3.2 ms for a 2 MB block.

I was curious how this (additive?) 3.2ms delay compares to existing block connection latency, so I sampled some logs from some of the okayish-CPU-but-well-networked machines I run. The [`Connect block: XXms` logs average to 67.8ms](https://gist.github.com/jamesob/2038a3ba3322726348f7479d4ac3e8e5) from that sample (after stripping out the long startup outliers). So f
...

💬 MarcoFalke commented on pull request "refactor: Make more transaction size variables signed":
(https://github.com/bitcoin/bitcoin/pull/28059#issuecomment-1651846152)
lgtm ACK 92de74ef181b42d774bc6b12329bc0c27caf0081 🥔

<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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 92de74ef181b42d
...

💬 pinheadmz commented on pull request "include verbose debug messages in testmempoolaccept reject-reason":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-1651847491)
> Why not pass the details in `m_debug_message` and add that as a new field?

My thinking was to match exactly the single combined string returned by `sendrawtransaction`. But if adding the new field gets your ACK @luke-jr I'll make the change ;-)

💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1275032616)
Yeah, that would be my approach if not the current one. But going to keep as is for now since everyone seems to be happy with the current approach.

💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651909353)
> But I don't see what is iffy about the code.

Using a `PeerManager::Options` object to initialize fee estimation is what irks me a bit. I suppose using peerman instead of the options object would be a bit more elegant indeed, but not worth the code rearrangement atm.

Looks like everyone's happy with the approach as is, so am going to refrain from further changes for now.

💬 MarcoFalke commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1275046063)
Is this faster than the setup in `./src/wallet/test/fuzz/notifications.cpp`? If yes, could switch that one over as well?

👍 john-moffett approved a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127#pullrequestreview-1547929186)
ACK fa9108f85afdc926fd6a8b96cc2acff7ca25d7a8

👋 MarcoFalke's pull request is ready for review: "ci: Move ASan USDT to persistent_worker"
(https://github.com/bitcoin/bitcoin/pull/28161)

💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1651962091)
@jamesob Thanks for your comments, I think I largely agree.

It's a good observation that the case where a large non-compact block message is necessary for block connectivity is already fairly pessimal. Some more points to put this overhead in perspective:
* If you assume - say - a 1 Gbit/s connection, then receiving a byte takes 8 ns already, so adding 1.6 ns at most adds ~20% (regardless of whether the received data is short or big), and this ignores any other processing on top for the new
...

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#issuecomment-1651975910)
cc @0xB10C about the second commit

💬 stickies-v commented on pull request "net processing: clamp PeerManager::Options user input":
(https://github.com/bitcoin/bitcoin/pull/28149#discussion_r1275110654)
I'm not really keen on setting limits. I don't know what good values would be, and ultimately it's not a huge problem if the value is too high. I think issuing a warning could be sensible as it does have meaningful impact, but I don't have a strong view. Too many warnings/popups is also not great UX.

For example, this would log a warning in the concole, and on qt issue a warning pop-up if a value > 100x default value is entered:

```diff
diff --git a/src/node/peerman_args.cpp b/src/node/pe
...

🚀 fanquake merged a pull request: "refactor: Remove C-style const-violating cast, Use reinterpret_cast"
(https://github.com/bitcoin/bitcoin/pull/28127)

💬 pinheadmz commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1651999459)
Due to the multiple datadirs in the `feature_config_args` test, this assertion actually *does* fail in `combine_logs.py`:

https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86

I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1652004878)
> This is I think the biggest concern with a streaming API: the caller immediately gets (parts of) the decrypted message back, but is not allowed to use/inspect them in any way before the full message has been processed.

So just to be sure I understand, the risk is not any additional space requirement (since we need to buffer the full decrypted message anyway as things stand right now), but rather the risk of streaming is that the caller of this API will make some unwise use of a partially pr
...