💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#discussion_r1680520000)
I should probably also disable the copy-constructor on `Sv2Client`?

📝 maflcko opened a pull request: "refactor: Make m_last_notified_header private"
(https://github.com/bitcoin/bitcoin/pull/30466)
Seems brittle to expose mutable fields public.

Fix it by making it private.

Fixes https://github.com/bitcoin/bitcoin/pull/30425#discussion_r1677633601

💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2232606942)
Now using mock sockets #30205, thanks @vasild!

💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2232609396)
Works like a charm! I'll see if I can add them to the tests in #30332 as well.

💬 hodlinator commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#discussion_r1680559019)
Good to see the removal of ` == true` as well in `InitBlocksdirXorKey()`. :+1:

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