📝 dergoegge opened a pull request: "ci: Split out native fuzz jobs for macOS and windows"
(https://github.com/bitcoin/bitcoin/pull/31073)
Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.
In both jobs the `fuzz` binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.
(https://github.com/bitcoin/bitcoin/pull/31073)
Split out two new CI jobs (for native macOS and windows) that run the fuzz tests on the qa-assets input corpora.
In both jobs the `fuzz` binary is built with `-DBUILD_FOR_FUZZING` to enable `Assume` assertions as well as `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`.
👋 dergoegge's pull request is ready for review: "ci: Split out native fuzz jobs for macOS and windows"
(https://github.com/bitcoin/bitcoin/pull/31073)
(https://github.com/bitcoin/bitcoin/pull/31073)
💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407691628)
Just for brainstorming, also made a modified branch that reduces parallel compact block downloads to a count of 2, which would allow one or more backoff blocks at tip at a less aggressive pace: https://github.com/instagibbs/bitcoin/commits/mzumsande_202403_near_tip_stalling_2/
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407691628)
Just for brainstorming, also made a modified branch that reduces parallel compact block downloads to a count of 2, which would allow one or more backoff blocks at tip at a less aggressive pace: https://github.com/instagibbs/bitcoin/commits/mzumsande_202403_near_tip_stalling_2/
💬 theuni commented on pull request "build: Bump minimum supported macOS to 12.0":
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2407700684)
> > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
>
> If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
Yes, good point. I didn't realize 12.0 was eol'd. We shouldn't let qt dictate what we support.
Agree that 13 would make more sense
...
(https://github.com/bitcoin/bitcoin/pull/31048#issuecomment-2407700684)
> > Running Bitcoin Core on unsupported OSes may expose users to security issues.
> > macOS Big Sur 11 received its last security update ([11.7.10](https://support.apple.com/en-us/106338)) over a year ago.
>
> If this is the reasoning we are going to use, shouldn't this be bumping to `10.13`, given `10.12` is also "unsupported" in terms of security updates?
Yes, good point. I didn't realize 12.0 was eol'd. We shouldn't let qt dictate what we support.
Agree that 13 would make more sense
...
💬 LarryRuane commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2407842364)
Force pushed Jon's suggestion, thanks - https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1795456841
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2407842364)
Force pushed Jon's suggestion, thanks - https://github.com/bitcoin/bitcoin/pull/30859#discussion_r1795456841
💬 jonatack commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2407851530)
ACK e64b2f1a16e8d0ad2cd181d84e3b70312e3cdf33
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2407851530)
ACK e64b2f1a16e8d0ad2cd181d84e3b70312e3cdf33
💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1797273429)
I think calling it a "dumb thing" that people want to do is a little bit of a simplification. Nearly every indexing API [offers this information](https://electrum.readthedocs.io/en/latest/protocol.html#blockchain-address-subscribe), and commercial end users make use of the address for various things. You can imagine a wallet that is programmed not to reuse addresses but still wants to display the address when a spend is detected. If we can easily generate this information (sPK -> address) and at
...
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1797273429)
I think calling it a "dumb thing" that people want to do is a little bit of a simplification. Nearly every indexing API [offers this information](https://electrum.readthedocs.io/en/latest/protocol.html#blockchain-address-subscribe), and commercial end users make use of the address for various things. You can imagine a wallet that is programmed not to reuse addresses but still wants to display the address when a spend is detected. If we can easily generate this information (sPK -> address) and at
...
💬 instagibbs commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2407899208)
concept ACK on a functional test for this logic, note that at the unit test level there is already a bit of coverage: https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp#L106
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2407899208)
concept ACK on a functional test for this logic, note that at the unit test level there is already a bit of coverage: https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp#L106
🤔 hebasto reviewed a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2363335124)
Post-merge ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6.
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2363335124)
Post-merge ACK 882f736d0a607976ee5e3a6cbcb5385524bc72c6.
💬 mzumsande commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407928676)
[973cd01 ](https://github.com/bitcoin/bitcoin/commit/973cd0174ae44ff9cbc3b41912323f71f8eafca4)to [5e1ff82](https://github.com/bitcoin/bitcoin/commit/5e1ff82251283c07274c62e1b65d5aff9be80212):
- added more detailed coverage by @instagibbs (thanks!)
- provided missing block and check that sync finishes in the end in `near_tip_stalling` subtest
- remove unused `total_bytes_recv_for_blocks` in first commit
> Just for brainstorming, also made a modified branch that reduces parallel compact blo
...
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407928676)
[973cd01 ](https://github.com/bitcoin/bitcoin/commit/973cd0174ae44ff9cbc3b41912323f71f8eafca4)to [5e1ff82](https://github.com/bitcoin/bitcoin/commit/5e1ff82251283c07274c62e1b65d5aff9be80212):
- added more detailed coverage by @instagibbs (thanks!)
- provided missing block and check that sync finishes in the end in `near_tip_stalling` subtest
- remove unused `total_bytes_recv_for_blocks` in first commit
> Just for brainstorming, also made a modified branch that reduces parallel compact blo
...
💬 mzumsande commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#discussion_r1797299338)
added, by connecting a peer that provides the missing block in the end, and checking that the sync finishes.
(https://github.com/bitcoin/bitcoin/pull/29664#discussion_r1797299338)
added, by connecting a peer that provides the missing block in the end, and checking that the sync finishes.
💬 satsie commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1797311701)
The thinking behind this is because it is a 4D array, all the nesting required to create it can be a headache to look at. Lines 725 - 727 are the array sizes, the second arguments needed for `std::array` creation. Each one of these lines up with the `std::array` they are part of.
For example, `NUM_DIRECTIONS` finishes the declaration off the parent level `std::array` that all the others are nested in. For that reason, `NUM_DIRECTIONS` lines up with the space after that array's opening `<`.
...
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r1797311701)
The thinking behind this is because it is a 4D array, all the nesting required to create it can be a headache to look at. Lines 725 - 727 are the array sizes, the second arguments needed for `std::array` creation. Each one of these lines up with the `std::array` they are part of.
For example, `NUM_DIRECTIONS` finishes the declaration off the parent level `std::array` that all the others are nested in. For that reason, `NUM_DIRECTIONS` lines up with the space after that array's opening `<`.
...
💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407953923)
> Is it a problem that on slow connections or new connections with unfilled mempools the parallel compact blocks download (which is triggered immediately instead of having a timeout) would hit more often than necessary?
I think that's already the case, a saving grace is that since we only trigger parallel cb with high-bandwidth cb peers it won't happen until you've seen a few blocks at tip at least.
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2407953923)
> Is it a problem that on slow connections or new connections with unfilled mempools the parallel compact blocks download (which is triggered immediately instead of having a timeout) would hit more often than necessary?
I think that's already the case, a saving grace is that since we only trigger parallel cb with high-bandwidth cb peers it won't happen until you've seen a few blocks at tip at least.
👍 hebasto approved a pull request: "guix: use GCC 13 to build releases"
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2363408909)
ACK f9e0010de394ad6ae5eb6a22ead7ebc116a16fd4.
Guix build (on both `x86_64` and `aarch64`):
```
d6a2d55943232d35bc56c36341b8501af7637c1f337a1e49f2ac34baced7d884 guix-build-f9e0010de394/output/aarch64-linux-gnu/SHA256SUMS.part
3bbb9714ea3196a2fca7f2d8db8a309bc298b43ace4397f11ff8acc8b6d829d3 guix-build-f9e0010de394/output/aarch64-linux-gnu/bitcoin-f9e0010de394-aarch64-linux-gnu-debug.tar.gz
6b5402883ebef604c6c1da21d81d9414e731be41bf294a3a92f5fb5ff5be300b guix-build-f9e0010de394/output/aar
...
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2363408909)
ACK f9e0010de394ad6ae5eb6a22ead7ebc116a16fd4.
Guix build (on both `x86_64` and `aarch64`):
```
d6a2d55943232d35bc56c36341b8501af7637c1f337a1e49f2ac34baced7d884 guix-build-f9e0010de394/output/aarch64-linux-gnu/SHA256SUMS.part
3bbb9714ea3196a2fca7f2d8db8a309bc298b43ace4397f11ff8acc8b6d829d3 guix-build-f9e0010de394/output/aarch64-linux-gnu/bitcoin-f9e0010de394-aarch64-linux-gnu-debug.tar.gz
6b5402883ebef604c6c1da21d81d9414e731be41bf294a3a92f5fb5ff5be300b guix-build-f9e0010de394/output/aar
...
💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2407976069)
Apparently, I haven't had time to address feedback and rebase this PR recently. Perhaps it could be labelled "Up for grabs"?
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2407976069)
Apparently, I haven't had time to address feedback and rebase this PR recently. Perhaps it could be labelled "Up for grabs"?
📝 hebasto converted_to_draft a pull request: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).
The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.
However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).
The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.
However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...
📝 hebasto converted_to_draft a pull request: "Release `LockData::dd_mutex` before calling `*_detected` functions"
(https://github.com/bitcoin/bitcoin/pull/26781)
Both `double_lock_detected()` and `potential_deadlock_detected()` functions call `LogPrintf()` which in turn implies locking of the `Logger::m_cs` mutex. To avoid a deadlock, the latter must not have the `Mutex` type (see https://github.com/bitcoin/bitcoin/pull/16112).
With this PR the mentioned restriction has been lifted, and it is possible now to use our regular `Mutex` type for the `Logger::m_cs` mutex instead of a dedicated `StdMutex` type (not introducing that change here, as its diff i
...
(https://github.com/bitcoin/bitcoin/pull/26781)
Both `double_lock_detected()` and `potential_deadlock_detected()` functions call `LogPrintf()` which in turn implies locking of the `Logger::m_cs` mutex. To avoid a deadlock, the latter must not have the `Mutex` type (see https://github.com/bitcoin/bitcoin/pull/16112).
With this PR the mentioned restriction has been lifted, and it is possible now to use our regular `Mutex` type for the `Logger::m_cs` mutex instead of a dedicated `StdMutex` type (not introducing that change here, as its diff i
...
📝 hebasto converted_to_draft a pull request: "test, subprocess: Improve coverage report correctness"
(https://github.com/bitcoin/bitcoin/pull/30075)
As a child process uses the [`execvp()`](https://linux.die.net/man/3/execvp) call, an explicit dumping of the collected profile information is required.
Coverage:
- on the master branch:
- with this PR:
This PR mostly follows [gcc/libgcc/libgcov-interface.c#L320-L332](https://github.com/gc
...
(https://github.com/bitcoin/bitcoin/pull/30075)
As a child process uses the [`execvp()`](https://linux.die.net/man/3/execvp) call, an explicit dumping of the collected profile information is required.
Coverage:
- on the master branch:
- with this PR:
This PR mostly follows [gcc/libgcc/libgcov-interface.c#L320-L332](https://github.com/gc
...
🤔 LarryRuane requested changes to a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2363290423)
I think you also should update line 93 (change "4" to "5")
```
argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
```
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2363290423)
I think you also should update line 93 (change "4" to "5")
```
argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
```
💬 LarryRuane commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1797252340)
683b558a020f1632b0a3cbdaa165adbd1423281c
```suggestion
str += (s == "NETWORK_LIMITED") ? 'l' : ((s == "P2P_V2") ? '2' : ToLower(s[0]));
```
(readability, nonblocking nit, if you happen to retouch)
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1797252340)
683b558a020f1632b0a3cbdaa165adbd1423281c
```suggestion
str += (s == "NETWORK_LIMITED") ? 'l' : ((s == "P2P_V2") ? '2' : ToLower(s[0]));
```
(readability, nonblocking nit, if you happen to retouch)