Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 l0rinc commented on pull request "[IBD] coins: increase default UTXO flush batch size to 32 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-3234329502)
Thanks a lot for testing this @hodlinator and @Eunovo.

It seems that different configurations behave differently here, so I've retested a few scenarios on a few different platforms to understand what the performance increase depends on exactly. It's not even a linear speedup (i.e. sometimes 32 MiB is faster, sometimes it's 64 MiB), some `dbcache` values reliably produce a lot faster results, while others reliably produce slower results for the same config (i.e. the before-after-plot for diffe
...
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3234350722)
Also, would be nice to have a pull request for https://github.com/bitcoin-core/qa-assets prepared. Possibly without caching is fine, if the msan-fuzz task fits in the GHA timeout.
👋 ajtowns's pull request is ready for review: "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)"
(https://github.com/bitcoin/bitcoin/pull/29843)
💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308112657)
In https://github.com/bitcoin/bitcoin/pull/32159/commits/57ce645f05d18d8ad10711c347a5989076f1f788 (_net: filter for default routes in netlink responses_)

Just info for other reviewers, I found this useful since I'm generally unfamiliar with how routing and gateways work:

As I understand, the destination prefix is a mask indicating what routes the gateway can be used for, a destination prefix of `0` (alternatively `0.0.0.0` or `::0`) indicates that the gateway can be used for all destinatio
...
💬 l0rinc commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2308173787)
I started it and accidentally left it in the background and it fetched 100k blocks - if it's an "afterglow", it's a resilient one. And I did reproduce it on 3 separate servers, seems like a real bug - though likely not introduced in this PR, will open an issue for it or investigate it myself later this week.
💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-3234490515)
> It is just dead code now, no?

@maflcko I think you're right.
Will squash after review, think it's easier this way.
💬 davidgumberg commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#discussion_r2308207185)
In https://github.com/bitcoin/bitcoin/pull/32159/commits/42e99ad77396e4e9b02d67daf46349e215e99a0f (_net: skip non-route netlink responses_)

The `man 7 rtnetlink` page (https://man7.org/linux/man-pages/man7/rtnetlink.7.html) does not really offer much insight here, but as far as I can tell, the convention comes from the fact that the rtnetlink interface is also used for monitoring changes to the kernel's routing table, and because the userspace application is trying to maintain an idea of the
...
💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#discussion_r2308221628)
forgot to remove the line, will do next push
🚀 fanquake merged a pull request: "ci: return to using dash in CentOS job"
(https://github.com/bitcoin/bitcoin/pull/33261)
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308229142)
done
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308230173)
I like to see `false` if it's not in b validation.
Fixed the "estimate of"
🚀 fanquake merged a pull request: "threading: reduce the scope of lock in getblocktemplate"
(https://github.com/bitcoin/bitcoin/pull/33264)
🚀 fanquake merged a pull request: "rpc: followups for 33106"
(https://github.com/bitcoin/bitcoin/pull/33189)
🚀 fanquake merged a pull request: "ci: use LLVM 21"
(https://github.com/bitcoin/bitcoin/pull/33258)
💬 polespinasa commented on pull request "rpc, logging: add backgroundvalidation to getblockchaininfo":
(https://github.com/bitcoin/bitcoin/pull/33259#discussion_r2308265935)
That's a good point, but that's actually what we are showing in the logs. Do we want to keep consistency between both? Update maybe also the logging?


```[background validation] UpdateTip: new best=00000000000000000007b796179f5abb7eed59f736e3418eb8381c29eb78527a height=744000 version=0x20400004 log2_work=93.616004 tx=747106639 date='2022-07-07T14:47:25Z' progress=0.605933 cache=358.0MiB(2606151txo)```


In `validation.cpp`
```c++
constexpr int BACKGROUND_LOG_INTERVAL = 2000;
if
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376222)
Done
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376554)
You're correct - the comments were misleading. I've updated them to accurately reflect that MiniWallet uses consistent fee behavior for all transactions. The test still validates `getblockstats` functionality across different block structures, which is the primary goal.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2308376622)
You're correct that UTXO statistics don't depend on output destination. However, I'm keeping external outputs to maintain consistency with the original test's transaction structure. Changing to `send_self_transfer` would alter the generated test data and expected statistics values.

Since the goal is to replace the wallet implementation while preserving test behavior, `send_to` with external scripts is the most faithful translation of the original `sendtoaddress` calls.
💬 w0xlt commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#issuecomment-3234759432)
> IMO this warning is sufficient, but if you keep the error approach, consider removing the warning.
Thanks for the review @naiyoma .
The error will occur regardless, whether in master or in this PR.
This PR simply provides a clearer and more useful message.

Regarding removing the warning, I think the `bind` validation could be moved to occur before the onion validation. Good catch.
hebasto closed a pull request: "cmake, guix: Skip building tests in subtrees for releases"
(https://github.com/bitcoin/bitcoin/pull/32054)