💬 furszy commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1290158584)
done

💬 achow101 commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#issuecomment-1673265402)
ACK 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9

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

TBH, I’m not super familiar with the files that are being touched, but I verified that it’s almost exclusively code moves, and looked over the minuscule other changes (like adding and removing includes, extracting something into a function).

👍 TheCharlatan approved a pull request: "refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1571971717)
ACK bda557e5ebe7f754984a34ddfd33d2540c0303b9

💬 glozow commented on pull request "policy: nVersion=3 and Package RBF":
(https://github.com/bitcoin/bitcoin/pull/25038#issuecomment-1673294258)
Rebased on top of #28251 for the issue posted in #26403

👍 theStack approved a pull request: "test: locked_wallet, skip default fee estimation"
(https://github.com/bitcoin/bitcoin/pull/28232#pullrequestreview-1572001513)
ACK 5364dd8666ae1afa35536f9b4fc0170eeaf37888

💬 joostjager commented on pull request "policy: make unstructured annex standard":
(https://github.com/bitcoin/bitcoin/pull/27926#issuecomment-1673299204)
Clearly data backed up in this way isn't guaranteed forever and forever free. However, for specific users this might still represent the most feasible option available. At the very least chain backup can serve as a safety net in case a conventional backup that is fully under control of the user somehow gets lost or becomes inaccessible.

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