💬 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

💬 furszy commented on pull request "scripted-diff: Modernize nLocalServices to m_local_services":
(https://github.com/bitcoin/bitcoin/pull/30885#discussion_r1757575935)
tiny nit:
I know this is an scripted-diff but.. could call `GetLocalServices()` here.

💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2347235405)
I believe that's answered positively by the timestamps and structure of the
calling code.

A better test would be to reproduce this with regtest which I'll create a
script to demonstrate soon.

But at the same time it seems there should be a mutex around the chain tip.
If there isn't this is basically guaranteed to happen. I haven't looked yet.

On Thu, Sep 12, 2024, 4:18 PM Martin Zumsande ***@***.***>
wrote:

> can you rule out that someone else on the network just mined a block, a
...

💬 katesalazar commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347237302)
In my environment, 5d15485a is a crashing rev
and inmediate ancestor 1a41e635 is a non-crashing rev.

🤔 jonatack reviewed a pull request: "scripted-diff: Modernize nLocalServices to m_local_services"
(https://github.com/bitcoin/bitcoin/pull/30885#pullrequestreview-2301419943)
Review ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca

Following the last push, suggest renaming the pull title to `scripted-diff: Modernize nLocalServices naming`