💬 pablomartin4btc commented on pull request "rpc: Do not wait for headers inside loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1921952510)
> > Curious about reasonable arguments to keep it though
>
> The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.

You might consider this coming then #28620 (make `loadtxoutset` async), the linked fix could be updated soon I think.

💬 achow101 commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1921984879)
Post merge ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1922011219)
I moved the static constants into classes and renamed them a bit. Expanded the fuzzer to send multiple messages back and forth (like the BIP324 fuzzer).

I dropped the 1 hour wiggle room in certificate timestamps, because it adds complexity and I don't expect this to cause any issues in practice. A self-signed certificate is generated when the TemplateProvider loads, which is unlikely the same second someone connects to it.

The fuzzer found a bug where I forgot to defragment messages larger
...

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474980968)
I'll look into replace `write` with `>>`.

Not sure if it's worth changing `m_sig` from `std::array<unsigned char>` to `std::array<std::byte>`. It's only used by `VerifySchnorr` and `SignSchnorr` which want a `Span<unsigned char>`, so whether we cast it here or there doesn't matter?

💬 benthecarman commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-1922026282)
Huge Concept ACK, this is great

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1922038851)
> would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.

That wouldn't work either, because with that last commit we actually use v2 when using P2PConnection (if --v2transport is chosen), so all of the tests that are fixed in this PR would fail. The problem is that we already run multiple the tests in the test runner with `--v2transport` but actually use v1 with P2PConnection...
I'll try to think of other solutions. O
...

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474995958)
Renamed

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474996068)
Added a comment.

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474996185)
Done as suggested.

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474996445)
Done as suggested

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474999990)
Added a comment.

> Separately, it seems like SQLiteBatch::WriteKey and SQLiteBatch::ExecStatement could be deduplicated or consolidated. They are basically identical copies of the same code.

The main difference is that `WriteKey` has an additional `BindBlobToStatement`, and a way to deduplicate that didn't occur to me when writing this originally. Will leave for a followup if anyone wants to do it though.

💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1475000625)
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_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)