💬 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
...
💬 laanwj commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2387085025)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2387085025)
Concept ACK
💬 fjahr commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2387131238)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2387131238)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5
💬 davidgumberg commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD on some configurations":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2387137498)
> I tested this PR on an AMD Ryzen 7950x machine with Ubuntu 24.04, with one local network peer and a gigabit internet connection.
>
> ```
> bitcoind -dbcache=30000 -stopatheight=863000 -blocksdir=/magnetic/.bitcoin -addnode=local-network
> ```
>
> Before: 5 hours 10 minutes After: 4 hours and 55 minutes
>
> Time includes 20 minutes to flush chainstate to disk during shutdown.
@Sjors Thanks for testing! I suspect that the biggest improvements will be seen on memory bandwidth constr
...
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2387137498)
> I tested this PR on an AMD Ryzen 7950x machine with Ubuntu 24.04, with one local network peer and a gigabit internet connection.
>
> ```
> bitcoind -dbcache=30000 -stopatheight=863000 -blocksdir=/magnetic/.bitcoin -addnode=local-network
> ```
>
> Before: 5 hours 10 minutes After: 4 hours and 55 minutes
>
> Time includes 20 minutes to flush chainstate to disk during shutdown.
@Sjors Thanks for testing! I suspect that the biggest improvements will be seen on memory bandwidth constr
...
🤔 ismaelsadeeq reviewed a pull request: "test: switch MiniWallet padding unit from weight to vsize"
(https://github.com/bitcoin/bitcoin/pull/30718#pullrequestreview-2341539943)
post-merge ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
This PR simplify `_bulk_tx` a lot.
(https://github.com/bitcoin/bitcoin/pull/30718#pullrequestreview-2341539943)
post-merge ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
This PR simplify `_bulk_tx` a lot.
📝 mzumsande opened a pull request: "test: add missing sync to feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31016)
This fixes a race:
- In the `test_estimate_dat_is_flushed_periodically` subtest, node 0 is isolated and creates 10 blocks (no sync).
- In `clear_estimates` the nodes are reconnected (but we don't wait for them to sync!)
- In the `sanity_check_rbf_estimates` subtest, node 1 generates another block and syncs with the other nodes. The sync fails if the generated block is at the same height as the tip of the node.
Fix this by adding a sync to `clear_estimates`.
Fixes #30990
(https://github.com/bitcoin/bitcoin/pull/31016)
This fixes a race:
- In the `test_estimate_dat_is_flushed_periodically` subtest, node 0 is isolated and creates 10 blocks (no sync).
- In `clear_estimates` the nodes are reconnected (but we don't wait for them to sync!)
- In the `sanity_check_rbf_estimates` subtest, node 1 generates another block and syncs with the other nodes. The sync fails if the generated block is at the same height as the tip of the node.
Fix this by adding a sync to `clear_estimates`.
Fixes #30990
💬 mzumsande commented on issue "ci: Intermittent issue in feature_fee_estimation.py in sanity_check_rbf_estimates (Block sync timed out after 2400s)":
(https://github.com/bitcoin/bitcoin/issues/30990#issuecomment-2387172634)
Looks like a sync was missing, see #31016 for a fix.
(https://github.com/bitcoin/bitcoin/issues/30990#issuecomment-2387172634)
Looks like a sync was missing, see #31016 for a fix.