📝 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.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1850697300)
Thanks, decided to take this. I was a bit careful here, because I lifted the check from secp, which also does not use variadic args: https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L174. But thinking a bit more about it, I could not come up with a good reason not to, so took your suggestion.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1850697300)
Thanks, decided to take this. I was a bit careful here, because I lifted the check from secp, which also does not use variadic args: https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L174. But thinking a bit more about it, I could not come up with a good reason not to, so took your suggestion.
⚠️ andremralves opened an issue: "Functional tests: `feature_bind_port_discover.py` is failing"
(https://github.com/bitcoin/bitcoin/issues/31336)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The test is failing due to `Discover()` not being able to finding the local addresses "1.1.1.1" and "2.2.2.2".
### Expected behaviour
The test should pass. Addr1 and Addr2 should be found.
### Steps to reproduce
Run the following commands:
```bash
sudo ifconfig lo:0 1.1.1.1/32 up && sudo ifconfig lo:1 2.2.2.2/32 up
./build/test/functional/test_runner.py --ihave1111and2222 feature_bi
...
(https://github.com/bitcoin/bitcoin/issues/31336)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The test is failing due to `Discover()` not being able to finding the local addresses "1.1.1.1" and "2.2.2.2".
### Expected behaviour
The test should pass. Addr1 and Addr2 should be found.
### Steps to reproduce
Run the following commands:
```bash
sudo ifconfig lo:0 1.1.1.1/32 up && sudo ifconfig lo:1 2.2.2.2/32 up
./build/test/functional/test_runner.py --ihave1111and2222 feature_bi
...
🤔 l0rinc reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2449278047)
I love that you search for these inconsistencies and attack them head on.
+1 for adding representative tests (which I didn't review in detail yet, will do in the next round of reviews).
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2449278047)
I love that you search for these inconsistencies and attack them head on.
+1 for adding representative tests (which I didn't review in detail yet, will do in the next round of reviews).
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850720954)
slightly unrelated: Checking the different options, it's not always trivial to assess what negation would mean, but
```C++
if (gArgs.IsArgSet("-color")) {
const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
if (color == "always") {
should_colorize = true;
} else if (color == "never") {
should_colorize = false;
} else if (color != "auto") {
throw std::runtime_error("Invalid value for -color option
...
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850720954)
slightly unrelated: Checking the different options, it's not always trivial to assess what negation would mean, but
```C++
if (gArgs.IsArgSet("-color")) {
const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
if (color == "always") {
should_colorize = true;
} else if (color == "never") {
should_colorize = false;
} else if (color != "auto") {
throw std::runtime_error("Invalid value for -color option
...
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850737479)
in `scripted-diff: Avoid printing version information for -noversion`:
Nit: the replacement doesn't need any escapes, if you edit again, consider simplifying:
```bash
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version')
-END VERIFY SCRIPT-
```
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850737479)
in `scripted-diff: Avoid printing version information for -noversion`:
Nit: the replacement doesn't need any escapes, if you edit again, consider simplifying:
```bash
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version')
-END VERIFY SCRIPT-
```