💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1672291591)
Force-pushed:

- Improved `Merge`: now it checks the input set and it tries to merge every solution with a "manual" one.
- Improved `AddInputs` to check the total weight.

💬 jonatack commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1672297168)
Seeing these failures locally:

```
$ ./src/test/test_bitcoin -t denialofservice_tests
Running 5 test cases...
Assertion failed: lock m_nodes_mutex not held in net.cpp:1598; locks held:
'cs_main' in net_processing.cpp:5203 (in thread 'test')
unknown location:0: fatal error: in "denialofservice_tests/stale_tip_peer_management": signal: SIGABRT (application abort requested)
test/denialofservice_tests.cpp:182: last checkpoint
Assertion failed: (ret.second), function AddArg, file args.cpp,
...

💬 ariard commented on pull request "doc: Use GitHub's "Alert" markdown syntax":
(https://github.com/bitcoin/bitcoin/pull/28243#issuecomment-1672297394)
+1 Sooner we’ll move out of Github better we’ll be and we would be rather to prepare for greater disruptions of GH availability (e.g regularly backing up the decade of comments on the repo) than rely more on its features.

💬 brunoerg commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#issuecomment-1672302859)
Concept ACK

🤔 amitiuttarwar reviewed a pull request: "p2p: Drop m_recently_announced_invs bloom filter"
(https://github.com/bitcoin/bitcoin/pull/27675#pullrequestreview-1555948320)
review ACK fb02ba3c5f5

it's been a while since I've hung out with mempool transaction code, so in addition to reading through the code I spent a good chunk of energy refreshing contextual behaviors. some context behind the ack:
- thinking through if there is a meaningful shift in privacy model: I find these changes acceptable
- understanding the memory tradeoff: this approach makes sense to me, esp with #27630 in the pipeline
- re-familiarizing myself with how transactions enter the mempo
...

💬 amitiuttarwar commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1279974340)
is this used anywhere?

💬 amitiuttarwar commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1280016445)
out of curiosity, why did you opt to make a separate function instead of having `last_sequence` be an optional field on `info`? is it to make it more apparent to future callers (aka devs implementing new call sites) that `last_sequence` must be passed in when retrieving a `TxMempoolInfo` with intent to relay?

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289385441)
nit: could verify the exact weight by summing up all the utxos weight.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289386461)
can't be negative. It's a ranged amount.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289382284)
`continue`?

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289385038)
`continue`?

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289387061)
The manual selection is always the same. Could place it outside of the loop.

💬 furszy commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1289387991)
```c++
assert(result->GetInputSet().size() == input_set.size() + manual_utxos.size());
```

💬 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