💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756741748)
Thanks, fixed!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756741980)
Thanks, fixed typo

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756754331)
to avoid an infinite loop when the linking is incorrect (e.g. next returns itself), we could `assert(count_linked++ < count_flagged)`;

💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756759971)
Yes, absolutely! But, this PR or something else would need to be merged first, or we would need to have a method `SetFresh` because there's still a case where we have FRESH but not DIRTY.

I think we should keep the internal flags as a bitmap, since if we introduce new fields for each state it will increase the memory footprint of each `CoinsCachePair`.

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756770827)
> because there's still a case where we have FRESH but not DIRTY.

I only see https://github.com/bitcoin/bitcoin/blob/34a101d4babd0cd2df86b414da385c3bded84027/src/coins.cpp#L204-L210 and https://github.com/bitcoin/bitcoin/blob/34a101d4babd0cd2df86b414da385c3bded84027/src/coins.cpp#L96 in production code - in both cases we're only setting `FRESH` together with `DIRTY`.
Did I miss anything in production code?

> I think we should keep the internal flags as a bitmap, since if we introduce new
...

💬 m3dwards commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#discussion_r1756773854)
I'll take it

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1756777516)
> Would be good to add a comment explaining `if(CMAKE_GENERATOR MATCHES "Ninja|Makefiles")` saying that COMPILER_LAUNCHER variable only works for these backends not other backends. Otherwise the check seems arbitrary and doesn't seem to make sense.

Thanks! Done.

🤔 maflcko reviewed a pull request: "test: Drop no longer needed workarounds"
(https://github.com/bitcoin/bitcoin/pull/30847#pullrequestreview-2300178606)
lgtm

💬 maflcko commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#discussion_r1756777352)
Just for reference. This never worked in the first place, because "Skipping" was never picked up, because logging was disabled until it was re-added in commit f03c9420958de31fdfecec5fa3e23134aac61803

💬 fanquake commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756779260)
When `-DBUILD_FOR_FUZZING=ON` is passed.

💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346184415)
review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756

👍 stickies-v approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300018207)
ACK fa37d9b0fd0b8f1b23163887618b2fa1a817d596

I like the new test structure and added documentation, thanks! Left some non-blocking nits, only if you need to force-push.

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756687137)
nit: in this new new structure, `err_mix` makes more sense than `check_mix`

<details>
<summary>`s/check_/err_/g`</summary>

```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index 4c61632fce..f398404d75 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -62,25 +62,25 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
PassFmt<2>("%2$*3$d");
PassFmt<1>("%.*f");

- auto check_mix{"Format specifiers
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756671121)
nit: `%8$` is not a valid specifier
```suggestion
// Positional specifier, like %8$s
```

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756690672)
+1 for focusing on run-time errors, formatting errors aren't worth spending much time on imo.

💬 fanquake commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2346192841)
Guix Build
```bash
d4baa1d2e9ee7abeb60d115dd52c6bc7d02e8116fe4b94c0f040371321e37b80 guix-build-253a691441dd/output/aarch64-linux-gnu/SHA256SUMS.part
7c38410b9468799b74371ba195f34121a4789e917dcf611f2ccc4b81f8a63eee guix-build-253a691441dd/output/aarch64-linux-gnu/bitcoin-253a691441dd-aarch64-linux-gnu-debug.tar.gz
4a6ec8df80822e5938b1a55f2e08a945b12a9d4f8b2f1bad05ea8be90a3b6173 guix-build-253a691441dd/output/aarch64-linux-gnu/bitcoin-253a691441dd-aarch64-linux-gnu.tar.gz
48de6fea5e2d35be4
...

💬 hebasto commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756794139)
Could you please suggest your alternative?

💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2346210098)
re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

CI timeout issues seem unrelated.

📝 theStack opened a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877)
Updates the .editorconfig file, first introduced in 2021 (see PR #21123, commit 7a135d57) w.r.t. following changes:
- consider Rust .rs files (relevant since #28076, commit bbbbdb0c)
- reflect build system change to CMake (#30454, #30664)
- add setting for bare Makefile still used for depends builds

Can be tested e.g. by using the editorconfig-vim plugin (https://github.com/editorconfig/editorconfig-vim). The PR is made under the assumption that the file is still considered useful, especia
...

💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2346225294)
I put some time into implementing 3 variations on different personal branches:

### [unchanged_key_CDiskTxPos](https://github.com/bitcoin/bitcoin/compare/20b259332ff1d2e6ca6b291ffeda00b51497bbed...hodlinator:bitcoin:pr/24539_unchanged_key_CDiskTxPos) / baf57fb3a45c97d6cfb531b9252c97d4f27ad7ae

`uint256` value -> `CDiskTxPos`

Still have key of `COutPoint`: 32 + 4 = 36 bytes
old value - `uint256`: 32 bytes
new value - `CDiskTxPos`: 4 + 4 + 4 = 12 bytes

36+32 -> 36+12 => 29% theoretical
...