💬 dhruv commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#issuecomment-1477247368)
Addressed review by @theStack.

💬 dhruv commented on pull request "BIP324: Add encrypted p2p transport {de}serializer":
(https://github.com/bitcoin/bitcoin/pull/23233#issuecomment-1477250064)
Latest push:
- Upstream changes from #25361
- Rebased

💬 dhruv commented on pull request "BIP324: CKey encode/decode to elligator-swift":
(https://github.com/bitcoin/bitcoin/pull/23432#issuecomment-1477250673)
Rebased.

💬 dhruv commented on pull request "BIP324: Handshake prerequisites":
(https://github.com/bitcoin/bitcoin/pull/23561#issuecomment-1477251465)
Rebased.

💬 dhruv commented on pull request "BIP324: Enable v2 P2P encrypted transport":
(https://github.com/bitcoin/bitcoin/pull/24545#issuecomment-1477252274)
Rebased.

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477374232)
Can remove `--valgrind` from title? See also https://github.com/bitcoin/bitcoin/pull/27199#pullrequestreview-1349425797

💬 MarcoFalke commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477392747)
> 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?

💬 jonasschnelli commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1477425501)
The original idea of this patch was to add a small and easy to maintain **efficient** way to generate a fee histogram to bitcoin core (see original PR description https://github.com/bitcoin/bitcoin/pull/15836#issue-434109585).

As described there, it was always possible to use `getrawmempool` (@jhoenicke script) to achieve this goal.

However, it is **not efficient** to dumb out all transactions of the complete mempool in a json format just to calculate the fee histogram with a python script
...

💬 S3RK commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1477439839)
As far as I understand after this PR is merged we will start unsetting the blank flag and loosing the users' intent.
Are we okay with loosing the intent for the wallets that used the code between this and the follow up PR?

💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure under `--valgrind` ":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1477448564)
Looks like there are several issues here. The first one, unrelated to the test failure is that the return value is thrown away (thread.join() returns None, always)

📝 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