💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1289400462)
No, it's not

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1289401710)
Concept ACK on improving the example usage. Any tips on how to make it work with macOS 13.5 arm64? Seeing `Undefined symbols for architecture arm64` errors at this step in both versions of this README (thanks!)

<details><summary>terminal output</summary></p>

```zsh
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake --version
cmake version 3.27.1
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ cmake -S . -B build -DLLVM_DIR=$(llvm-config --libdir)/cmake/llv
...

👋 jonatack's pull request is ready for review: "p2p: outbound network diversity improvements"
(https://github.com/bitcoin/bitcoin/pull/28248)

🤔 ariard reviewed a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1570874742)
Concept ACK

💬 ariard commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1289439494)
It could be noted what represents a “failed solve” is up to the caller. Actually `TxoutType::WITNESS_UNKNOWN` is considered as a failure in `AreInputsStandard`, as witness unknown is a special case of non-standardness for undefined segwit output.

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1672442082)
Force-pushed addressing:
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289387991
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289387061
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289386461
- https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289382284

Thanks, @furszy!

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289454868)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289454926)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289454987)
Done

💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289455057)
yea, done!

💬 fanquake commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1289566686)
Use vanilla LLVM/Clang, Not Apple LLVM/Clang.

💬 willcl-ark commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1289659607)
@jonatack took the first. @pinheadmz removed the space. Thanks.

💬 willcl-ark commented on pull request "doc: Use GitHub's "Alert" markdown syntax":
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672704282)
Thanks for the feedback, I share the worry with becoming locked in to a single vendor for our development.

IMO adding markdown syntax is not a big threat to this, but something is always more than nothing. FWIW Gitlab also [supports alerts via bootstrap](https://about.gitlab.com/handbook/markdown-guide/#colorful-sections) (so would require another change if we moved there ever), asciidoc has native admonition support, etc.

> They also make reading the source files more difficult, which is
...

💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289727862)
In 21ac6b2932b0f5ea9da4c251c65f4b88dfe8e983 "test: wallet, verify migration doesn't crash for an invalid script"

s/former/latter

💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1289724128)
In 01126667b16991b11766bbd17a73bee2505ae56f "wallet: disallow migration of invalid or not-watched scripts"

Instead of iterating over `not_migrated_scripts` for every address book record that gets here, we could instead convert the address book destination to a script and just do a lookup in the set.

```suggestion
CScript addr_script = GetScriptForDestination(addr_pair.first);
if (!addr_script.empty() && not_migrated_scripts.count(script) > 0) {

...

🤔 stratospher reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1554883653)
tested ACK 1c7582e.

tested it using [this branch](https://github.com/stratospher/bitcoin/commits/more_fuzz_202306_bip324_ciphersuite) which uses random inputs from `FuzzedDataProvider` to check whether the ciphertext produced by this PR and the python BIP code produce the same output.

I also liked the simpler current approach for streaming API changes. the interfaces of the cipher look more efficient and cleaner compared to the previous implementations!

💬 stratospher commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1283327643)
990f0f8: aad in the BIP allows upto 4095 max bytes (max garbage length). is this a performance saver too?

💬 stratospher commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1285005961)
(micro nit)
1c7582e: s/accomodate/accommodate

💬 stratospher commented on issue "fuzz: Invoking python interpreter from a C++ file":
(https://github.com/bitcoin/bitcoin/issues/22973#issuecomment-1672899107)
discussed in https://github.com/bitcoin/bitcoin/issues/23915

✅ stratospher closed an issue: "fuzz: Invoking python interpreter from a C++ file"
(https://github.com/bitcoin/bitcoin/issues/22973)