💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1475000788)
Done. Changed the `HasActiveTxn()` check to an assert.

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1475000995)
Done. Changed the `HasActiveTxn()` check to an assert.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1922047439)
Thanks for your fantastic in-depth review, @tdb3. I force-pushed to 58cb22c83c

This addresses the two discrepancies you caught in your actions 1 & 2:
- Empty batch requests are no longer considered "all notification" batches, by specifically handling the empty batch edge case
- Unparsable requests are responded to with `"id":null` again, by making `null` the default value of the `id` property in JSONRPCRequest. If a (parseable) request is 2.0 with no id field, we remove the `null` using `st
...

💬 achow101 commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1475005524)
I've removed the `wallet_dirs.insert` from `reload_wallet` and moved that to above the three `reload_wallet` calls that actually need it. The lambda is still useful since we have to unload `local_wallet` every time.

💬 achow101 commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1475006072)
Updated the commit message.

💬 ryanofsky commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1475035982)
re: https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473699881

> ## Action (1): **Unexpected difference**
>
> `curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/`
>
> Result: Unexpected difference
>
> v26.0 behavior: `[]`
>
> PR 27101 ([771d1e1](https://github.com/bitcoin/bitcoin/commit/771d1e1d206efe687b8661ab966cc1a62cc7ba39)) behavior: (no content) (in alignment with 2.0 spec, but not matching legacy behavior) Applicable line(s)
...

💬 Christewart commented on pull request "Add `SigVersion` parameter to `IsOpSuccess()`":
(https://github.com/bitcoin/bitcoin/pull/29265#issuecomment-1922086028)
> That said, I think a change like this belongs in the PR that implements a consensus change that needs it. It's generally better not to touch consensus code unless necessary.

Closing this PR on this recommendation.

For future readers that might find this useful, you can cherry-pick this commit: https://github.com/bitcoin/bitcoin/pull/29221/commits/50b72867b3ca4c794f8a768b8b2a9de7fbec1508 or find the latest in #29171

✅ Christewart closed a pull request: "Add `SigVersion` parameter to `IsOpSuccess()`"
(https://github.com/bitcoin/bitcoin/pull/29265)

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475072896)
(From IRL discussion)

The actually implemented optimization here is actually more powerful than what is described by the comment, because the weight isn't compared. Due to the fact that that among equal-value utxo groups, the lower weight ones sort first, higher weight ones are even worse, and can also be skipped.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475073648)
Yep, will update.

💬 murchandamus commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-1922141545)
I circled back to review, I think this has a merge conflict

💬 ajtowns commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1922177927)
Rebased

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1922201347)
rebased and always use v1 instead of disabling completely in the three slow (sub)tests.

👋 mzumsande's pull request is ready for review: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358)

💬 jamesob commented on pull request "test: Assumeutxo with more than just coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/29354#issuecomment-1922203109)
ACK https://github.com/bitcoin/bitcoin/pull/29354/commits/fa5cd66f0a47d1b759c93d01524ee4558432c0cc

💬 jamesob commented on pull request "rpc: Do not wait for headers inside loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1922205551)
Looks fine to me.

💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922260596)
I've dropped the clz changes as a test and kept only the endian/byteswap change. Locally on my machine the benchmarks look the same before and after.

If the corecheck benchmarks look better I'll update the title/description here.

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475114260)
In commit: test: Add coin_grinder_tests

I think clearing the expected result here is not necessary. You can get rid of this (and potentially also just move the declaration of `expected_result` here instead.

🤔 sr-gi reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1857573521)
Concept ACK.

First pass, I need to dig a bit deeper, but it makes sense to me

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475116651)
In commit: test: Add coin_grinder_tests

Same as per point 5)