🚀 glozow merged a pull request: "cluster mempool: Implement changeset interface for mempool"
(https://github.com/bitcoin/bitcoin/pull/31122)
(https://github.com/bitcoin/bitcoin/pull/31122)
💬 dergoegge commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2488561557)
Perhaps this makes more sense in combination with a test that uses it? Currently `FuzzedSock::Accept` seems entirely unused: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/util/net.cpp.gcov.html
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-2488561557)
Perhaps this makes more sense in combination with a test that uses it? Currently `FuzzedSock::Accept` seems entirely unused: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/test/fuzz/util/net.cpp.gcov.html
💬 kevkevinpal commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#discussion_r1850347673)
maybe we can update the error message to be
```
ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(m_path_lock) + '\n' + "The directory and .lock file already exist, please ensure the directory .lock file does not exist.");
```
it is fine if the directory exists, we are just don't want `.lock` file to be present?
I can see if I can come up with better wording than what I suggested above
(https://github.com/bitcoin/bitcoin/pull/31328#discussion_r1850347673)
maybe we can update the error message to be
```
ExitFailure("Cannot obtain a lock on test data lock directory " + fs::PathToString(m_path_lock) + '\n' + "The directory and .lock file already exist, please ensure the directory .lock file does not exist.");
```
it is fine if the directory exists, we are just don't want `.lock` file to be present?
I can see if I can come up with better wording than what I suggested above
💬 kevkevinpal commented on pull request "test: locking -testdatadir when not specified and then deleting lock and dir at end of test":
(https://github.com/bitcoin/bitcoin/pull/31328#discussion_r1850374636)
updated in fe6da3744420bcbac9002ae324dfa04902aed490
(https://github.com/bitcoin/bitcoin/pull/31328#discussion_r1850374636)
updated in fe6da3744420bcbac9002ae324dfa04902aed490
💬 ryanofsky commented on pull request "Add destroy to BlockTemplate schema":
(https://github.com/bitcoin/bitcoin/pull/31288#issuecomment-2488688261)
Will go ahead and merge even though it only has 2 acks, since it's basically just adding a line to a schema file. Having this merged should make the related PR easier to maintain and review.
(https://github.com/bitcoin/bitcoin/pull/31288#issuecomment-2488688261)
Will go ahead and merge even though it only has 2 acks, since it's basically just adding a line to a schema file. Having this merged should make the related PR easier to maintain and review.
📝 MarnixCroes opened a pull request: "contrib: fix BUILDDIR in gen-bitcoin-conf script"
(https://github.com/bitcoin/bitcoin/pull/31332)
On master the gen-bitcoin-conf script doesn't work as it cannot find bitcoind.
This is because of the build dir that is now being used since cmake.
This PR fixes it.
**master**
```
./gen-bitcoin-conf.sh
/home/marnix/projects/bitcoin/src/bitcoind not found or not executable.
```
**PR**
```
./gen-bitcoin-conf.sh
Generating example bitcoin.conf file in share/examples/
```
(https://github.com/bitcoin/bitcoin/pull/31332)
On master the gen-bitcoin-conf script doesn't work as it cannot find bitcoind.
This is because of the build dir that is now being used since cmake.
This PR fixes it.
**master**
```
./gen-bitcoin-conf.sh
/home/marnix/projects/bitcoin/src/bitcoind not found or not executable.
```
**PR**
```
./gen-bitcoin-conf.sh
Generating example bitcoin.conf file in share/examples/
```
📝 hodlinator opened a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333)
Catching up to bench & unit tests. Makes for more orderly paths for fuzz tests using `BasicTestingSetup`.
### Before
```
/tmp/test_common bitcoin/0748ae43ef8fa80703bc/regtest/blocks/xor.dat
```
### After
```
/tmp/test_common bitcoin/tx_pool_standard/f18b3744625e0600eb0c/regtest/blocks/xor.dat
```
(https://github.com/bitcoin/bitcoin/pull/31333)
Catching up to bench & unit tests. Makes for more orderly paths for fuzz tests using `BasicTestingSetup`.
### Before
```
/tmp/test_common bitcoin/0748ae43ef8fa80703bc/regtest/blocks/xor.dat
```
### After
```
/tmp/test_common bitcoin/tx_pool_standard/f18b3744625e0600eb0c/regtest/blocks/xor.dat
```
💬 hodlinator commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2488709503)
New PR #31333.
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2488709503)
New PR #31333.
⚠️ fanquake opened an issue: "ci: brew link workaround step no-longer working"
(https://github.com/bitcoin/bitcoin/issues/31334)
CI failure in #31333. https://github.com/bitcoin/bitcoin/actions/runs/11935219880/job/33266067166?pr=31333:
```bash
Run # A workaround for "The `brew link` step did not complete successfully" error.
<...>
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /opt/homebrew
Could not symlink bin/pkg-config
Target /opt/homebrew/bin/pkg-config
```
(https://github.com/bitcoin/bitcoin/issues/31334)
CI failure in #31333. https://github.com/bitcoin/bitcoin/actions/runs/11935219880/job/33266067166?pr=31333:
```bash
Run # A workaround for "The `brew link` step did not complete successfully" error.
<...>
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /opt/homebrew
Could not symlink bin/pkg-config
Target /opt/homebrew/bin/pkg-config
```
💬 kevkevinpal commented on pull request "fuzz: Implement G_TEST_GET_FULL_NAME":
(https://github.com/bitcoin/bitcoin/pull/31333#issuecomment-2488754481)
ACK [92d3d69](https://github.com/bitcoin/bitcoin/pull/31333/commits/92d3d691f097ead8e5ae571eb9bf691133a6aa49)
This helps clean up the `/tmp/test_common bitcoin` dir and makes it easier to know which directory is running which test
I tested by running an individual fuzz test with multiple workers and observing the `/tmp` dir
`FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=8 -workers=8`
I also tested by running the fuzz `test_runner.py` and observing the `/tmp` dir
` ./bui
...
(https://github.com/bitcoin/bitcoin/pull/31333#issuecomment-2488754481)
ACK [92d3d69](https://github.com/bitcoin/bitcoin/pull/31333/commits/92d3d691f097ead8e5ae571eb9bf691133a6aa49)
This helps clean up the `/tmp/test_common bitcoin` dir and makes it easier to know which directory is running which test
I tested by running an individual fuzz test with multiple workers and observing the `/tmp` dir
`FUZZ=utxo_total_supply ./build_fuzz/src/test/fuzz/fuzz -jobs=8 -workers=8`
I also tested by running the fuzz `test_runner.py` and observing the `/tmp` dir
` ./bui
...
📝 fanquake opened a pull request: "macOS: swap docs & CI from pkg-config to pkgconf"
(https://github.com/bitcoin/bitcoin/pull/31335)
Migrate the macOS build docs and CI from `pkg-config` to `pkgconf`. As the former now just redirects to the later.
Upstream is currently mass-migrating its formula. i.e https://github.com/Homebrew/homebrew-core/pull/198317.
If #31334 persists we can add a similar workaround as we do for Python.
(https://github.com/bitcoin/bitcoin/pull/31335)
Migrate the macOS build docs and CI from `pkg-config` to `pkgconf`. As the former now just redirects to the later.
Upstream is currently mass-migrating its formula. i.e https://github.com/Homebrew/homebrew-core/pull/198317.
If #31334 persists we can add a similar workaround as we do for Python.
💬 hebasto commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2488931506)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2488931506)
Concept ACK.
🚀 ryanofsky merged a pull request: "Add destroy to BlockTemplate schema"
(https://github.com/bitcoin/bitcoin/pull/31288)
(https://github.com/bitcoin/bitcoin/pull/31288)
💬 fanquake commented on pull request "macOS: swap docs & CI from pkg-config to pkgconf":
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2488944785)
Given that the CI is now working here, (and #31334 wasn't one-off, it's happening for all new pushes), have updated the commit message and PR description.
(https://github.com/bitcoin/bitcoin/pull/31335#issuecomment-2488944785)
Given that the CI is now working here, (and #31334 wasn't one-off, it's happening for all new pushes), have updated the commit message and PR description.
💬 andremralves commented on issue "Discover() will not run if listening on any address with an explicit bind=0.0.0.0":
(https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2488958085)
Is `Discover()` working properly? I tried running `test_runner.py --ihave1111and2222 feature_bind_port_discover.py` and the test didn't find the `1.1.1.1` and `2.2.2.2` addresses. In the commit where `feature-bind_port_discover.py` was added, the test works as expected and passes. Actually, I think, now, only ipv6 addresses are being discovered.
(https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2488958085)
Is `Discover()` working properly? I tried running `test_runner.py --ihave1111and2222 feature_bind_port_discover.py` and the test didn't find the `1.1.1.1` and `2.2.2.2` addresses. In the commit where `feature-bind_port_discover.py` was added, the test works as expected and passes. Actually, I think, now, only ipv6 addresses are being discovered.
🤔 l0rinc reviewed a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449193292)
Concept ACK, please see my simplification nits.
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449193292)
Concept ACK, please see my simplification nits.
💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750)
Since C++17 we can [safely compare](https://en.cppreference.com/w/cpp/utility/optional/operator_cmp) an `optional<T>` with another `T`:
```C++
template< class T, class U >
constexpr bool operator!=(const optional<T>& opt, const U& value);
(23) (since C++17)
```
If you modify again, consider simplifying to:
```suggestion
return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt;
```
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750)
Since C++17 we can [safely compare](https://en.cppreference.com/w/cpp/utility/optional/operator_cmp) an `optional<T>` with another `T`:
```C++
template< class T, class U >
constexpr bool operator!=(const optional<T>& opt, const U& value);
(23) (since C++17)
```
If you modify again, consider simplifying to:
```suggestion
return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt;
```
💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850685295)
Does the `BOOST_REQUIRE` serve here as a better error message than what `m_tip_block.value()` would already throw (i.e. `bad_optional_access()`)?
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850685295)
Does the `BOOST_REQUIRE` serve here as a better error message than what `m_tip_block.value()` would already throw (i.e. `bad_optional_access()`)?
💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850668343)
I've noticed that we're checking whether the optional has a value in different ways here and in `interfaces.cpp`.
If you change the PR again, please consider unifying:
```suggestion
return kernel_notifications.m_tip_block || ShutdownRequested(node);
```
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850668343)
I've noticed that we're checking whether the optional has a value in different ways here and in `interfaces.cpp`.
If you change the PR again, please consider unifying:
```suggestion
return kernel_notifications.m_tip_block || ShutdownRequested(node);
```
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2489128571)
Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 ([kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1) -> [kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_1..kernelApi_2))
* Added @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757015877), implementing variadic args for nonnull attribute macro.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2489128571)
Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 ([kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1) -> [kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_1..kernelApi_2))
* Added @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757015877), implementing variadic args for nonnull attribute macro.