💬 glozow commented on pull request "Add benchmark for miniminer":
(https://github.com/bitcoin/bitcoin/pull/28152#issuecomment-1673300954)
cc @murchandamus

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1673321815)
Concept ACK

However 34aca2d5f26f0441a366942dc56bd220e806d63c seems to move in the exact opposite direction of #26291. Thoughts @ajtowns & @instagibbs? We can easy move that constant again though.

Otherwise code review ACK 3a6e49e80fb6c1a7b0ed792cd0611545df63d3da.

I dream of a future where `validation.cpp` is split between consensus and policy (and the former doesn't depend on `policy.h`).

What do kernel folks think of this separation? cc @TheCharlatan

Nit: maybe rename `addresstyp
...

💬 dergoegge commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#issuecomment-1673322410)
Closing for now, I want to do some interface work before adding the tests and will open a large meta PR soon that includes all the refactoring + tests at the end.

✅ dergoegge closed a pull request: "net, test: Virtualise CConnman and add initial PeerManager unit tests"
(https://github.com/bitcoin/bitcoin/pull/25515)

👍 Sjors approved a pull request: "Break up script/standard.{h/cpp}"
(https://github.com/bitcoin/bitcoin/pull/28244#pullrequestreview-1571575185)
Concept ACK

However 34aca2d5f26f0441a366942dc56bd220e806d63c seems to move in the exact opposite direction of #26291. Thoughts @ajtowns & @instagibbs? We can easy move that constant again though.

Otherwise code review ACK 3a6e49e80fb6c1a7b0ed792cd0611545df63d3da.

I dream of a future where `validation.cpp` is split between consensus and policy (and the former doesn't depend on `policy.h`).

What do kernel folks think of this separation? cc @TheCharlatan

Nit: maybe rename `addresstyp
...

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1289917821)
6ec6e5e0e8390d9dca77dfc64bd273860bdbdd93 nit: drop `(see script.h)`

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290113970)
582708b9e0003c1dbe0b4bec1aada8dde9feed24: can be `const` (seems to compile fine).

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1290108557)
582708b9e0003c1dbe0b4bec1aada8dde9feed24: Note to other reviewers: we don't want to move `ScriptHash` into `script.h` because it has no business in consensusland. Since the constructor here needs `ScriptHash`, dropping it avoids the circular dependency.

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#discussion_r1289950752)
3a6e49e80fb6c1a7b0ed792cd0611545df63d3da: this is the only file whose scope feels a bit ambiguous. One might be tempted to think it's a wallet thing, but `policy.h` uses it too. Suggested comment:

```cpp
// Solver methods are used by both policy and the wallet, but not consensus.
```

Ideally we'd cleanly split policy and wallet related stuff, but this warning should at least prevent someone accidentally changing policy when working on the wallet.

💬 Sjors commented on pull request "Break up script/standard.{h/cpp}":
(https://github.com/bitcoin/bitcoin/pull/28244#issuecomment-1673353862)
Maybe cherry-pick 461259c4ecc1e48d028eaea56061e72a6667ce4f instead of 34aca2d5f26f0441a366942dc56bd220e806d63c. Seems to compile and pass all tests.

💬 theuni commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290259830)
Hmm, LLVM should be header-only, so I think this is indicative of some real problem.

Could you please paste your link line generated by `make VERBOSE=1`? For Linux mine is:
`/usr/bin/c++ -fPIC -O3 -DNDEBUG -shared -o libbitcoin-tidy.so "CMakeFiles/bitcoin-tidy.dir/bitcoin-tidy.cpp.o" "CMakeFiles/bitcoin-tidy.dir/logprintf.cpp.o"`

---

A few possibilities:
1. Something about the way it's being compiled/linked makes ld64 grumpy.
2. Something related to [this magic line](https://github.
...

🤔 dergoegge reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1572046251)
The net processing changes look much better (no chainstate pointers on CNodeState, yay!).

💬 dergoegge commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1290225497)
This look suspiciously similar to `FindNextBlocksToDownload`, is there no way to unify the two?

(sorry if I missed previous discussion on this)

💬 dergoegge commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1290241176)
propbably not worth it to pass the chainstate role then?

💬 dergoegge commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1290237668)
`NewPoWValidBlock` is already gated with `m_highest_fast_announce` but I think it would make sense to also ignore bg chainstate events explicitly.

💬 jonatack commented on issue "Raise maximum -dbcache setting":
(https://github.com/bitcoin/bitcoin/issues/28249#issuecomment-1673442897)
There is some related discussion and testing in https://github.com/bitcoin/bitcoin/pull/6102 containing the commit you linked to and more recently in https://github.com/bitcoin/bitcoin/pull/25325.

🤔 instagibbs reviewed a pull request: "validation: avoid mempool eviction mid-package evaluation"
(https://github.com/bitcoin/bitcoin/pull/28251#pullrequestreview-1572237699)
conditional ACK https://github.com/bitcoin/bitcoin/pull/28251/commits/c2c9dfe95b3c5b8332521d9a691725ee55ec3450

on having a test case catching the crash included. Tested with my own test case in https://github.com/bitcoin/bitcoin/pull/26403/commits/e3622f7ec944ec0e4c48ec3f29835d9f90d9d55c

💬 instagibbs commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290355065)
was this always supposed to be true?

💬 instagibbs commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290350524)
Declaration comments for `SubmitPackage` should reflect that it doesn't limit anymore, or just not mention limiting.

💬 jonatack commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#discussion_r1290358956)
Thank you @fanquake and @theuni. I think I'm using clang/llvm from homebrew (and have `export PATH="/opt/homebrew/opt/llvm/bin:$PATH"` in `.zshrc`) but could be missing something. Tried 1. and 2. above and still see `Undefined symbols for architecture arm64` at the second step.

<details><summary>output of <code>make VERBOSE=1</code></summary><p>

```bash
jon|master =:~/bitcoin/bitcoin/contrib/devtools/bitcoin-tidy$ clang --version
Homebrew clang version 16.0.6
Target: arm64-apple-darwi
...

🤔 furszy reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1572286861)
utACK 1942ea0