💬 stickies-v commented on pull request "refactor: Revert additional univalue check in ParseSighashString":
(https://github.com/bitcoin/bitcoin/pull/28162#discussion_r1274785751)
I don't think catching runtime errors here is the best approach. We never expect this to fail at runtime, only if the function is used the wrong way by a programmer. Will this not unnecessarily hide true bugs?

I think avoiding dead code makes sense, but I also like the function being self-contained and self-descriptive like this without too much overhead, so I guess overall I'm ~0 on the change.

As an alternative, how about instead of catching `std::runtime_error&`, we only call `ParseSigh
...

💬 dergoegge commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274865617)
No strong opinion, but it would be perfectly fine to just do
```suggestion
if (!args.GetBoolArg("-blocksonly", DEFAULT_BLOCKSONLY)) {
```
here and then keep the peerman opts were they are right now.

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