💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3309288797)
Rebased to resolve conflict with https://github.com/bitcoin/bitcoin/pull/33333.

💬 achow101 commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360894055)
Looks like the revert is in the wrong commit.

🤔 furszy reviewed a pull request: "index: remove unnecessary locator cleaning in BaseIndex::Init()"
(https://github.com/bitcoin/bitcoin/pull/32882#pullrequestreview-3241749692)
utACK facd01e6ffbbd019312f370a3807de0b95bbd745

At least for me, there’s no need to add me as a co-author when the code or suggestion is simple. Generally, only add people when they contribute a substantial change.
Just a small thought.

💬 hodlinator commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3309491269)
Parallel uninstall entries:
<img width="1358" height="508" alt="image" src="https://github.com/user-attachments/assets/3681dbae-127e-44ab-9031-7b6515fd020c" />

Error when trying to uninstall the second entry after the first one has already been uninstalled:
<img width="1357" height="509" alt="image" src="https://github.com/user-attachments/assets/cae121b8-d388-4ae7-9c1b-91d675420b8d" />

💬 hodlinator commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#discussion_r2361035808)
Apart from the added ` (64-bit)` suffix, these are copied from the uninstaller sections below, excluding items which would not originally have had the suffix.

💬 achow101 commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3309585706)
Given that the scope of the changes in this PR has expanded quite a bit since it was added to the milestone, and since the changes take place within consensus validation, I think this should be removed from the milestone. I don't think this will be able to get sufficient review for this release, and I don't think the changes here are necessary for the upcoming release.

💬 Crypt-iQ commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3309591741)
ACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0

💬 achow101 commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3309658082)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5

🚀 achow101 merged a pull request: "Remove unnecessary casts when calling socket operations"
(https://github.com/bitcoin/bitcoin/pull/33378)

💬 fanquake commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3309713059)
cc @sipsorcery

💬 achow101 commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-3309717927)
ACK b81f37031c8f2ccad9346f1b65ee0f8083c44796

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3241370035)
Code review ACK d1be44d3a672fc343eb3e64f4c0fea1a7e3030aa. Thanks for the update. I do feel like this is simpler with just one table. I left some suggestions below which are mostly about documentation and not important.

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360621351)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Could consider dropping the RPCConvertTable class and g_rpc_convert_table global variable. The RPCConvertTable class has no state and only has two standalone methods which could be plain functions.

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361185088)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Just IMO, but I feel like the example is confusing because "key=store" looks like a case where "key" actually is a named parameter and = sign should be parsed, not a case where it shouldn't be parsed. Also the next two sentences seem to just be repeating what the first two sentences say without out adding anything new. And I'm not sure the "IMPORTANT" section is really so imp
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361189357)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Maybe consider deleting this comment. It seems like it is mostly just repeating what code is doing without explaining why it is doing these things. If you do think a comment would be helpful here, probably the most useful thing might be some examples of the things being parsed. I suggested some documentation with examples in da919fb63c519ac91c9704c58f1bbe522d7a8879 ([tag](htt
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2361200492)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Note: with #33230 this could have JSON_OR_STRING added as well

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2360610696)
In commit "rpc: Handle -named argument parsing where '=' character is used" (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Could consider just leaving this function above the RPCConvertTable class instead of moving it below, since it is not changing. This would make the diff a little smaller.

💬 achow101 commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3309768622)
ACK 1a4ad0ae501d15f77b1601ace3e1a1c8bf440dd6

Tested on Windows 11, and it appears to correctly remove the old `(64-bit)` uninstall entry.

⚠️ polespinasa opened an issue: "Use compact blocks while doing background validation"
(https://github.com/bitcoin/bitcoin/issues/33431)
We don’t use Compact Blocks while performing background validation if the `assumetxoutset` option was used.

After loading a snapshot and syncing all the way to the chain tip, Compact Blocks can be used for newly announced blocks because the mempool is already being populated even if doing background validation.

💬 luke-jr commented on pull request "wallet/rpc: add scan_utxoset option to getbalance(s) to verify wallet balance accuracy":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2361315826)
This shouldn't be a positional parameter, at least.