💬 furszy commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2307260467)
> Could update the description and note that this bug was caused by https://github.com/bitcoin/bitcoin/pull/22217.

Sure. Done!.

> I think this error could also be triggered by editing settings.json and adding a non-string wallet name.

Yes, nice. Added coverage for it too.

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729027931)
nit: could be made a bit more readable by using `auto` and implicitly constructing `Hex`:

<details>
<summary>git diff on df92661444</summary>

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 644c8ffc72..050aa2a5a9 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -154,15 +154,14 @@ BOOST_AUTO_TEST_CASE(parse_hex)

// Basic test vector
std::vector<unsigned char> expected(std::begin(HEX_PARSE_OUTPUT), std::end(HEX_PARSE_OUTP
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1728787408)
Is there a specific reason to put `Hex` in the `detail` namespace? I think it could be useful to have `consteval` functions take a `Hex` parameter. For example, the `base_blob(string_view)` ctor can be quite elegantly deduplicated:

<details>
<summary>git diff on df92661444</summary>

```diff
diff --git a/src/uint256.h b/src/uint256.h
index 4124a34719..3616b0f9cb 100644
--- a/src/uint256.h
+++ b/src/uint256.h
@@ -16,6 +16,7 @@
#include <cstdint>
#include <cstring>
#include <optio
...

👍 stickies-v approved a pull request: "refactor: Replace ParseHex with consteval ""_hex literals"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2256959862)
ACK df92661444f46790b12d5061344d72106ef820d9 .

The new ""_hex literals approach makes for a much more ergonomic interface that is concise, clear and easy to use. Since the ""_hex operators are templated on the string literal, It does generate a lot of template instantiations, but since these are all consteval, I believe these should not end up in the binary, so that seems like a reasonable trade-off.

Left a few nits and a possible approach to further clean up the `base_blob::base_blob(str
...

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729071473)
nit in 6954b1f1143a73d4024f5564e287d8eeb34ff577: no more need for `MakeByteSpan`:

```suggestion
hw.write(G_uncompressed);
```

💬 stickies-v commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729082336)
Since we're templating these on `Hex` instances, this string literal approach will produce quite a rather large number of template instantiations. I wonder if keeping these `consteval` would also better allow the compiler to make sure these instantiations don't end up in the binary?

👍 fanquake approved a pull request: "test: Avoid duplicate curl call in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/30703#pullrequestreview-2257475032)
ACK fa5aeab3cb18405ecf8a1401d89539b924a618f6

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2307287556)
> Find1P1CPackage doesn't actually check the normal reject filter at all with respect to the child, only reconsiderable, so that assertion gets hit.

I think it might be more that `MempoolRejectedTx` doesn't remember if a tx failed for a different reason before considering an orphan?

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2307287794)
While I'm not a fan of mixed casing, I agree that it's probably outside the scope of current change.

ACK 18d65d27726bf9fc7629b8e794047a10c9cf6156

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130184)
done

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130331)
done

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1729130506)
done

💬 l0rinc commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1729138611)
> makes the definitions inconsistent and adds noise

I agree that it's likely just a documentation change for the reader (unsurprisingly, my trivial godbolt experiment also showed the same compiled output), but the methods *are* a bit different, isn't it useful signal if we mark them as such?

💬 fanquake commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2307312714)
~0. I'm not convinced this is something we need to add or maintain (also given existing options), or that if we did, this repository is the right place for it. Agree with the other comments.

👍 TheCharlatan approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2257534072)
ACK 4a76a8300f295e8eab0ea8ab1ea1580450f131c8

This has been the behaviour since reconsiderblock was introduced ~10 years ago https://github.com/bitcoin/bitcoin/commit/9b0a8d3152b43b63c99878d0223a1681993ad608#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R2176 .

⚠️ maflcko opened an issue: "Intermittent timeout on p2p_ibd_stalling.py"
(https://github.com/bitcoin/bitcoin/issues/30704)
On slow hardware (e.g. qemu s390x), the test may take long enough for a block timeout to hit.

Should be easy to fix with by using mocktime.

Test folder: https://drahtbot.space/temp_scratch/p2p_ibd_stalling_23.tar.zstd

Log:

```
node0 2024-08-22T06:47:16.815656Z [msghand] [net_processing.cpp:6260] [SendMessages] Timeout downloading block 18f9c9ae8651e9aeb776ec51943c7fb4b813b1dedae262459d1915f69c345f52 from peer=0, disconnecting
node0 2024-08-22T06:47:16.815964Z [msghand] [net_proc
...

📝 maflcko opened a pull request: "test: Avoid intermittent block download timeout in p2p_ibd_stalling"
(https://github.com/bitcoin/bitcoin/pull/30705)
Fixes #30704

The goal of the test is to check the stalling timeout, not the block download timeout.

On extremely slow hardware (for example qemu virtual hardware), downloading the 1023 blocks may take longer than the block download timeout.

Fix it by pinning the time using mocktime, and only advance it when testing the stalling timeout.

💬 pablomartin4btc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1729191860)
Building with `depends` on macOS setting the host as `arm64-apple-darwin` (following the notes in the readme file) I got confused noticing that the cross compilation was activated in the configuration (which was solved originally in https://github.com/hebasto/bitcoin/pull/125), then I checked in Ubuntu 22.04 which works fine and rerun the command on macOS without specifying `HOST=` and I found that in M3 the actual triplet is `aarch64-apple-darwin`, should we add it? Perhaps in a follow-up).

🤔 pablomartin4btc reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2257577490)
re-ACK 41051290ab3b6c36312cec26a27f787cea9961b4

Built successfully on macOS 14.4 with/ without `GUI` and legacy wallet. Noticed I didn't have `ccache` installed so I also played with/ without it and reviewed the `productivity.md` doc.

Also ran the tests with `ctest` and performed the `deploy` that had a in issue when compiling with `--target` identified by @fanquake [earlier](https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2288437679) and I've verified that works perfect now.

...

🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2257590987)
Reviewed up to e164614e32010bc8061cc99ed486188235efa780 (i.e. everything modulo fuzz and unit tests in the last 3 commits), lgtm so far.

Slightly off-topic, but: on some commits like 4e58d84da95d97d64f563b5c3e116769cd9ff89b `--color-moved=dimmed-zebra` didn't detect any lines as move-only for me (with 2-3 exceptions that seem completely arbitrary), any suggestions for advanced git options and/or recommendations for other fancy diff tools that can cope with that? (Or well, maybe I should just
...