💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232618421)
> No behaviour changes for Autotools builds.

I don't think this is true. This is a bugfix.

Previously one could not run the tests in the autotools out-of-source build dir:

```
$ ./test/functional/wallet_disable.py
bash: ./test/functional/wallet_disable.py: No such file or directory
$ ln --symbolic ../../../test/functional/wallet_disable.py ./test/functional/
$ ./test/functional/wallet_disable.py
Traceback (most recent call last):
File "./test/functional/wallet_disable.py", lin
...

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232621185)
Concept ACK on the behavior changes and the bugfix, but please properly describe the changes.

👍 TheCharlatan approved a pull request: "refactor: Make m_last_notified_header private"
(https://github.com/bitcoin/bitcoin/pull/30466#pullrequestreview-2182133067)
lgtm fa927055dd43dda945396574273a210186beec9f

💬 hebasto commented on pull request "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds":
(https://github.com/bitcoin/bitcoin/pull/30465#issuecomment-2232644321)
My Guix build:
```
x86_64
460732347ffc71500f84d1174f6a6011b887681e82fc1704c1187cb1701bf9af guix-build-5ff8074361e9/output/aarch64-linux-gnu/SHA256SUMS.part
b084b5371e3acff1850c8f3234299f99c3635fd497919f28407acaee27322877 guix-build-5ff8074361e9/output/aarch64-linux-gnu/bitcoin-5ff8074361e9-aarch64-linux-gnu-debug.tar.gz
9f0f40cb3510197338efca2c7423fb7615a34a8b21a90c500c7fa45898875a9d guix-build-5ff8074361e9/output/aarch64-linux-gnu/bitcoin-5ff8074361e9-aarch64-linux-gnu.tar.gz
b571a25f1
...

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232654759)
@maflcko
> Previously one could not run the tests in the autotools out-of-source build dir:
>
> After this fix, it is possible.

That's true. But I did not consider this case as it requires a manual creation of a symlink to a test, which Autottols do not do, but CMake does.

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232664350)
In any case, creating the symlinks seems like a good way to test this, no?

👍 hodlinator approved a pull request: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052#pullrequestreview-2182119840)
ACK fa986c918a239cabda198f044b6eb9739c298be9

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680561263)
nit: Could put cheapest condition first:
```suggestion
if (opts.use_xor && fs::is_empty(opts.blocks_dir)) {
```

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680615465)
Smaller change if we make `util_xor()` take an offset:
```diff
diff --git a/test/functional/feature_reindex.py b/test/functional/feature_reindex.py
index f079a5e343..9f3f5357ba 100755
--- a/test/functional/feature_reindex.py
+++ b/test/functional/feature_reindex.py
@@ -43,15 +43,15 @@ class ReindexTest(BitcoinTestFramework):
NUM_XOR_BYTES = 8 # From InitBlocksdirXorKey::xor_key.size()
xor_dat = xor_f.read(NUM_XOR_BYTES)

- def util_xor(data, key):
+
...

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232705519)
Tested with the steps I provided.

tested ACK a8e3af1a82dd584a1cc3ffbe587e66889f72e3c 🎨

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trus
...

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232711655)
> In any case, creating the symlinks seems like a good way to test this, no?

Sure thing. The PR description has been updated.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2232722842)
Friendly ping @stickies-v :)

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1680652673)
question: Is there a reason why this needs to change, but the other `os.path.dirname(os.path.realpath(__file__))` and `Path(__file__).parents` are fine?

👍 dergoegge approved a pull request: "refactor: Make m_last_notified_header private"
(https://github.com/bitcoin/bitcoin/pull/30466#pullrequestreview-2182266399)
utACK fa927055dd43dda945396574273a210186beec9f

💬 maflcko commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2232743499)
> which means that 29.0 will be built using CMake.

Could add the 29.0 milestone?

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680658887)
May do if I have to re-touch. This is only called once per process, so shouldn't matter.

💬 fanquake commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1680665264)
CPP flag handling seems inconsistent throughout the CI changes, which is confusing. It'd be good to consistently put all CPPFLAGS into CPPFLAGS, rather than having them sometimes in CXXFLAGS, sometimes in (append) CPPFLAGS, and sometimes spread between the two, i.e here.

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680671825)
Thanks, done!

💬 maflcko commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680671935)
Thanks, done!

💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2232775166)
> > which means that 29.0 will be built using CMake.
>
> Could add the 29.0 milestone?

Done.