🤔 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

💬 mzumsande commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347161468)
can you rule out that someone else on the network just mined a block, and the inconclusive `getblocktemplate` result belongs to a previous call that was sent off before that new block was connected?

💬 fjahr commented on pull request "scripted-diff: Modernize nLocalServices to m_local_services":
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757532379)
Hm, ok, I interpreted the comment on the namespace above as that we ignore that here. But I missed the variable below and I the others are just older. Changed.

💬 davidgumberg commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2347208216)
I think that there are also some calls to `std::rewind`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/test/streams_tests.cpp#L264

and `std::fgetc`:
https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/node/utxo_snapshot.cpp#L76

that are problematic if we are caching file position.

Also, if it seems sensible to mitigate the risk of someone modifying the file position in the future without changing `m_position`
...

🤔 furszy reviewed a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2301395429)
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b

It seems that with the latest changes, the `deleteRwSettings` is no longer used.
It would be nice to add test coverage for it and for the null value usage as well.

👍 marcofleon approved a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2301402833)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5

I took a look at c308e3200d6968be553ed031d09b4bccb46b504b and 2e0b6742b82e60ea685afd25f2d19b8b558678ce in https://github.com/bitcoin/bitcoin/pull/30116 (assuming those commits are mostly final) to understand a bit better how `m_is_inbound` will be used for Erlay. Based on that, I'd say the changes in this PR make sense.

🤔 furszy reviewed a pull request: "scripted-diff: Modernize nLocalServices to m_local_services"
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301405397)
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca