💬 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

🤔 jonatack reviewed a pull request: "doc: Improve documentation of rpcallowip"
(https://github.com/bitcoin/bitcoin/pull/27480#pullrequestreview-1572270154)
ACK

Non-blockers, consider changing the commit message to something like `doc: update help for -rpcallowip config option` (it's not an RPC help), and changing the two `#21070` to `PR 21070` so GitHub doesn't add mentions in that issue on every push.

💬 jonatack commented on pull request "doc: Improve documentation of rpcallowip":
(https://github.com/bitcoin/bitcoin/pull/27480#discussion_r1290372278)
```suggestion
argsman.AddArg("-rpcallowip=<ip>", "Allow JSON-RPC connections from specified source. Valid values for <ip> are a single IP (e.g. 1.2.3.4), a network/netmask (e.g. 1.2.3.4/255.255.255.0), a network/CIDR (e.g. 1.2.3.4/24), all ipv4 (0.0.0.0/0), or all ipv6 (::/0). This option can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
```

💬 furszy commented on issue "wallet_importdescriptors.py: can't decode bytes in position 228861-228863: unexpected end of data":
(https://github.com/bitcoin/bitcoin/issues/28030#issuecomment-1673537303)
#28035 was merged, can this be closed?

📝 dergoegge opened a pull request: "[no merge, meta] refactor: net/net processing split"
(https://github.com/bitcoin/bitcoin/pull/28252)
This PR is supposed to provide context for some of the refactoring PRs I've been working on (#25268, #26621, etc).

The aim is to completely decouple CConnman and its internals from PeerManager to allow for isolated testing of our message processing code (isolated from net only as isolating from validation is another can of worms). To get there, this work refactors CConnman's API to use `NodeId` as the primary handle for managing connections and defines a new interface `ConnectionsInterface` t
...