📝 MarcoFalke opened a pull request: "test: Replace threading with concurrent.futures"
(https://github.com/bitcoin/bitcoin/pull/27287)
`threading` has no easy way to get the return value or exception once the target function stops. Not checking the return value or exception can make tests more fragile and failures harder to debug.

Fix this by checking the return value (or exception) by wrapping the function execution into a future and calling `result()` on it.

Can be reviewed with `--ignore-all-space`.

(There are still some uses of `threading` around, because some tests do expect an exception to be thrown and caught in
...

💬 TheCharlatan commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1143019799)
This is not consistent with how the other options are handled. This overrides the value of the passed in option even if no argument was passed in. I would handle this the same way as `max_tip_age` and also move the `MAX_SCRIPTCHECK_THREADS` to `chainstatemanager_opts.h`.

💬 TheCharlatan commented on pull request "refactor: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#discussion_r1143022988)
Nit: This diff would be more move only if `par_value` were still called `script_threads`. I also find `script_threads` a better name than `par_value`.

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477480656)
See https://github.com/bitcoin/bitcoin/pull/27287

💬 TheCharlatan commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1477480680)
re-ACK ebfc2a2dddedcbf384f435716d30d8418a77505b

💬 rebroad commented on issue "Processing of new blocks slower than necessary due to overly selective peer selection":
(https://github.com/bitcoin/bitcoin/issues/21803#issuecomment-1477495357)
The first node that announced the block with the existing code (well, at the time I raised this issue) generally gets designated as a high-bandwidth peer once the blocktxn has been received from it, and the block that provided the cmpctblock first often gets redesignated as a low-bandwidth peer (with the existing code). With my code changes, the node that provided the cmpctblock first is less likely to lose it's high-bandwidth designation, and yet the node that provided the header first does get
...

💬 fanquake commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#issuecomment-1477523767)
@mzumsande see also #27282.

💬 kristapsk commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477532882)
> > How can we expect the Bitcoin Core wallet to behave reasonably if it can't predict the fees well?
>
> The Bitcoin Core wallet does not use this feature?

Does not use yet?

💬 prusnak commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477559496)
> The Bitcoin Core wallet does not use this feature?

My point is that a wallet without fee estimation is not very usable and harms the overall network. If we don't want to have an effective way of determining the fee in Bitcoin Core, we might as well remove the entire wallet from the codebase.

👍 TheCharlatan approved a pull request: "rpc: Add submit option to generateblock"
(https://github.com/bitcoin/bitcoin/pull/18933)
tACK fa18504d5767a40dc9827fb081633219bf251001

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143154239)
in commit 92dbf055528f16fcf51ea4bb310b9f319796d57f

I took another look at this and it seems this isn't a bugfix, but a workaround to silence a clang-tidy warning.

My understanding is that `FakeCheckCheckCompletion` can't be moved/swapped more efficiently than (default)constructing it, since it is just an empty struct. However, the underlying vector that holds the objects still needs to malloc (according to the comment you deleted).

I think an alternative fix would be to simply call `cle
...

💬 glozow commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477583517)
There seems to be confusion here. This PR does not touch the fee estimator or the wallet in any way. The fee estimator already keeps track of feerates internally; the wallet queries it directly for feerate estimates. I don't know of any examples where the wallet needs to see a graph of something.

If you feel that the fee estimation algorithms need improvement, by all means I'd say some review/PRs there would be welcome. Maybe take a look at #25380 or #21161.

For those that feel very passio
...

💬 ismaelsadeeq commented on pull request "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/27280#issuecomment-1477586167)
> Can be tested with:
>
> ```diff
> diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
> index 70a802dc58..1d1c6a64b6 100755
> --- a/test/functional/feature_init.py
> +++ b/test/functional/feature_init.py
> @@ -52,7 +52,7 @@ class InitStressTest(BitcoinTestFramework):
> assert_equal(200, node.getblockcount())
>
> lines_to_terminate_after = [
> - b'Validating signatures for all blocks',
> + b'.Validating signat
...

💬 0xB10C commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477586224)
It's confusing to me why this PR, tagged as `[RPC/REST/ZMQ]` and only touching `src/rest.cpp` and files in `src/rpc/*`, should have anything to do with the wallet or the GUI. AFAIK the wallet doesn't use the RPC interface to communicate with bitcoind. If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.

💬 kristapsk commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477594252)
> If the wallet or the GUI would want a histogram, it couldn't use the code added in this PR.

That would require just moving fee histogram code out of `MempoolInfoToJSON()` to a separate function somewhere out of `rpc/`, code itself seems to me reusable.

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143174094)
(No idea if this makes a difference, the compiler may optimize everything into the same binary anyway)

💬 darosior commented on pull request "p2p: remove adjusted time":
(https://github.com/bitcoin/bitcoin/pull/25908#issuecomment-1477612785)
Concept ACK.

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1143197655)
in commit b43ef231d241e7f7ef7c7685a9ca7a6274d1b943


Any reason to keep the unused and dangerous `CScriptCheck` default constructor? (For example, it leaves `txdata` completely uninitialized?)

👍 willcl-ark approved a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101)
ACK 2da823199

Left a few comments which could be addressed here, in a followup or not at all. None of them materially affect the current implementation of this feature which works well.

Although, I would be curious to know why we are still responding with `http 500` errors for invalid json after the move to json 2.0.

💬 willcl-ark commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1142695923)
Were we not using the out-of-spec `"jsonrpc": "1.0"` here previously?

edit: It seems it doesn't make any difference as was not being validated? 🤷🏼‍♂️