💬 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.

💬 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__)
```

💬 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
...

💬 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?

💬 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.

⚠️ 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
...

💬 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

💬 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?

💬 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"
```

...

🤔 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.

💬 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
...

💬 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?

🚀 hebasto merged a pull request: "build: Remove lingering Windows registry & shortcuts (#32132 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/33422)

💬 hebasto commented on pull request "[30.0] Final changes + rc2":
(https://github.com/bitcoin/bitcoin/pull/33424#issuecomment-3313948750)
Backport https://github.com/bitcoin/bitcoin/pull/33422?

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2364619758)
Thanks for the suggestion! I think I've addressed this correctly in [07027af](https://github.com/bitcoin/bitcoin/pull/33247/commits/07027afa155f8ec9715c761d80630b31723c2c32) -- built for linux successfully; here's my GUIX build hashes:

```8890962245e7c04cba900e0c5b6c57e2d9b3779f08d7bb6f0f1ec8baee9a1425 dist-archive/bitcoin-07027afa155f.tar.gz
4ed2d3dfe075bf5f98042bfe2165d858a4913b6933c8098e52cf607fbe76c84b x86_64-linux-gnu/bitcoin-07027afa155f-x86_64-linux-gnu-debug.tar.gz
7b11bd93f10a375
...

💬 hebasto commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313959247)
> > 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?

We usually avoid breaking people’s tests just to collect coverage data :)

💬 hodlinator commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364630671)
Curious what other reviewers think, but not a blocker.

💬 hebasto commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364631092)
> Does it display the correct value if we print `GetTotalRAM()`?

Yes, it does.

💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313964858)
I assumed we want to be notified about unsupported systems, is that not the case?

💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364638838)
I find it important to modify this area of the code in tiny steps, it's why I separated the two concerns in the first place.