💬 ChrisCho-H commented on pull request "script: throw disabled err for op_verif and op_vernotif":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1673249045)
add `"reserved"` in bad opcode err message in following commit 7ec6faff12b846255395aefa2f6dfa05bcb27bd7

💬 mzumsande commented on pull request "p2p: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1673249593)
> Is there a case where we really need to punish for this anyway? We already bypass the punishment for compact-block relaying...

IBD I think, where blocks are typically received out of order, and fully validated later. If peers send us blocks then that turn out to be invalid, we probably want to remember these peers to disconnect them.

🤔 furszy reviewed a pull request: "wallet: bugfix, disallow migration of invalid scripts"
(https://github.com/bitcoin/bitcoin/pull/28125#pullrequestreview-1571923281)
Updated per feedback. Thanks achow.

💬 furszy commented on pull request "wallet: bugfix, disallow migration of invalid scripts":
(https://github.com/bitcoin/bitcoin/pull/28125#discussion_r1290145531)
I don't think that will work for p2pk scripts.

We store the p2pk script as `PKHash` destinations inside the address book. So, by calling `GetScriptForDestination()`, the resulting script will be a p2pkh. Which will not match the watched p2pk inside `not_migrated_scripts`.

With the current `ExtractDestination()` call, we are going in the other way around: we convert the p2pk script into a `PKHash` destination, which matches the destination stored in the address book.

What could do is cre
...

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