🤔 TheCharlatan reviewed a pull request: "refactor: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/28218#pullrequestreview-1571859693)
Why not make `NotifyHeaderTip` a private `ChainstateManager` member? The PR title seems to already hint as much.

💬 achow101 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1673195541)
I think this was fixed by #28067

💬 brunoerg commented on pull request "fuzz: fix a couple incorrect assertions in the `coins_view` target":
(https://github.com/bitcoin/bitcoin/pull/28215#discussion_r1290112536)
Thanks, @dergoegge

💬 instagibbs commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-1673202074)
@sipa you might want to weigh in as well

💬 fanquake commented on issue "BIP324 tracking issue":
(https://github.com/bitcoin/bitcoin/issues/27634#issuecomment-1673203112)
Review comments from #28008 that could be incorporated into future changes:

- [ ] https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1554883653
- [ ] https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1277460913
- [ ] https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1284724692
- [ ] https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1284733846
- [ ] https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1284753030
- [ ] https://github.com/bitcoin/bi
...

💬 fanquake commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1673204546)
I locked this thread, because the discussion here was clearly not achieving anything (other than spamming the thousands of subscribers to this repository).

I'd suggest that anyone who's interested in continuing this discussion, take their thoughts to the mailing list: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-August/021840.html.

In the mean time, I think it's unlikely that Bitcoin Core is going to make any changes in regards to `OP_RETURN`. For now, I'm going to close th
...

✅ fanquake closed a pull request: "Remove arbitrary restrictions on OP_RETURN by default"
(https://github.com/bitcoin/bitcoin/pull/28130)

💬 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