💬 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`

💬 katesalazar commented on issue "Closing a wallet using the fa46088440 28.x QT client segfaults":
(https://github.com/bitcoin/bitcoin/issues/30887#issuecomment-2347251385)
I came back to fa46088440, then reverted 5d15485a on top, _looks like healed_.