💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#discussion_r1274732379)
Agreed, makes more sense. Will move down if I have to retouch.

💬 stickies-v commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651491901)
> I'm not sure that I love using a subsystem's opts as a cache for init vars.

I felt iffy about it too ([as per my initial suggestion](https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268400386)), it's not the most elegant. I do think using a single var is safer and more clear than having a separate `ignores_incoming_txs` var, but I'm happy to drop the second commit if that's what people prefer.

💬 fanquake commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#issuecomment-1651515949)
> @fanquake could you provide more info on the methodology to reproduce your data?

@jonatack I'm just running the preprocessor and comparing the to-be-compiled output. You could inject it into the compile command from `V=1`. i.e something like `/usr/bin/ccache g++ -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/home/ubuntu/bitcoin=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -DHAVE_BUILD_INFO -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univ
...

💬 MarcoFalke commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1651531909)
You could add back the `const bool ignores_incoming_txs{peerman_opts.ignore_incoming_txs};` alias? But I don't see what is iffy about the code. Are you saying it would be cleaner for `node.fee_estimator` to be constructed after the peerman, and using the peerman property instead of the peerman options property?

🤔 stickies-v reviewed a pull request: "refactor: Revert additional univalue check in ParseSighashString"
(https://github.com/bitcoin/bitcoin/pull/28162#pullrequestreview-1547452713)
Approach ACK on the release notes updates (nice catch MarcoFalke), not so sure about the code changes yet.

💬 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