💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1284001844)
Because `ForEachNode` takes a `std::function`, the thread safety annotations on the lambda are dropped, so the compiler can't verify that `ForEachNode` has actually taken all the locks that the lambda expects to have held. Problem is that doing it any other way (eg templates) requires `ForEachNode` to be annotated to require any extra locks that the lambda requires that were already held by the caller.

💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1665042245)
Just for context: For testing I used `-datacarriersize=9999999` and then put the following nulldata into the `mempool.dat`: `raw(6a4458354f2150254041505b345c505a58353428505e2937434329377d2445494341522d5354414e444152442d414e544956495255532d544553542d46494c452124482b482a)`. On current master I got several hits by different virus scanners. On this pull, all virus scanners were green and didn't put the `mempool.dat` in the quarantine.

💬 eriknylund commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284003587)
@Sjors Given my basic understanding of output descriptors, I'd much appreciate your thoughts on this block of code, to make sure it meets the issue criterias and if this is a good way to test it.

💬 BrandonOdiwuor commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1284001548)
To improve maintainability and support forward compatibility, consider abstracting the handling of different versions into a separate helper function like ‘[evalCheckSig](https://github.com/bitcoin/bitcoin/blob/a4ca4975880c4f870c6047065c70610af2529e74/src/script/interpreter.cpp#L391)’ does. This approach will allow for easier modification and extension for new versions of Silent addresses, and would also be more clear how version handling is done

💬 paplorinc commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#discussion_r1284067105)
isn't it weird that the error message states "Cannot have 1000 keys" when k is not 1000?

💬 MarcoFalke commented on pull request "test: verify spend from 999-of-999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/28212#issuecomment-1665127576)
The linter is failing. Also, please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1284072810)
Thanks, fixed CI

👋 MarcoFalke's pull request is ready for review: "mempool: Persist with XOR"
(https://github.com/bitcoin/bitcoin/pull/28207)

👍 MarcoFalke approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1562333801)
re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 👠

<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: re-ACK 1c976c691cc4b20f430
...

👍 vasild approved a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213#pullrequestreview-1562352316)
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678

💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1284103954)
changed back to array and this was not added back but it is not needed because the array is default-initialized: `std::array<...> foo = {};`

💬 vasild commented on pull request "rpc: add 'getnetmsgstats', new rpc to view network message statistics":
(https://github.com/bitcoin/bitcoin/pull/27534#issuecomment-1665196395)
@luke-jr, for sure there will be people that don't need this. But it is just an extra RPC, if somebody does not need it, then he/she will not call it. Similar stats are already provided in the `getpeerinfo` RPC output.

💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1665223818)
ACK 1e65aae806

Updates shown by `git range-diff 1e65aae806...1b52d16` look correct to me and glad to get the nits in this initial pull.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665228548)
> All other comments are to be addressed shortly.

Are you still working on this? Apart from a squash and addressing the 4 review comments this is rfm, no?

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1665232531)
Rebased, and added one more line of documentation

💬 hebasto commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1665233643)
> > All other comments are to be addressed shortly.
>
> Are you still working on this?

I am. My apologies for a delay.

💬 naumenkogs commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1665244546)
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678

💬 naumenkogs commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1284155363)
1e9684f39fba909b3501e9402d5b61f4bf744ff2
"w hether"

👍 naumenkogs approved a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1562445761)
ACK 1e9684f39fba909b3501e9402d5b61f4bf744ff2

💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#discussion_r1284186407)
I understand what you intended but it does not seem to be interpreted in that way for `pop two top stack items`. `pop pop add push` is more confusing to me. I appreciate your feedback