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

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747630899)
It is capitalized to match the substitution in the pkg-config file. I had the impression the substitution variables are usually capitalized as a convention.

💬 TheCharlatan commented on pull request "kernel: Create usable static kernel library":
(https://github.com/bitcoin/bitcoin/pull/30814#issuecomment-2334702440)
Updated 766881e33d13ca2887f7ed179cdcc6bc007a6325 -> 0dd16d7118f10ac291a45644769121cbdfd2352f ([staticKernel_0](https://github.com/TheCharlatan/bitcoin/tree/staticKernel_0) -> [staticKernel_1](https://github.com/TheCharlatan/bitcoin/tree/staticKernel_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/staticKernel_0..staticKernel_1))

* Addressed @hebasto's [comment](https://github.com/bitcoin/bitcoin/pull/30814#discussion_r1747381222), fixed spacing around `if` statements.
* Address
...

👍 hebasto approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2287050210)
ACK 0dd16d7118f10ac291a45644769121cbdfd2352f.

💬 TheCharlatan commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747648065)
Why call this `Kernel` and not `bitcoinkernel`?

🤔 TheCharlatan reviewed a pull request: "test: Work around boost compilation error"
(https://github.com/bitcoin/bitcoin/pull/30834#pullrequestreview-2287065362)
Nice, post-merge ACK

👍 TheCharlatan approved a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509#pullrequestreview-2287066648)
Re-ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

💬 hebasto commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747651146)
For brevity. Do you want me to update it?

💬 TheCharlatan commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#discussion_r1747665388)
I don't think it matters really what it is called. As far as I know there is no way to list all available install components from the command line and discover its name. With the targets you can at least do `cmake --build build --target help`, so my idea was that this might make it more discoverable. But a component might also include multiple targets, so I don't think colliding their names here really helps in terms of making it clearer. So in short, no need to change.

👍 TheCharlatan approved a pull request: "build: Introduce "Kernel" installation component"
(https://github.com/bitcoin/bitcoin/pull/30835#pullrequestreview-2287088274)
ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46