👍 martinus approved a pull request: "refactor: Make `CCheckQueue` RAII-styled"
(https://github.com/bitcoin/bitcoin/pull/26762#pullrequestreview-1377151234)
Code review ACK 51f91c2c5bd, as far as I can say now deadlocks are not possible any more when shutting down. I started an initial sync with `-assumevalid=0` so script checks are done, stopping the process a few times with Ctrl+C, all good; shutdown is still quick. `-par` still works too.

💬 martinus commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1161479180)
nit, just a thought: maybe it would make sense to have a method like `signalStop()` or similar in `CCheckQueue` that only sets `m_request_stop` to true, and call this as soon as we know we don't need the queue's result any more. That way the threads will stop processing and stop wasting CPU while shutting down.

On the other hand the queue will usually pretty quickly stop working unless it has lots of work items, so I guess in most cases this wouldn't make any difference.

💬 MarcoFalke commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1161589201)
```suggestion
cmake -B "${BASE_SCRATCH_DIR}"/msan/build/ -DLLVM_ENABLE_RUNTIMES='libcxx;libcxxabi' -DCMAKE_BUILD_TYPE=Release -DLLVM_USE_SANITIZER=MemoryWithOrigins -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF -S "${BASE_SCRATCH_DIR}"/msan/llvm-project/runtimes
```

nit?

💬 fanquake commented on pull request "test: LLVM/Clang 16 for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27436#discussion_r1161597309)
Yea. Fixed

💬 achow101 commented on pull request "doc: Add example of how to mix private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/27414#issuecomment-1501769224)
Concept ACK

Please squash your commits

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1161726047)
yes, that was something I touched while I was verifying the `req` class member.

💬 achow101 commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#issuecomment-1501822228)
ACK 68eed5df8656bed1be6526b014e58d3123102b03

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1161727508)
I'll change it. Thanks.

💬 achow101 commented on pull request "wallet: Avoid underpaying transaction fees when signing taproot spends":
(https://github.com/bitcoin/bitcoin/pull/23502#issuecomment-1501832645)
> How would this behave in the context of changeless transactions? Would we be overestimating the target and dropping an excess to the fees?

All overages due to size estimation differences are dropped to fees. The output values are set based upon the estimated size and they cannot be changed after signing. Once signing is completed and the final transaction created, if it ends up being smaller than estimated, whatever could have been saved will not be saved.

> Does that get recognized when
...

💬 achow101 commented on pull request "wallet: Avoid underpaying transaction fees when signing taproot spends":
(https://github.com/bitcoin/bitcoin/pull/23502#discussion_r1161737139)
I don't quite follow your question.

The estimated stack size is used to choose which result to return as multiple results could be valid. In normal signing, we use the smallest valid stack. In estimating, we use the largest.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161754205)
I followed the indentation style used for the same `warnings` field in RPCs createmultisig, addmultisigaddress, importmulti, importdescriptors, etc., as yes, `clang-format` would change the whole function.

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161754652)
Done

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161754817)
Changed to `temp_mtx`

💬 achow101 commented on pull request "bumpfee: avoid making bumped transactions with too low fee when replacing outputs":
(https://github.com/bitcoin/bitcoin/pull/27308#discussion_r1161757888)
> If I understand this test right, it checks whether the absolute fee of a smaller replacement transaction sufficiently surpasses that of a larger original transaction.

No, it doesn't check any feerates since we can't see the candidate bump transaction. The calculations here are just for figuring out a feerate that would fail the modified checks and checking that `bumpfee` fails as expected.

> I looked around in `wallet_bumpfee.py`, and had the impression that we generally seem to assume t
...

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1161770541)
Thanks for looking! I'm not sure whether compilers optimize for it but happy to drop that unrelated change, will do.

💬 ryanofsky commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1161771202)
> [8cd7730](https://github.com/bitcoin/bitcoin/commit/8cd77302305a2387368aabe8744b1859caf9956a): paging @ryanofsky: do you think we should move all this complexity here to the interface?

Right, probably it would be better to move the new code from the `wallet:WalletImpl::getAvailableBalance()` method in `src/wallet/interfaces.cpp` into the `wallet::GetAvailableBalance()` function in `src/wallet/spend.cpp` or into another function there like `wallet::GetSelectedCoinsBalance`. Reasoning is

-
...

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161777870)
Changed here and elsewhere

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161777986)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778269)
Done as suggested

💬 achow101 commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#discussion_r1161778333)
Done as suggested