💬 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.
(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
...
(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
(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) {
+
...
(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
...
(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);`
(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
(https://github.com/bitcoin/bitcoin/commit/bd45bc611b8b245d0a6bc8e0f5a224b0642f06d0#r164459989)
Hamid
💬 hamid2235 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/bd45bc611b8b245d0a6bc8e0f5a224b0642f06d0#r164460003)
Hamid
(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.
(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.
(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.
(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?
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2294577799)
Done.
💬 pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3215466979)
<ins>_Updates_</ins>
- Addressed @brunoerg's [feedback](https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2291906741); removed redundant comment, as the added assertion makes the message clear enough.
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3215466979)
<ins>_Updates_</ins>
- Addressed @brunoerg's [feedback](https://github.com/bitcoin/bitcoin/pull/33082#discussion_r2291906741); removed redundant comment, as the added assertion makes the message clear enough.
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294732188)
Yeah, I don't mind the order much, especially now that it seems like neither will be in v30. Thanks for asking!
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294732188)
Yeah, I don't mind the order much, especially now that it seems like neither will be in v30. Thanks for asking!
💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294736486)
Oh, so `clang-format` was pushing for it... seems it could do with some reconfiguration in the initializer-list aspect.
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294736486)
Oh, so `clang-format` was pushing for it... seems it could do with some reconfiguration in the initializer-list aspect.
💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258)
After https://github.com/bitcoin/bitcoin/pull/32813 we could customize the list formatting as well
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2294742258)
After https://github.com/bitcoin/bitcoin/pull/32813 we could customize the list formatting as well
👍 TheCharlatan approved a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592#pullrequestreview-3146111612)
ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763
(https://github.com/bitcoin/bitcoin/pull/32592#pullrequestreview-3146111612)
ACK 46ca7712cb5fcf759cfc9f4f32d74215c8c83763
💬 TheCharlatan commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#discussion_r2294770722)
Not really related to this PR, but I really don't like how long the scope of the lock is here. We could reduce this a bit by doing something like
```diff
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index b710c605bc..750ca72bb4 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -707,2 +706,0 @@ static RPCHelpMan getblocktemplate()
- WAIT_LOCK(cs_main, csmain_lock);
- uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
@@ -737,0 +736 @@ static RPCHelpMan getb
...
(https://github.com/bitcoin/bitcoin/pull/32592#discussion_r2294770722)
Not really related to this PR, but I really don't like how long the scope of the lock is here. We could reduce this a bit by doing something like
```diff
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index b710c605bc..750ca72bb4 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -707,2 +706,0 @@ static RPCHelpMan getblocktemplate()
- WAIT_LOCK(cs_main, csmain_lock);
- uint256 tip{CHECK_NONFATAL(miner.getTip()).value().hash};
@@ -737,0 +736 @@ static RPCHelpMan getb
...