💬 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
...
(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.
(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
...
(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
...
(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
...
(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.
(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.
(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
...
(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.
(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`?
(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
(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
(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?
(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.
(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
(https://github.com/bitcoin/bitcoin/pull/30835#pullrequestreview-2287088274)
ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46
👍 hebasto approved a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287096217)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe.
> Looks like this is not the solution for the reproducibility issue
The PR description rightfully stated that "this should not change behaviour".
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287096217)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe.
> Looks like this is not the solution for the reproducibility issue
The PR description rightfully stated that "this should not change behaviour".
📝 hebasto opened a pull request: "build: Use CMake's default permissions in macOS `deploy` target"
(https://github.com/bitcoin/bitcoin/pull/30838)
This PR is an attempt to fix https://github.com/bitcoin/bitcoin/issues/30815.
Based on https://github.com/bitcoin/bitcoin/pull/30823.
(https://github.com/bitcoin/bitcoin/pull/30838)
This PR is an attempt to fix https://github.com/bitcoin/bitcoin/issues/30815.
Based on https://github.com/bitcoin/bitcoin/pull/30823.
💬 TheCharlatan commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747656292)
Did you move these out of here because you'd otherwise get the following error:
```
*** Uncaught exception ***
capnp/compiler/node-translator.c++:1374: context: member.name = arg
kj/filesystem.c++:315: failed: expected parts.size() > 0 [0 > 0]; can't use ".." to break out of starting directory
stack: 59a492c93c94 59a492c93fc7 59a492c916b3 59a492a192f8 59a492b094db 59a492b08eb6 59a492b35fad 59a492b431c8 59a492b043ad 59a492b085a9 59a492b07e69 59a492b080d7 59a492b35b7f 59a492b36834 59a492b4302
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1747656292)
Did you move these out of here because you'd otherwise get the following error:
```
*** Uncaught exception ***
capnp/compiler/node-translator.c++:1374: context: member.name = arg
kj/filesystem.c++:315: failed: expected parts.size() > 0 [0 > 0]; can't use ".." to break out of starting directory
stack: 59a492c93c94 59a492c93fc7 59a492c916b3 59a492a192f8 59a492b094db 59a492b08eb6 59a492b35fad 59a492b431c8 59a492b043ad 59a492b085a9 59a492b07e69 59a492b080d7 59a492b35b7f 59a492b36834 59a492b4302
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334759967)
This is a confusing CI failure:
```
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-tx.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-cli.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoind.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-qt.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-util.1
+ false
```
https://cirrus-ci.com/task/6743063731634176
It's
...
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2334759967)
This is a confusing CI failure:
```
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-tx.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-cli.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoind.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-qt.1
-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-util.1
+ false
```
https://cirrus-ci.com/task/6743063731634176
It's
...
👍 TheCharlatan approved a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287108659)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe
(https://github.com/bitcoin/bitcoin/pull/30823#pullrequestreview-2287108659)
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe