💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360677074)
```suggestion
tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
```

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360683563)
nit/unrelated: I'm surprised to see an `std::list` here, `std::vector` is likely faster even for random erases

👍 l0rinc approved a pull request: "test: don't throw from the destructor of DebugLogHelper"
(https://github.com/bitcoin/bitcoin/pull/33388#pullrequestreview-3241444220)
Core review ACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0

I would also prefer adding quotes around the message but it's not a blocker.

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360670884)
That is my understanding as well, thanks

📝 john-moffett opened a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430)
Right now we return an opaque `{"success" : false}` in `addpeeraddress` for an empty or invalid IP. This changes it to throw `RPC_CLIENT_INVALID_IP_OR_SUBNET` with the error message `Invalid IP address`. Tests updated to match.

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3309277841)
Looked into this @achow101 and you're right about the setup issues - they weren't legacy wallet specific but rather configuration problems.
I've addressed the three issues you identified:

1. Added self.uses_wallet = True to prevent -disablewallet
2. MiniWallet naturally avoids deprecated settxfee by using modern fee handling
3. MiniWallet sidesteps multiwallet complexity entirely

Regarding your concern about the --gen-test-data path being rarely executed: I've ensured the test runs wit
...

💬 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
...