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

💬 ryanofsky commented on pull request "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#issuecomment-1673234551)
re: https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1571859693

> Why not make `NotifyHeaderTip` a private `ChainstateManager` member?

Because it doesn't access private state. I think the point of classes in c++ is to encapsulate private state. If there is a group of functions operating on public state, they should be standalone functions, so they can be split up into files and organized by functionality. This way we can avoid being permanently stuck with a sprawling mess of
...

💬 ChrisCho-H commented on pull request "fix: bad opcode err msg includes reserved opcode":
(https://github.com/bitcoin/bitcoin/pull/28234#issuecomment-1673244113)
closed as it would be better to integrate this update here https://github.com/bitcoin/bitcoin/pull/28169

✅ ChrisCho-H closed a pull request: "fix: bad opcode err msg includes reserved opcode"
(https://github.com/bitcoin/bitcoin/pull/28234)

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