💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747371536)
Ah yes, according to https://eel.is/c++draft/expr.const.cast#6

I still have a slight preference to keep the const unless removing it has a clear benefit.

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747373351)
Very good points, in this version I can indeed add back the `const`, thanks!
Also added a simple test to make sure `CScript() << _hex_v_u8` and `CScript() << _hex` produce the same `CScript()` (can be removed if it passes CI and is deemed superfluous)
See: https://github.com/bitcoin/bitcoin/compare/14080bb53d649ed1c99f121f377d2c8f2019c99e..03bdefff8c6663da895d0603469dee15aa4ad35d

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1747378139)
Agree, in C++23 we can just use `.contains` which would simplify this slightly

🤔 hebasto reviewed a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2286670859)
Approach ACK 766881e33d13ca2887f7ed179cdcc6bc007a6325.

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747381222)
style nit: All other CMake code uses the `if()` command without an additional space.

💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747385054)
The last push looks fine, but I still have a slight preference to use UCharSpanCast or std::as_bytes, which preserve constness by default.

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747393816)
```suggestion
set(LIBS_PRIVATE "")
```
if there is no intention to let the user set it as a cache variable.

(same for `all_target_static_link_libs` a few lines above)

Why this local variable name capitalized?

💬 hebasto commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747397241)
style: A shorter alternative:
```suggestion
string(APPEND LIBS_PRIVATE " -l${lib}")
```

👍 hebasto approved a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827#pullrequestreview-2286729300)
ACK 37a6739203ea3da394acef38b18a794e67543a90.

💬 luke-jr commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1747435819)
These are identical now. `wine_runtime` seems no longer needed at all?

⚠️ jlest01 opened an issue: "Misleading index error message"
(https://github.com/bitcoin/bitcoin/issues/30836)
When running node with the `txindex` parameter for the first time, we see the following message in the log:
```ERROR: Commit: Failed to commit latest txindex state```

This misleads the user into believing that something is wrong, but the indexing process normally begins shortly thereafter.
It appears that the reason for this message is that there is no index yet. Since `src/index/base.h` is common to all indexes, this message may appear in all indexes.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1747533292)
Hmm there are a lot of entry points to `FlushStateToDisk`, and many places where `SetBestBlock` is called. I think this PR should leave this as is to not complicate review.

I was thinking we could remove the `full_flush_completed` variable if we didn't have this check.

📝 ariard opened a pull request: "Add NODE_TXRELAY_V2."
(https://github.com/bitcoin/bitcoin/pull/30837)
This is the top commit from #30572, where the new node service bit support commit is extracted on its own.

See the corresponding BIP draft for motivation: https://github.com/bitcoin/bips/pull/1663

Dissociating this change from halting the processing of unrequested transaction, allow the node service bit support to be used for further policies and mechanisms, beyond this mechanism only.

📝 ariard converted_to_draft a pull request: "Add NODE_TXRELAY_V2."
(https://github.com/bitcoin/bitcoin/pull/30837)
This is the top commit from #30572, where the new node service bit support commit is extracted on its own.

See the corresponding BIP draft for motivation: https://github.com/bitcoin/bips/pull/1663

Dissociating this change from halting the processing of unrequested transaction, allow the node service bit support to be used for further policies and mechanisms, beyond this mechanism only.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2334631685)
@mzumsande

> That has nothing to do with free relay, because it doesn't propagate to the rest of the network (hence the "relay" in the name). It's also not a interesting attack because there are dozens of ways for an attacker to waste both its own and its peer's bandwidth when it is on one end of the connection:

Sure, technically this `GETDATA` duplication alone isn't a free relay bandwidth, in the sense that a transaction is relayed many times over the network with no feerate / absolute
...

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2334633632)
> Would be nice to change the title from "... transactions v2" to something else. Otherwise, it may be a bit confusing in the context of "v2 transactions" (transactions with version number set to 2, not 3), or "v2 transport" (the P2P thing).

See https://github.com/bitcoin/bitcoin/pull/30837, as the signaling support for transaction V2 can be discussed separately from halting the processing of unrequested transactions.

💬 TheCharlatan commented on pull request "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage":
(https://github.com/bitcoin/bitcoin/pull/30823#issuecomment-2334683378)
Looks like this is not the solution for the reproducibility issue:
```
972bb22e9c3e5fc8858c8351dae323000604cc05dc1bb2b24a35c558d5c20c5d guix-build-1f054eca4e77/output/arm64-apple-darwin/SHA256SUMS.part
53379e0ac143a7c150563ac10973ee7af5dc827e53f96a8b2519cdf1992ece63 guix-build-1f054eca4e77/output/arm64-apple-darwin/bitcoin-1f054eca4e77-arm64-apple-darwin-unsigned.tar.gz
34439d69eb914fb4e758af68f7ee543da1305409621ff34df0b95d785ba03ba3 guix-build-1f054eca4e77/output/arm64-apple-darwin/bitco
...

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747621990)
`std::as_bytes` converts *to* `std::span<const std::byte>`, but I need a `const std::span<const unsigned char>` since I can only call `prevector#insert` with `class iterator { T* ptr{};` which isn't trivial to migrate to a byte span as far as I can tell.
And `UCharSpanCast` seems to work with `Span`, which I understood we're trying to get away from.
Or I could do
```C++
return *this << std::span<const uint8_t>(UCharCast(b.data()), b.size());
```
instead of the current:
```C++
return *thi
...

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#issuecomment-2334690307)
Re https://github.com/bitcoin/bitcoin/pull/30814#issuecomment-2334333003Z

> While installing libbitcoinkernel.pc, why ignore CMake's configs for downstream projects using CMake?

Do you mean why I am not contributing a cmake configuration file as well in this pull request? Or is there something wrong with the current installation step that precludes also installing a configuration file? I just did not have the need for a cmake configuration file so far,. It would be good to contribute one e
...

💬 mzumsande commented on issue "Misleading index error message":
(https://github.com/bitcoin/bitcoin/issues/30836#issuecomment-2334690558)
This should be fixed already in master and for v28: See #29671.