💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757418027)
The comment is wrong. There is no `std::byte` in a string's char type.

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757416946)
nit: Instead of describing what the code does, it would be better to explain *why* it does something, where relevant. Here, it could say that string-REGEX-REPLACE` was found to be faster than file-APPEND or string-APPEND. But just a style nit, up to you.

💬 maflcko commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#issuecomment-2347038904)
> > it would be good to test this on Windows and Linux with the minimum and maximum supported cmake version to make sure this is really the last time this needs to be touched.
>
> While the speed difference is algorithmic, shouldn't be OS dependent, I would appreciate if someone would do that.

Some people claimed that string-APPEND was faster on Windows than file-APPEND, but it was the inverse on Linux. So I think it would be good to check on both platforms. (I can do Linux, but not Window
...

⚠️ fabioBaraDev opened an issue: "No such file or directory: bitcoind Error"
(https://github.com/bitcoin/bitcoin/issues/30891)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo

- [X] I still think this issue should be opened here

### Report

Hi guys, I am using a macbook Sonoma 14.0 and I am following the compilation tutorial, when I tried to run the Functional tests, I get this error:
```
FileNotFoundError: [Errno 2] No such file or directory: '/Users/user/Documents/personalProjects/bitcoin/src/bitcoind'
```
I already tried to run `make clean` and `make` again bu
...

💬 glozow commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2347058915)
> It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355

Should this fuzzer fail if I make this line return the result from `IsContinuationOfLowWorkHeadersSync`?
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/net_processing.cpp#L2900

💬 maflcko commented on issue "No such file or directory: bitcoind Error":
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347065300)
What are the exact steps to reproduce?

🤔 murchandamus reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2301194468)
utACK 69bf58dc0e25897e9fde435c9823a921590a90dc

Thanks for the additional examples and improving the documentation

💬 murchandamus commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1757439318)
I agree with @hodlinator, and this may be a bit verbose for something that is output to the console. How about:

```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path of the wallet directory (or a legacy wallet’s .dat file). Takes an absolute path or a path relative to the default wallet directory."},
```

💬 hebasto commented on issue "No such file or directory: bitcoind Error":
(https://github.com/bitcoin/bitcoin/issues/30891#issuecomment-2347066598)
> I am following the compilation tutorial, when I tried to run the Functional tests, I get this error:
>
> I also using v27.1

Are you using documentation for the same version you are building?

💬 jonatack commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2347070928)
@stratospher (needs rebase) happy to do a call and look at this together to move it forward if you like.

💬 ryanofsky commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2347083912)
Code review ACK 555513ac4e40f25b54d5f0473904adb23f4e3df7. This is great and I'm surprised it could be implemented so easily.

I do have questions about the second commit "log: Make format errors visible in debug builds" (fa0383761696d2c5c2e88208676cc993694fc1d4). I understand compile time checking should prevent most runtime errors, so the behavior in that commit of aborting the program when an invalid format string is specified is not as annoying for log-print debugging as it would be otherwi
...

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757479620)
removed them

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757479739)
I'm not surprised that regex replaces are faster (hence I didn't comment on that), but cmake syntax isn't my main preference, that's why I explained that instead.
I could explain the history, but the end result is so simple that it may not be needed.
If other reviewers require it, I'll add some extra comments, of course.

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757479784)
removed

🤔 hebasto reviewed a pull request: "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake"
(https://github.com/bitcoin/bitcoin/pull/30888#pullrequestreview-2301281287)
Changes look correct. Going to benchmark them on Windows tomorrow.

💬 hebasto commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757494865)
nit: Can be replaced with the `cmake_path()` command.

From CMake [docs](https://cmake.org/cmake/help/latest/command/get_filename_component.html):
> _Changed in version 3.20:_ This command has been superseded by the [`cmake_path()`](https://cmake.org/cmake/help/latest/command/cmake_path.html#command:cmake_path) command...

💬 hebasto commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757499489)
nit: Using the `string(REPEAT ...)` command to create a string of 16 dots can improve the code's clarity, I think.

💬 rot13maxi commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1757499656)
`SCRIPT_VERIFY_DISCOURAGE_OP_CAT` is acting as a CAT-specific `SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS` that can be turned off while still not relaying transasctions with other OP_SUCCESS opcodes. Since this is the first time we're carving out an OP_SUCCESS and doing this, it would be great to have a test that exercises the combinations of SCRIPT_VERIFY_DISCOURAGE_OP_CAT and SCRIPT_VERIFY_OP_CAT

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757506396)
I thought of that but couldn't find a way to do it inline - and defing a new variable in a separate line just for this makes it less readable
```cmake
string(REPEAT "." 16 line_width)
string(REGEX REPLACE "${line_width}" "\\0\n" formatted_bytes "${hex_content}")
```

💬 l0rinc commented on pull request "build: optimize .h generation in GenerateHeaderFrom{Raw,Json}.cmake":
(https://github.com/bitcoin/bitcoin/pull/30888#discussion_r1757521241)
Done, thanks