💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348809433)
review ACK 2a581144f28bad44de40122864f2f7b9fc5000de 👒

<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=
trusted comment: review ACK 2a581144f28b
...

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348832162)
(Also tested on a fresh Ubuntu 24.04 VM and saw a 30x speedup, which is useful, because this part isn't cached by ccache between builds)

💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2348843336)
> Locally these tests take about 20 seconds and appear in the correct bucket.

I think the sorting isn't really based on the "correct bucket", but rather the inverse overall duration, the buckets are just a guideline/reminder. That is, as long as the longest running test is started first, everything is fine.


Did you measure if this actually makes a meaningful difference?

👍 hebasto approved a pull request: "build: add `standard branch-protection` to hardening flags for aarch64-linux"
(https://github.com/bitcoin/bitcoin/pull/30433#pullrequestreview-2303002464)
ACK 001b1cf010453adbb1316a6fa8911398953afe61.

💬 maflcko commented on pull request "test: re-bucket p2p_node_network_limited":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2348847390)
Also, could rebase for a fresh CI? At least for me the CI timeouts went away earlier this night when trying to reproduce, but I don't know if they are really gone, and whether the issue was inside this repo, or on the CI runners, or on the CI backend software, or on the Cirrus/GHA integration.

💬 maflcko commented on pull request "doc: unit test runner help fixup":
(https://github.com/bitcoin/bitcoin/pull/30890#issuecomment-2348854269)
review ACK 0024d2c6ea0fafe9b9949af4bbcd0c583e580746

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758844489)
thanks for the feedback!

I updated this to
```
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
cmake -B build --toolchain=depends/x86_64-pc-linux-gnu/toolchain.cmake
cmake --build build
build/src/bitcoind -regtest -printtoconsole -debug=ipc
BITCOIND=$(pwd)/build/src/bitcoind build/test/functional/test_runner.py
```

Which does start running the tests please let me know if this looks ok

👍 hebasto approved a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2303103922)
ACK 2a581144f28bad44de40122864f2f7b9fc5000de.

The new scripts introduce trailing spaces in the generated header, but these are not essential.

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758847279)
thanks! I removed that in [5a85022](https://github.com/bitcoin/bitcoin/pull/30875/commits/5a85022cc6c315e9c756bbbb63a7ed992e7f23d3)

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758851197)
thanks! I updated this to `cmake build` instead of `configure script` in [5a85022](https://github.com/bitcoin/bitcoin/pull/30875/commits/5a85022cc6c315e9c756bbbb63a7ed992e7f23d3)

let me know if you'd prefer different wording!

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#discussion_r1758851652)
thanks! I updated this to `cmake build` instead of `configure script` in [5a85022](https://github.com/bitcoin/bitcoin/pull/30875/commits/5a85022cc6c315e9c756bbbb63a7ed992e7f23d3)

let me know if you'd prefer different wording!

💬 laanwj commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2348948414)
> I made a significant change here, making m_position an std::optional<int64_t>, which is std::nullopt when the position is unknown (due to ftell failing, and no successful fseek afterwards).

i first couldn't think of a valid scenario where ftell would fail but we'd want to continue, but i guess this maks sense when reading a FIFO or device, when there isn't really an offset.

💬 kevkevinpal commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2348968829)
> It would be nice to fix all remaining instances in one go. See also [#30664 (comment)](https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2320892444) and [#30664 (comment)](https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2321610186)

There are still some changes I need to address from these comments

⚠️ fanquake opened an issue: "translations: `28.x` update pulled in random strings?"
(https://github.com/bitcoin/bitcoin/issues/30897)
Transations were updated for 28.x in #30715.

Looks like that update in things that are not translations. i.e:

https://github.com/bitcoin/bitcoin/blob/e43ce250c6fb65cc0c8903296c8ab228539d2204/src/qt/locale/bitcoin_gl_ES.ts#L6

?

💬 fanquake commented on pull request "qt: 28.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/30715#issuecomment-2348975779)
See #30897. This has pulled broken strings.

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2348983390)
> The new scripts introduce trailing spaces in the generated header, but these are not essential.

In theory the spaces in the data itself could be dropped completely, because for the compiler (and a human reader) a single `,` without space is sufficient. But I agree it doesn't matter, so probably best to leave as-is for now.

🤔 BrandonOdiwuor reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2303165380)
Concept ACK

💬 hebasto commented on issue "translations: `28.x` update pulled in random strings?":
(https://github.com/bitcoin/bitcoin/issues/30897#issuecomment-2348987750)
> Transations were updated for 28.x in #30715.
>
> Looks like that update pulled in things that are not translations. i.e:
>
> https://github.com/bitcoin/bitcoin/blob/e43ce250c6fb65cc0c8903296c8ab228539d2204/src/qt/locale/bitcoin_gl_ES.ts#L6
>
> ?

This is a poor / malicious translation.

https://app.transifex.com/bitcoin/bitcoin/translate/#gl_ES/qt-translation-028x/508593963:

💬 maflcko commented on issue "translations: `28.x` update pulled in random strings?":
(https://github.com/bitcoin/bitcoin/issues/30897#issuecomment-2348993105)
Aren't LLMs capable of translation? With all the hype around them I wonder if a script can be written to check that each translation pair is a valid translation. With 4o-mini the cost should also be trivial.

💬 fanquake commented on pull request "doc: fixed inconsistencies in documentation between autotools to cmake change":
(https://github.com/bitcoin/bitcoin/pull/30875#issuecomment-2348994520)
Also in `test_deterministic_coverage.sh`: `Run \"./configure --enable-lcov\"`