Bitcoin Core Github
44 subscribers
122K links
Download Telegram
βœ… glozow closed a pull request: "fuzz: Extend mini_miner fuzz coverage to max block weight"
(https://github.com/bitcoin/bitcoin/pull/31803)
πŸ’¬ fjahr commented on pull request "fuzz: Extend mini_miner fuzz coverage to max block weight":
(https://github.com/bitcoin/bitcoin/pull/31803#issuecomment-3407517424)
> Given the mini_miner_selection fuzz target is removed in #33629 (see [c88360c](https://github.com/bitcoin/bitcoin/commit/c88360cd82b6ba59b345d6d573b791e4964930d2)), it might be appropriate to close this? Thanks for working on this fwiw!

Makes sense, thanks for the heads up.
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433436966)
> I'd like to echo the concern other reviewers raised about separating the thread pool from the script parallelization work. Since script parallelization is already quite complex, would you consider starting with a simpler 2-threaded implementation in this PR, and introducing the general-purpose thread pool in a separate PR?

Adding 2 threads or `n` threads represents the exact same work. The code complexity comes from the two-step procedure that allows sequential dumps after parallel block da
...
πŸ€” glozow reviewed a pull request: "policy: don't CheckEphemeralSpends on reorg"
(https://github.com/bitcoin/bitcoin/pull/33616#pullrequestreview-3341571869)
This seems to make sense. I'd feel more confident if we add some more test cases for topologies and reorg scenarios that this does (not) apply to?
πŸ’¬ glozow commented on pull request "policy: don't CheckEphemeralSpends on reorg":
(https://github.com/bitcoin/bitcoin/pull/33616#discussion_r2433433677)
I think it's impossible for `m_bypass_limits` to be true in this function, so undoing this diff doesn't make a difference. Could we omit it or `Assume(!args.m_bypass_limits)`?
πŸ’¬ brunoerg commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3407583434)
> Tested, and `--skipreorg` successfully reduces test runtime from `60s to 35s`.
>
> I’m not very sure about this approach β€” splitting was also suggested here https://github.com/bitcoin/bitcoin/issues/16613#issuecomment-548952103 I think it’s a better long-term solution, but could be done in a follow-up.

I agree on that for the long-term, I could address on a follow-up and keep this as a simple solution for now.
πŸ’¬ andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433482378)
> Such simple PRs

The thread pool, tests, and fuzz harness are 560 lines. More than half the additions to this PR. I don't think it's fair to say that would be a simple PR.
πŸ’¬ stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2432963713)
This doesn't combine well with `invalidateblock`: e.g. when invalidating the tip, the log just gets spammed with warnings as we cycle through new peers:

```
2025-10-15T14:45:32Z UpdateTip: new best=000000092ca51c152010fd76552e12d9c9c7f410ebd04012dcff49eeecb6cfff height=274019 version=0x20000000 log2_work=42.943635 tx=27881630 date='2025-10-15T14:27:13Z' progress=1.000000 cache=71.8MiB(503926txo)
...
2025-10-15T14:45:50Z UpdateTip: new best=0000000b9e7f0760c48d76ef301616dbaa1421788b29911c02
...
πŸ’¬ maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3407677785)
Not sure why I get this failure, but when installing a fresh Fedora VM, then podman-docker inside that, and then running this pull rebased, I got:

```
MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh" ./ci/test_run_all.sh

...

100% tests passed, 0 tests failed out of 135

Total Test time (real) = 42.30 sec
+ traffic_monitor_end unit
+ test_name=unit
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (
...
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3407722811)
> ### Way forward
>
> Given that this PR has struggled to attract reviewers over the past two years, I think we need a different approach.
This is a risky change that touches critical index infrastructure - let's do it in tiny, focused steps instead.
>
> I'd like to propose keeping this PR open as a draft tracking PR that contains the full end-to-end implementation for reference and discussion. Meanwhile, we can merge the work through a series of smaller, focused PRs:

> * Start with a mi
...
πŸ‘ theuni approved a pull request: "Update libmultiprocess subtree in 30.x branch"
(https://github.com/bitcoin/bitcoin/pull/33519#pullrequestreview-3341726802)
ACK ae63cc4bf2d25a5b7ce9d166aa288431e8ca16a7 . Verified that it's the same as what's in master.

I don't see any reason to wait. We can always bump again for v30.1 if necessary.
πŸ’¬ maflcko commented on pull request "test: add option to skip large re-org test in feature_block":
(https://github.com/bitcoin/bitcoin/pull/33003#issuecomment-3407742302)
I am not sure about splitting this test. I think there is a benefit to run a reorg on a "dirty" state, so if the reorg test is run, we'd want to fully run it. If the reorg should be skipped, there is the option added in this pull.
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3407743683)
Now that the CI is happy again. Update regarding the introduced changes:

Before we had queues at two different levels, one at the thread pool level and another one at the index level. The initial rationale behind it was to granularly control the way on which the process interrupts them when needed. Not draining the index's `pending_tasks` queue as soon as the first task fails to process or a shutdown was triggered. But, after talking with mzumsande last week, I realized that this was actually
...
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433579778)
Yeah, updated. Thanks!
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433580060)
updated. Thanks!
πŸ’¬ furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2433592734)
> > Such simple PRs
>
> The thread pool, tests, and fuzz harness are 560 lines. More than half the additions to this PR. I don't think it's fair to say that would be a simple PR.

I don't think the fuzz and tests hard to grasp, but fair. I should have said that it is simple in comparison to the other changes, which require further contextual knowledge and not just multi-threading.
πŸ’¬ instagibbs commented on pull request "policy: don't CheckEphemeralSpends on reorg":
(https://github.com/bitcoin/bitcoin/pull/33616#discussion_r2433595098)
Took feedback in other PR ahead of time https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2389364292

I can definitely put an `Assume()` on it.
πŸ“ MamunC0der opened a pull request: "Update GitHub Action to github-script@v8"
(https://github.com/bitcoin/bitcoin/pull/33634)
Version v8 introduces performance improvements, enhanced JavaScript runtime support, and updated dependencies for greater security and reliability.
Release notes: actions/github-script@v8: https://github.com/actions/github-script/releases/tag/v8
πŸ€” hodlinator reviewed a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3341089959)
Reviewed 4aad3714d66cc80c08cb1c827fb49fba7dcf8d50

All annotations make sense, but left Q regarding 1-2 potentially missing ones.
πŸ’¬ hodlinator commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2433600255)
Shouldn't we also annotate this one with
```C++
EXCLUSIVE_LOCKS_REQUIRED(!connman.m_nodes_mutex)
```
?
...which leads to also annotating `ThreadMessageHandler`.