👍 brunoerg approved a pull request: "fuzz: reduce iterations in slow targets"
(https://github.com/bitcoin/bitcoin/pull/33429#pullrequestreview-3246433430)
ACK 6a33970fef1b7b4d634f28277607b882958c95ac
I verified the perfomance gain. e.g. mini_miner target is now 10x faster on my machine.
mini miner (master):
```
#256 pulse cov: 4182 ft: 24483 corp: 253/57Kb exec/s: 128 rss: 278Mb
#512 pulse cov: 4234 ft: 27611 corp: 501/1139Kb exec/s: 8 rss: 761Mb
#602 INITED cov: 4235 ft: 28000 corp: 576/4293Kb exec/s: 5 rss: 797Mb
#602 DONE cov: 4235 ft: 28000 corp: 576/4293Kb lim: 489495 exec/s: 5 rss: 797Mb
Done 602 runs in 113 second(s)
```
...
(https://github.com/bitcoin/bitcoin/pull/33429#pullrequestreview-3246433430)
ACK 6a33970fef1b7b4d634f28277607b882958c95ac
I verified the perfomance gain. e.g. mini_miner target is now 10x faster on my machine.
mini miner (master):
```
#256 pulse cov: 4182 ft: 24483 corp: 253/57Kb exec/s: 128 rss: 278Mb
#512 pulse cov: 4234 ft: 27611 corp: 501/1139Kb exec/s: 8 rss: 761Mb
#602 INITED cov: 4235 ft: 28000 corp: 576/4293Kb exec/s: 5 rss: 797Mb
#602 DONE cov: 4235 ft: 28000 corp: 576/4293Kb lim: 489495 exec/s: 5 rss: 797Mb
Done 602 runs in 113 second(s)
```
...
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313378297)
Thanks @hebasto for finding it, thanks @vasild for the quick PR.
I'm working on trying to reproduce this on the mentioned platforms, but there's a reason I didn't add them originally since I couldn't test them via godbolt (https://xania.org/202506/how-compiler-explorer-works indicates they don't have native BSD or s390x servers).
@vasild if you edit again, can you please add @hebasto as a coauthor for the change?
Concept ACK for both changes in the PR (the maybe unused + adding BSD), I'
...
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313378297)
Thanks @hebasto for finding it, thanks @vasild for the quick PR.
I'm working on trying to reproduce this on the mentioned platforms, but there's a reason I didn't add them originally since I couldn't test them via godbolt (https://xania.org/202506/how-compiler-explorer-works indicates they don't have native BSD or s390x servers).
@vasild if you edit again, can you please add @hebasto as a coauthor for the change?
Concept ACK for both changes in the PR (the maybe unused + adding BSD), I'
...
💬 Crypt-iQ commented on pull request "fuzz: reduce iterations in slow targets":
(https://github.com/bitcoin/bitcoin/pull/33429#issuecomment-3313486792)
crACK 6a33970fef1b7b4d634f28277607b882958c95ac
(https://github.com/bitcoin/bitcoin/pull/33429#issuecomment-3313486792)
crACK 6a33970fef1b7b4d634f28277607b882958c95ac
🤔 furszy reviewed a pull request: "fuzz: reduce iterations in slow targets"
(https://github.com/bitcoin/bitcoin/pull/33429#pullrequestreview-3246723125)
A few extra optimizations (plus a bug fix) to the mini miner target: [https://github.com/furszy/bitcoin-core/tree/2025\_fuzz\_improve\_mini\_miner](https://github.com/furszy/bitcoin-core/tree/2025_fuzz_improve_mini_miner). They don’t seem to add much gain per @brunoerg’s feedback, but might help on the long run. Feel free to take them.
(https://github.com/bitcoin/bitcoin/pull/33429#pullrequestreview-3246723125)
A few extra optimizations (plus a bug fix) to the mini miner target: [https://github.com/furszy/bitcoin-core/tree/2025\_fuzz\_improve\_mini\_miner](https://github.com/furszy/bitcoin-core/tree/2025_fuzz_improve_mini_miner). They don’t seem to add much gain per @brunoerg’s feedback, but might help on the long run. Feel free to take them.
🤔 mzumsande reviewed a pull request: "net: Provide block templates to peers on request"
(https://github.com/bitcoin/bitcoin/pull/33191#pullrequestreview-3242205039)
Would be helpful to link the BIP PR/Delving Post in the OP.
I looked through the code, but will leave any low-level comments for later.
On a conceptual level, this seems like a strong fingerprinting vector (tor/clearnet response would be identical if requests happen roughly at the same time and the same template is used).
I'm not sure if we have given up on that, considering that there seems to be no lack of other existing fingerprinting methods.
(https://github.com/bitcoin/bitcoin/pull/33191#pullrequestreview-3242205039)
Would be helpful to link the BIP PR/Delving Post in the OP.
I looked through the code, but will leave any low-level comments for later.
On a conceptual level, this seems like a strong fingerprinting vector (tor/clearnet response would be identical if requests happen roughly at the same time and the same template is used).
I'm not sure if we have given up on that, considering that there seems to be no lack of other existing fingerprinting methods.
💬 mzumsande commented on pull request "net: Provide block templates to peers on request":
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2364203706)
Any reason to do this here instead of using the scheduler like, for example, `ReattemptInitialBroadcast` does?
(https://github.com/bitcoin/bitcoin/pull/33191#discussion_r2364203706)
Any reason to do this here instead of using the scheduler like, for example, `ReattemptInitialBroadcast` does?
💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3313660257)
Have a few questions and comments:
1. Main question is why is this new applySolution method needed at all if submitSolution() modifies the block and there is already a getBlock() method?
2. I think it would be good if submitSolution documented fact that calling it changes getBlockHeader, getBlock, getCoinbaseTx values.
3. New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior
...
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3313660257)
Have a few questions and comments:
1. Main question is why is this new applySolution method needed at all if submitSolution() modifies the block and there is already a getBlock() method?
2. I think it would be good if submitSolution documented fact that calling it changes getBlockHeader, getBlock, getCoinbaseTx values.
3. New comments say that coinbase witness is not added automatically, but it's not really clear what happens if client doesn't add it. Block is rejected or invalid? Behavior
...
💬 ryanofsky commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2364004534)
In commit "mining: add applySolution() to interface" (45fb350a6be50049bc2a85f4b4d9036199bfe392)
Is there a reason this is calling submitSolution twice? Would be good to have a comment if this is necessary.
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2364004534)
In commit "mining: add applySolution() to interface" (45fb350a6be50049bc2a85f4b4d9036199bfe392)
Is there a reason this is calling submitSolution twice? Would be good to have a comment if this is necessary.
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364461162)
Based on https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/common/system.cpp#L79 I also suggest something similar:
```suggestion
#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__)
```
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364461162)
Based on https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/common/system.cpp#L79 I also suggest something similar:
```suggestion
#elif defined(__linux__) || defined(__APPLE__) || defined(__FreeBSD__) || defined(__OpenBSD__) || defined(__NetBSD__)
```
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313729211)
I was able to reproduce the problem with `FreeBSD` by adding a [dummy CI job](https://github.com/l0rinc/bitcoin/pull/39/commits):
* https://github.com/l0rinc/bitcoin/actions/runs/17867131621/job/50812048046?pr=39#step:3:865
> error: unused variable 'clamp'
After applying the PR the test passes successfully:
* https://github.com/l0rinc/bitcoin/actions/runs/17868501673/job/50816606849?pr=39#step:3:1394
> test/system_tests.cpp(23): info: check GetTotalRAM() >= 1000_MiB has passed
> GetTotal
...
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313729211)
I was able to reproduce the problem with `FreeBSD` by adding a [dummy CI job](https://github.com/l0rinc/bitcoin/pull/39/commits):
* https://github.com/l0rinc/bitcoin/actions/runs/17867131621/job/50812048046?pr=39#step:3:865
> error: unused variable 'clamp'
After applying the PR the test passes successfully:
* https://github.com/l0rinc/bitcoin/actions/runs/17868501673/job/50816606849?pr=39#step:3:1394
> test/system_tests.cpp(23): info: check GetTotalRAM() >= 1000_MiB has passed
> GetTotal
...
💬 hebasto commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364474599)
@l0rinc
Why are you suggesting skipping illumos-based distros?
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364474599)
@l0rinc
Why are you suggesting skipping illumos-based distros?
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364477371)
No, I just cannot personally test or ACK them since there's no way for me to test them, but if you do, that's enough for me.
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364477371)
No, I just cannot personally test or ACK them since there's no way for me to test them, but if you do, that's enough for me.
⚠️ ryanofsky opened an issue: "How to backport libmultiprocess changes?"
(https://github.com/bitcoin/bitcoin/issues/33439)
From https://www.erisian.com.au/bitcoin-core-dev/log-2025-09-18.html#l-365
```
<fanquake> sjors: how are you planning to handle version in the multiprocess subtree going forward?
<fanquake> *versioning
<Sjors[m]1> fanquake: do you mean versioning of the interface or of the multiprocess dependency?
<fanquake> I'm guessing release branches to match Core? Otherwise I'm wondering what the plan is if you need to land bugfixes from multiprocess that wont apply to release branches, due to api changes
...
(https://github.com/bitcoin/bitcoin/issues/33439)
From https://www.erisian.com.au/bitcoin-core-dev/log-2025-09-18.html#l-365
```
<fanquake> sjors: how are you planning to handle version in the multiprocess subtree going forward?
<fanquake> *versioning
<Sjors[m]1> fanquake: do you mean versioning of the interface or of the multiprocess dependency?
<fanquake> I'm guessing release branches to match Core? Otherwise I'm wondering what the plan is if you need to land bugfixes from multiprocess that wont apply to release branches, due to api changes
...
💬 achow101 commented on pull request "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3313768013)
ACK 79752b9c0b6bd9b2203ac98d28dd67734050c14a
(https://github.com/bitcoin/bitcoin/pull/33422#issuecomment-3313768013)
ACK 79752b9c0b6bd9b2203ac98d28dd67734050c14a
💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364568090)
> Don't see much of a point to change m_prev_script_checks_logged types twice
I deliberately separated "enabling" logs changes from enabling reasons to show the changes in small steps, reflected in the tests. They're fundamentally different changes that are reflected in separate test and behavior changes, do you feel strongly about merging them?
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364568090)
> Don't see much of a point to change m_prev_script_checks_logged types twice
I deliberately separated "enabling" logs changes from enabling reasons to show the changes in small steps, reflected in the tests. They're fundamentally different changes that are reflected in separate test and behavior changes, do you feel strongly about merging them?
💬 hebasto commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364592056)
I have also tested locally on the following system:
```
$ uname -snrv
SunOS openindiana 5.11 illumos-ee653ea2dd
```
If `... || defined(__illumos__)` is not appended, the following test fails::
```
$ ./build/bin/test_bitcoin -t system_tests/total_ram
Running 1 test case...
./test/system_tests.cpp(23): error: in "system_tests/total_ram": check GetTotalRAM() >= 1000_MiB has failed [std::nullopt < 1048576000]
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
```
...
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364592056)
I have also tested locally on the following system:
```
$ uname -snrv
SunOS openindiana 5.11 illumos-ee653ea2dd
```
If `... || defined(__illumos__)` is not appended, the following test fails::
```
$ ./build/bin/test_bitcoin -t system_tests/total_ram
Running 1 test case...
./test/system_tests.cpp(23): error: in "system_tests/total_ram": check GetTotalRAM() >= 1000_MiB has failed [std::nullopt < 1048576000]
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
```
...
🤔 hebasto reviewed a pull request: "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD"
(https://github.com/bitcoin/bitcoin/pull/33435#pullrequestreview-3247240250)
Additionally, I suggest guarding this check:https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/test/system_tests.cpp#L23 with the same `#ifdef`s as in the `GetTotalRAM()` implementation. Otherwise, this test will fail on any unmentioned system.
(https://github.com/bitcoin/bitcoin/pull/33435#pullrequestreview-3247240250)
Additionally, I suggest guarding this check:https://github.com/bitcoin/bitcoin/blob/56c6daa64f6bc29fb83f3ec8940c37ddc549edeb/src/test/system_tests.cpp#L23 with the same `#ifdef`s as in the `GetTotalRAM()` implementation. Otherwise, this test will fail on any unmentioned system.
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364600639)
Perfect, that's why I have added the test so that we can detect these.
Does it display the correct value if we print `GetTotalRAM()`?
```patch
diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
--- a/src/test/system_tests.cpp (revision 50ba74ae2cb97aa9052f4e20385dff3afdb6d7cd)
+++ b/src/test/system_tests.cpp (revision c968ae0b75785f6cc70d80f649a45b827a80b6b0)
@@ -21,6 +21,7 @@
BOOST_AUTO_TEST_CASE(total_ram)
{
BOOST_CHECK_GE(GetTotalRAM(), 1000_MiB);
+ std::co
...
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364600639)
Perfect, that's why I have added the test so that we can detect these.
Does it display the correct value if we print `GetTotalRAM()`?
```patch
diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
--- a/src/test/system_tests.cpp (revision 50ba74ae2cb97aa9052f4e20385dff3afdb6d7cd)
+++ b/src/test/system_tests.cpp (revision c968ae0b75785f6cc70d80f649a45b827a80b6b0)
@@ -21,6 +21,7 @@
BOOST_AUTO_TEST_CASE(total_ram)
{
BOOST_CHECK_GE(GetTotalRAM(), 1000_MiB);
+ std::co
...
💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313929935)
> Otherwise, this test will fail on any unmentioned system
That's exactly why it was added so that we can detect and cover those. Is it too strict?
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313929935)
> Otherwise, this test will fail on any unmentioned system
That's exactly why it was added so that we can detect and cover those. Is it too strict?
🚀 hebasto merged a pull request: "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/33422)
(https://github.com/bitcoin/bitcoin/pull/33422)