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

👍 dergoegge approved a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1571484607)
Code review ACK abe8536192c9f2cd6ba9d0e083f23dec4d20841f

💬 fanquake commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#issuecomment-1672921810)
We can track and deal with all remaining comments in one of the followup PRs.

🚀 fanquake merged a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008)

💬 dergoegge commented on pull request "p2p: outbound network diversity improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1672930044)
The title "p2p: outbound network diversity improvements" makes it sound like you are improving on the logic for making outbound connections but really you are refactoring and adding additional logging. Would you mind making the title more accurate?

> Please see the commit messages for details.

The PR description is added to the merge commit, so in my opinion it makes sense to have at least a summary of what the PR does in the description.

From our [docs](https://github.com/bitcoin/bitco
...