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

💬 hebasto commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3313992139)
> If we do want to make sure unsupported systems pass gracefully, can we at least add a warning test log for those cases?

I’d say that the `total_ram` test should be:
1) Skipped on unsupported platforms with an explanatory message;
2) Placed into its own test suite so that `ctest` can clearly distinguish and report the skipped test in the statistics.

💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364654157)
Done

💬 l0rinc commented on pull request "log: always print initial script verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2364654341)
I don't think it makes much difference, but looks like you do, so I have changed it to `const char*`, let me know what you think.

💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#discussion_r2364674969)
`NetBSD` also finished successfully: https://github.com/l0rinc/bitcoin/actions/runs/17870748757/job/50823702246?pr=39

💬 l0rinc commented on pull request "system: silence unused variable warning and make GetTotalRAM() work on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/33435#issuecomment-3314050255)
Good idea @hebasto, @vasild can you please cherry-pick [l0rinc/bitcoin@`f011c2c` (#39)](https://github.com/l0rinc/bitcoin/pull/39/commits/f011c2cdf170dbd53c7ec2e46379238579232d67) on top of the PR?
Please add @hebasto as a coauthor.

💬 l0rinc commented on pull request "index: handle case where pindex_prev equals chain tip in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2364845277)
Not sure what you mean, I prefer having a test that I can run with and without the fix to debug both cases to understand the change better. Otherwise this just looks like a random change that we can just revert and the CI wouldn't even catch it.

💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3314211120)
Code review reACK 2427939935f3e6669be6bf553be89639e0afabaa