💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2293918928)
I personally would let the maintainers decide the order, ank keep both open

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2293919832)
Thanks, done.

✅ fanquake closed an issue: "Indexes stuck on unknown best block after unclean shutdown"
(https://github.com/bitcoin/bitcoin/issues/33208)

🚀 fanquake merged a pull request: "index: Don't commit state in BaseIndex::Rewind"
(https://github.com/bitcoin/bitcoin/pull/33212)

💬 maflcko commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2294091366)
This seems to fail the integer sanitizer: https://cirrus-ci.com/task/6018134475276288?logs=check#L211:

```
test/node_init_tests.cpp(14): Entering test suite "node_init_tests"
test/node_init_tests.cpp(33): Entering test case "init_test"
2025-08-21T19:53:08.170490Z [unknown] [test/util/random.cpp:48] [void SeedRandomStateForTest(SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=ad2ae1d1b241a8a415874d45b2e104b01c396db4c21ec0899e66d0c85db1d978
2025-08-21T19:53:08.174281Z [te
...

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2294123857)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2294091366

Probably the node_init test (or maybe the unit test framework) should be disabling -natpmp after #33004 to prevent this code from running. But maybe there is a real bug on this line if it is subtracting `20 - 28`:

https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/common/netif.cpp#L127

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2294175851)
Good catch! Removed FEE_SATOSHIS and now using the default fee. The original settxfee(0.003) was arbitrary - since getblockstats measures UTXOs (not fees), the default fee works fine. Test still passes and code is cleaner.

💬 cedwies commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3214955658)
Concept ACK

One point to consider is a behavioral change in how the `onion_service_target` is selected.
Consider:
```md
-bind=192.167.1.10:8333=onion
-bind=127.0.0.1:8333=onion
```

Currently `192.167.1.10:8333` will be selected as `onion_service_target`. With this PR, it will be changed to `127.0.0.1:8333` (though one line in logs shows which address is used as target).


One option would be to keep using sets for deduplication, while still preserving the historical “first specifi
...

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2294179108)
Done

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2294190428)
FWIW chatgpt suggested the following fix for the netlink code. No idea if it is correct:

```diff
--- a/src/common/netif.cpp
+++ b/src/common/netif.cpp
@@ -123,6 +123,17 @@ std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
}

for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
+ // Netlink multipart dumps include control messages; ignore them.
+ if (hdr->nlmsg_type == NLMSG_DONE) {
+
...

💬 maflcko commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2294197834)
> https://github.com/bitcoin/bitcoin/pull/33004

Ah. I somehow assumed that the ISan warning was specific to this unit test and does not reproduce outside of it. However, I can now reproduce it outside:

```
root@ubuntu-16gb-hel1-1:~/b-c# sysctl -w net.ipv6.conf.all.disable_ipv6=1
net.ipv6.conf.all.disable_ipv6 = 1
root@ubuntu-16gb-hel1-1:~/b-c# UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/bit
...

💬 yancyribbens commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#discussion_r2294362185)
Since the goal here is to use the same sort as `descending_effval_weight`, L532 - L 535 can be replaced with this single line:

`return descending_effval_weight(group1, group2);`

💬 hamid2235 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/bd45bc611b8b245d0a6bc8e0f5a224b0642f06d0#r164459989)
Hamid

💬 hamid2235 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/bd45bc611b8b245d0a6bc8e0f5a224b0642f06d0#r164460003)
Hamid

💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3215378037)
Since these lists are tiny, Instead of a set for deduplication, we might as well use a simple O^2 algorithm, iterate from the back and remove whatever appeared in the front (it will likely be even faster than a set). If you decide to do that, I'd appreciate a randomized unit test checking it against a set deduplication.

💬 l0rinc commented on pull request "rpc: followups for 33106":
(https://github.com/bitcoin/bitcoin/pull/33189#issuecomment-3215395119)
> Hope this helps.

Why do you think that exactly? You didn't say anything new, my comment was about making it obvious from the PR's title what it's about (without having to open it) - and not only to ones who already know what 33106 was about.

💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2294520257)
This is here because the test fails without it.

💬 yancyribbens commented on pull request "coinselection: Tiebreak SRD eviction by weight":
(https://github.com/bitcoin/bitcoin/pull/33223#issuecomment-3215406859)
Any reason this is in Draft?

💬 achow101 commented on pull request "cli: Handle arguments that can be either JSON or string":
(https://github.com/bitcoin/bitcoin/pull/33230#issuecomment-3215421738)
> I guess this basically works for the existing params, since a block hash is never valid JSON (which forbids leading zeros), but could be dangerous to use for new parameters that are ambiguous.

I don't think we have any types where that would be the case. But this is also something that needs to be turned on for a parameter explicitly, so I don't expect this to be problematic.

> With that in mind, it might be worth instead having a new `RPCResult::Type::BLOCK_HEIGHT_OR_HASH` type to sanit
...

💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2294577799)
Done.