💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386732584)
> > The only exception may be the msan build, where the compiler is compiled.
> > (Just leaving a reply for context, in case someone reads the thread and stumbles upon this)
>
> Oh great, that will help simplify things a little then, as we can use more defaults. Not sure what I must've been looking at to come to the wrong conclusion here, but thanks for double-checking.
It isn't wrong. I presume it may still have to be set for the msan task, but this can probably be done in a follow-up an
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386732584)
> > The only exception may be the msan build, where the compiler is compiled.
> > (Just leaving a reply for context, in case someone reads the thread and stumbles upon this)
>
> Oh great, that will help simplify things a little then, as we can use more defaults. Not sure what I must've been looking at to come to the wrong conclusion here, but thanks for double-checking.
It isn't wrong. I presume it may still have to be set for the msan task, but this can probably be done in a follow-up an
...
📝 laanwj opened a pull request: "net: Use GetAdaptersAddresses to get local addresses on Windows"
(https://github.com/bitcoin/bitcoin/pull/31014)
Instead of a `gethostname` hack, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Suggested by Ava Chow.
Addiional changes:
- Add optional length checking to `CService::SetSockAddr` calls: In almost all cases (the only exception is `getifaddrs`), we know the size of the data passed into `SetSockAddr`, so we can check this to be what is expected, before reading the memory.
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and u
...
(https://github.com/bitcoin/bitcoin/pull/31014)
Instead of a `gethostname` hack, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Suggested by Ava Chow.
Addiional changes:
- Add optional length checking to `CService::SetSockAddr` calls: In almost all cases (the only exception is `getifaddrs`), we know the size of the data passed into `SetSockAddr`, so we can check this to be what is expected, before reading the memory.
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and u
...
👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2341163656)
re-ACK 221296c1f0917bcf4da8fa0bbc5494a28dc6b697
`git range-diff master b9aded4 221296c`
Just improving precision of filename argument description.
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2341163656)
re-ACK 221296c1f0917bcf4da8fa0bbc5494a28dc6b697
`git range-diff master b9aded4 221296c`
Just improving precision of filename argument description.
📝 theuni opened a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015)
See [Upstream docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_TEST_ALL_DEPENDENCY.html) for specifics.
Unfortunately, this **seems to have no effect on `ctest`** :(
This brings the test -> hack -> test cycle more inline with how it worked with autotools.
With `CMAKE_SKIP_TEST_ALL_DEPENDENCY` set to FALSE, `make test` will trigger a rebuild, ensuring that test binaries are current before running them.
To test:
```
cmake -S . -B build
make -C build -j24
touch src/prim
...
(https://github.com/bitcoin/bitcoin/pull/31015)
See [Upstream docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_TEST_ALL_DEPENDENCY.html) for specifics.
Unfortunately, this **seems to have no effect on `ctest`** :(
This brings the test -> hack -> test cycle more inline with how it worked with autotools.
With `CMAKE_SKIP_TEST_ALL_DEPENDENCY` set to FALSE, `make test` will trigger a rebuild, ensuring that test binaries are current before running them.
To test:
```
cmake -S . -B build
make -C build -j24
touch src/prim
...
💬 theuni commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386752224)
Ping @hebasto .
Also ping @fanquake as I think you've complained about this.
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386752224)
Ping @hebasto .
Also ping @fanquake as I think you've complained about this.
💬 hebasto commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386754681)
Does this solution support parallelism in the test phase?
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386754681)
Does this solution support parallelism in the test phase?
💬 theuni commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386761343)
With `ARGS=-jX`, yes.
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386761343)
With `ARGS=-jX`, yes.
💬 maflcko commented on pull request "ci: Approximate MAKEJOBS in image build phase":
(https://github.com/bitcoin/bitcoin/pull/30935#issuecomment-2386766435)
This may or may not be needed for https://github.com/bitcoin/bitcoin/issues/30852, but it could be useful even outside of that, if someone wanted to set `MAKEJOBS` when building the image.
(https://github.com/bitcoin/bitcoin/pull/30935#issuecomment-2386766435)
This may or may not be needed for https://github.com/bitcoin/bitcoin/issues/30852, but it could be useful even outside of that, if someone wanted to set `MAKEJOBS` when building the image.
💬 laanwj commented on pull request "depends: For mingw cross compile use -gcc-posix to prevent library conflict":
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386785047)
Yes, that seems similar! It would work perfectly fine without adding any `-L` spec, the linker knows where to find its libraries, no need to add the implicit link directories explicitly at all, but it does and it breaks.
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386785047)
Yes, that seems similar! It would work perfectly fine without adding any `-L` spec, the linker knows where to find its libraries, no need to add the implicit link directories explicitly at all, but it does and it breaks.
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2386800235)
Updated with the following changes:
- key is now `siphash(k, spent outpoint)` (8 bytes) where k is a random value created when the index is created, to defend against potential collisions attacks. Value is still a tx position (8 bytes)
- `gettxspendingprevout` will return the full spending tx if option ` include_tx` is set
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2386800235)
Updated with the following changes:
- key is now `siphash(k, spent outpoint)` (8 bytes) where k is a random value created when the index is created, to defend against potential collisions attacks. Value is still a tx position (8 bytes)
- `gettxspendingprevout` will return the full spending tx if option ` include_tx` is set
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386850370)
> Apart from the inital clone+apt, the tasks are not IO-bound (at least not on Hetzner Cloud).
I know in docker you can cache apt, I wonder if we can do the same on podman (having difficulty finding that they support the exact same flags)? e.g. in docker you can:
```bash
RUN --mount=target=/var/lib/apt/lists,type=cache,sharing=locked \
--mount=target=/var/cache/apt,type=cache,sharing=locked \
```
We'd need likely to delineate between Ubuntu versions etc. and this may only help wi
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386850370)
> Apart from the inital clone+apt, the tasks are not IO-bound (at least not on Hetzner Cloud).
I know in docker you can cache apt, I wonder if we can do the same on podman (having difficulty finding that they support the exact same flags)? e.g. in docker you can:
```bash
RUN --mount=target=/var/lib/apt/lists,type=cache,sharing=locked \
--mount=target=/var/cache/apt,type=cache,sharing=locked \
```
We'd need likely to delineate between Ubuntu versions etc. and this may only help wi
...
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783412030)
Isn't `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` set when using `-DBUILD_FOR_FUZZING=ON`?
```
if(BUILD_FOR_FUZZING)
message(WARNING "BUILD_FOR_FUZZING=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON.")
set(BUILD_DAEMON OFF)
set(BUILD_CLI OFF)
set(BUILD_TX OFF)
set(BUILD_UTIL OFF)
set(BUILD_UTIL_CHAINSTATE OFF)
set(BUILD_KERNEL_LIB OFF)
set(BUILD_WALLET_TOOL OFF)
set(BUILD_GUI OFF)
set(ENABLE_EXTERNAL_SIGNER OFF)
set(WITH_MINIUPNPC OFF)
set
...
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783412030)
Isn't `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` set when using `-DBUILD_FOR_FUZZING=ON`?
```
if(BUILD_FOR_FUZZING)
message(WARNING "BUILD_FOR_FUZZING=ON will disable all other targets and force BUILD_FUZZ_BINARY=ON.")
set(BUILD_DAEMON OFF)
set(BUILD_CLI OFF)
set(BUILD_TX OFF)
set(BUILD_UTIL OFF)
set(BUILD_UTIL_CHAINSTATE OFF)
set(BUILD_KERNEL_LIB OFF)
set(BUILD_WALLET_TOOL OFF)
set(BUILD_GUI OFF)
set(ENABLE_EXTERNAL_SIGNER OFF)
set(WITH_MINIUPNPC OFF)
set
...
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386938805)
> I know in docker you can cache apt
Interesting. Though, I think the apt is rarely called, and would be just 30 seconds anyway (https://cirrus-ci.com/task/5200852661567488?logs=ci#L199). The reason the image cache exists is that network error are avoided and the expensive llvm build is cached.
> resilient in terms of accessing a remote cache dir (it always maintains a local cache
It would be nice if the local cache was also shared, but I guess this can be done in the future, if nee
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386938805)
> I know in docker you can cache apt
Interesting. Though, I think the apt is rarely called, and would be just 30 seconds anyway (https://cirrus-ci.com/task/5200852661567488?logs=ci#L199). The reason the image cache exists is that network error are avoided and the expensive llvm build is cached.
> resilient in terms of accessing a remote cache dir (it always maintains a local cache
It would be nice if the local cache was also shared, but I guess this can be done in the future, if nee
...
💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783452154)
> DBUILD_FOR_FUZZING
This isn't set for the hfuzz net driver. It uses the `HFND_FUZZING_ENTRY_FUNCTION_CXX`
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783452154)
> DBUILD_FOR_FUZZING
This isn't set for the hfuzz net driver. It uses the `HFND_FUZZING_ENTRY_FUNCTION_CXX`
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783476135)
Yes, missed that. Just added it.
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783476135)
Yes, missed that. Just added it.
💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2387010267)
Here's an initial implementation of a `bitcoin` wrapper executable: commit 65e49f4579ee823284bf04f1295b59116acc727b [(branch)](https://github.com/ryanofsky/bitcoin/commits/pr/wrap)
This should be purely additive, and to willcl-ark's point, not break backwards compatibility. It just lets us add new binaries to a libexec/ directory instead of bin/.
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2387010267)
Here's an initial implementation of a `bitcoin` wrapper executable: commit 65e49f4579ee823284bf04f1295b59116acc727b [(branch)](https://github.com/ryanofsky/bitcoin/commits/pr/wrap)
This should be purely additive, and to willcl-ark's point, not break backwards compatibility. It just lets us add new binaries to a libexec/ directory instead of bin/.
📝 brunoerg converted_to_draft a pull request: "net: fuzz: bypass network magic and checksum validation"
(https://github.com/bitcoin/bitcoin/pull/31012)
Fixes #30957
We currently have some instructions on the documentation about how to fuzz the Bitcoin Core P2P layer using Honggfuzz NetDriver. It includes a patch to be applied to bypass network magic and checksum validation, that is outdated. However, we can change the code to bypass them using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
(https://github.com/bitcoin/bitcoin/pull/31012)
Fixes #30957
We currently have some instructions on the documentation about how to fuzz the Bitcoin Core P2P layer using Honggfuzz NetDriver. It includes a patch to be applied to bypass network magic and checksum validation, that is outdated. However, we can change the code to bypass them using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. It also makes the `p2p_transport_serialization` fuzz target simpler since we do not need to assist the network magic and checksum creation anymore.
💬 brunoerg commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2387029412)
Just moved it to draft to get some conceptual feedbacks first. I added a commit to bypass some asserts in `ConnmanTestMsg` that would fail since we're bypassing network magic and checksum validation. Also, I think we will have to change `p2p_transport_bidirectional` target since there are some sanity checks that would fail with the bypasses. Any thoughts?
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2387029412)
Just moved it to draft to get some conceptual feedbacks first. I added a commit to bypass some asserts in `ConnmanTestMsg` that would fail since we're bypassing network magic and checksum validation. Also, I think we will have to change `p2p_transport_bidirectional` target since there are some sanity checks that would fail with the bypasses. Any thoughts?
💬 mooncityorg commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2387034541)
CMAKE_SKIP_TEST_ALL_DEPENDENCY is false? @bitcoin-core-merge-script
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2387034541)
CMAKE_SKIP_TEST_ALL_DEPENDENCY is false? @bitcoin-core-merge-script
💬 antonilol commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2387041009)
That's a good way to prevent attackers from inserting collisions in everyone's database, the chance of collision will be dependent on k, which is unknown to an attacker and different for everyone (except by chance or people copying each other's database). Shouldn't 8 bytes of randomness be enough (instead of 16)? Assuming that siphash's output is uniformly distributed, more randomness than the output size (8 bytes) feels unnecessary.
Also, the rpc description should be updated, https://github
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2387041009)
That's a good way to prevent attackers from inserting collisions in everyone's database, the chance of collision will be dependent on k, which is unknown to an attacker and different for everyone (except by chance or people copying each other's database). Shouldn't 8 bytes of randomness be enough (instead of 16)? Assuming that siphash's output is uniformly distributed, more randomness than the output size (8 bytes) feels unnecessary.
Also, the rpc description should be updated, https://github
...