💬 ryanofsky commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096161473)
In commit "validation: only create a CCheckQueueControl if it's actually going to be used" (bcde9153012b2ce3e9ff5b7cd1c6f9866453e727)

Developer notes want single-line if or multiple line if with braces, not multiline if without braces.

> - If an `if` only has a single-statement `then`-clause, it can appear
> on the same line as the `if`, without braces. In every other case,
> braces are required, and the `then` and `else` clauses must appear
> correctly indented on a new line.
...

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096234268)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: could `Assert` this condition instead, as otherwise the logic suggests that it's possible that neither of the if-branches are executed (it's not, IIUC; the only reason to not have a cached aggregate pubkey at this point is if we have ranged participants, so this condition is always true)
```suggestion
} else {
Assert(IsRangedParticipants());
```

💬 theStack commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096247242)
in commit 048c95ea557cf81a8e55c5ffe9994cc9d8f3e039: not too fond of "reverse-hexstrings of byte-strings" creeping in, could do instead
```suggestion
using namespace util::hex_literals;
constexpr std::array<uint8_t, 32> MUSIG_CHAINCODE{"868087ca02a6f974c4598924c36b57762d32cb45717167e300622c7167e38965"_hex_u8};
```
and assign the constant to the uint256 instance via `... = uint256(MUSIG_CHAINCODE)`.
(but shouldn't be a blocker, can be improved in a follow-up as well, if others are even as a
...

💬 Sjors commented on issue "Awesome multisig PR labyrinth guide":
(https://github.com/bitcoin/bitcoin/issues/24861#issuecomment-2891856702)
The MuSig2 party is now at #31244.

Meanwhile it's still very tedious to setup a multisig wallet, MuSig2 or otherwise. Suggested review to make progress is #29136 which introduces a `unused(KEY)` descriptor.

Building on that, I think we need a `gethdkey` (singular) RPC that can derive any xpub for an `unused(KEY)`, in particular `m/87'/0'/0'`.

After that we need to make the wallet a bit smarter when it imports a descriptor that contains an xpub for which it has the master key.

The final piece
...

💬 ryanofsky commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891869792)
re: https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891771230

> > All these things together suggest to me that the Mining interface should probably be exposing the CBlockTemplate struct directly and not hiding it behind so many accessor methods.

Sorry, I did not write that very clearly. I am not suggesting dropping the `interfaces::BlockTemplate` class and only exposing the `CBlockTemplate` struct. I am just suggesting adding a `const CBlockTemplate*` accessor to the interface
...

👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2840890233)
Code review ACK 30a94b1ab9ae850d55cb9eb606a06890437bc75e. Only changes since last review are using WriteBestBlock method more places and updating comments.

It looks like there are some small review comments above that could be followed up, but with 3 acks I think it would be good to merge this shortly and let smaller improvements can be made later. I plan to merge this soon after testing locally.

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2096279905)
In commit "wallet: Update best block record after block dis/connect" (7bacabb204b6c34f9545f0b37e2c66296ad2c0de)

IMO would be good to assert `!loc.IsNull()` as a sanity check to ensure that doesn't happen. (It shouldn't, but node or wallet code could change in the future and invalidate current assumptions.)

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2089361928)
Could also be good to check `!loc.IsNull()` in this function like that code is doing. (Could assert it or just not write to the database if it is null.)

💬 Sjors commented on pull request "Mining: Avoid copying template CBlocks":
(https://github.com/bitcoin/bitcoin/pull/32547#issuecomment-2891956944)
I agree it could just be an addition to the interface, though it also fits well if we want to prune it a bit.

💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2096297063)
Indeed it's nice if we can avoid this, because although reviewing the reversal was easy, finding it later with search is tedious.

👍 darosior approved a pull request: "test: properly check for per-tx sigops limit"
(https://github.com/bitcoin/bitcoin/pull/32533#pullrequestreview-2851706605)
ACK 7bc64a8859c3644fdf2eeff59f72a778ae60ea3f

💬 theuni commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2096309054)
Nice! I guess it manages to see through the references after all. My testing shows that:
```c++
auto& queue = m_chainman.GetCheckQueue();
CCheckQueueControl<CScriptCheck> control(queue);
CCheckQueueControl<CScriptCheck> control2(queue)
```

```c++
CCheckQueueControl<CScriptCheck> control(m_chainman.GetCheckQueue());
CCheckQueueControl<CScriptCheck> control2(m_chainman.GetCheckQueue());
```

Both of those indeed work.

I suppose I came to the conclusion that the
...

💬 darosior commented on pull request "test: properly check for per-tx sigops limit":
(https://github.com/bitcoin/bitcoin/pull/32533#issuecomment-2891985815)
> against an accidental soft fork

You mean against a tightening of the rules, which in the context of standardness is harder than a loosening of the rules? :p

👍 hebasto approved a pull request: "Use WitnessV0KeyHash in TestAddAddressesToSendBook"
(https://github.com/bitcoin-core/gui/pull/875#pullrequestreview-2851735518)
ACK fa982f14254433a969499e93c1c3f0db31dce6ab, I have reviewed the code along with the related changes from https://github.com/bitcoin/bitcoin/pull/32511.

✅ hebasto closed an issue: "`AddressBookTests` failure"
(https://github.com/bitcoin-core/gui/issues/874)

✅ hebasto closed an issue: "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]"
(https://github.com/bitcoin/bitcoin/issues/32558)

🚀 hebasto merged a pull request: "Use WitnessV0KeyHash in TestAddAddressesToSendBook"
(https://github.com/bitcoin-core/gui/pull/875)

🤔 darosior reviewed a pull request: "script: short-circuit `GetLegacySigOpCount` for known scripts"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2851775828)
This is a significant refactoring of tricky consensus critical routines that have essentially never been modified since Satoshi wrote them 15 years ago. The exploration is interesting but it seems ill-advised to completely overhaul this critical code for a marginal IBD speedup.

I feel bad for being stop energy here, but since i have been an advocate for us voicing our opinion instead of just ignoring PRs i'll go and own it. Concept NACK: i don't think the risk-benefit ratio of this change is
...

🤔 mzumsande reviewed a pull request: "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage"
(https://github.com/bitcoin/bitcoin/pull/32516#pullrequestreview-2851588776)
Concept ACK

💬 mzumsande commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#discussion_r2096386541)
This comment is outdated, because `DEFAULT_ANCESTOR_LIMIT = 25` is no longer after the last commit. Not sure about the reason why it was used before, is it no longer valid?

Also, changing DEFAULT_ANCESTOR_LIMIT to 100 could be mentioned in the commit message, it also affects the other subtests.