💬 fanquake commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2468288385)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2468288385)
Concept ACK.
💬 l0rinc commented on pull request "doc: correct typos":
(https://github.com/bitcoin/bitcoin/pull/31271#issuecomment-2468293030)
ACK 726cbee9553b25bedfef70cfd5be9f1eeec8a30d
(https://github.com/bitcoin/bitcoin/pull/31271#issuecomment-2468293030)
ACK 726cbee9553b25bedfef70cfd5be9f1eeec8a30d
🚀 fanquake merged a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267)
(https://github.com/bitcoin/bitcoin/pull/31267)
💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836745793)
Done.
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836745793)
Done.
💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2468299823)
Force-pushed:
- Adjusted the cap and added the `Assume` for the `max_pct`.
- Adjusted `GetAddr` and `GetAddress` documentation.
- Adjusted both connman and addrman fuzz targets to use values from 0 to 100 for `max_pct`.
Thanks @mzumsande and @vasild.
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2468299823)
Force-pushed:
- Adjusted the cap and added the `Assume` for the `max_pct`.
- Adjusted `GetAddr` and `GetAddress` documentation.
- Adjusted both connman and addrman fuzz targets to use values from 0 to 100 for `max_pct`.
Thanks @mzumsande and @vasild.
💬 l0rinc commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2468308284)
The title still claims "fuzz", but the fix seems to be in production code - can we update the PR title?
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2468308284)
The title still claims "fuzz", but the fix seems to be in production code - can we update the PR title?
⚠️ maflcko opened an issue: "RFC: Adopt C++ Safe Buffers?"
(https://github.com/bitcoin/bitcoin/issues/31272)
C++ is unsafe, meaning that any code written in it may cause undefined behavior at runtime. One possible source of undefined behavior is out-of-range memory access.
While some limited compiler warnings exist to detect some obvious cases, tracking down out-of-range memory access is usually done at runtime with debugging tools such as Valgrind or Asan. However, such tools can normally not be used in production, because they are not hardening tools, see https://stackoverflow.com/a/70004411/20847
...
(https://github.com/bitcoin/bitcoin/issues/31272)
C++ is unsafe, meaning that any code written in it may cause undefined behavior at runtime. One possible source of undefined behavior is out-of-range memory access.
While some limited compiler warnings exist to detect some obvious cases, tracking down out-of-range memory access is usually done at runtime with debugging tools such as Valgrind or Asan. However, such tools can normally not be used in production, because they are not hardening tools, see https://stackoverflow.com/a/70004411/20847
...
🤔 maflcko reviewed a pull request: "fuzz: fix bad alloc in connman target"
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2427444404)
you'll have to squash the commits. Otherwise a fuzz bisect will fail on the addrman target as well. (connman already fails)
(https://github.com/bitcoin/bitcoin/pull/31235#pullrequestreview-2427444404)
you'll have to squash the commits. Otherwise a fuzz bisect will fail on the addrman target as well. (connman already fails)
💬 maflcko commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836765925)
style-nit: For new code it would be good to use `size_t{100}`, as explained in the dev notes.
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836765925)
style-nit: For new code it would be good to use `size_t{100}`, as explained in the dev notes.
💬 purpleKarrot commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2468345819)
There are four alternative implementations for `sha256`, each in its own source file which has to be compiled with dedicated compiler options. The current approach is to build a static library per source file, set the necessary compiler options for that library, and eventually link `bitcoin_crypto` against those libraries. This is needless complexity.
My proposed approach is to compile the source file with its dedicated options, but link the object files into `bitcoin_crypto` directly.
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2468345819)
There are four alternative implementations for `sha256`, each in its own source file which has to be compiled with dedicated compiler options. The current approach is to build a static library per source file, set the necessary compiler options for that library, and eventually link `bitcoin_crypto` against those libraries. This is needless complexity.
My proposed approach is to compile the source file with its dedicated options, but link the object files into `bitcoin_crypto` directly.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836790850)
@maflcko I think there is a conceptual simplicity in having all failure paths just needing to set the `state` object correctly, and be logging done in a uniform way based on just that. While it's true that there are a few control flow changes compared to the older code, those are really only needed to make the debug output in the presence of multiple consensus failure consistent; the code would be correct without them.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836790850)
@maflcko I think there is a conceptual simplicity in having all failure paths just needing to set the `state` object correctly, and be logging done in a uniform way based on just that. While it's true that there are a few control flow changes compared to the older code, those are really only needed to make the debug output in the presence of multiple consensus failure consistent; the code would be correct without them.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836793220)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836793220)
Fixed.
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2468354278)
Fixed the test-each-commit failure and addressed comment.
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2468354278)
Fixed the test-each-commit failure and addressed comment.
💬 fanquake commented on pull request "cmake: Revamp `FindLibevent` module":
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2468376483)
Guix Build
```bash
b526b46943f39780cb2a204cd64159f7dd4f502e482256ba78371c82d717a591 guix-build-5a96767e3f53/output/aarch64-linux-gnu/SHA256SUMS.part
77714a2034b96c572598342da2147a638abd626b29e4ccecfe786f53dbf72a4e guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu-debug.tar.gz
a32ce8d85863ec00f75b37ab7b10bfc37e85efd3a1016ed66ce6cbd0024d018a guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu.tar.gz
751eba9ba4c9c2432
...
(https://github.com/bitcoin/bitcoin/pull/31181#issuecomment-2468376483)
Guix Build
```bash
b526b46943f39780cb2a204cd64159f7dd4f502e482256ba78371c82d717a591 guix-build-5a96767e3f53/output/aarch64-linux-gnu/SHA256SUMS.part
77714a2034b96c572598342da2147a638abd626b29e4ccecfe786f53dbf72a4e guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu-debug.tar.gz
a32ce8d85863ec00f75b37ab7b10bfc37e85efd3a1016ed66ce6cbd0024d018a guix-build-5a96767e3f53/output/aarch64-linux-gnu/bitcoin-5a96767e3f53-aarch64-linux-gnu.tar.gz
751eba9ba4c9c2432
...
💬 sipa commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836809045)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1836809045)
Fixed.
👍 fanquake approved a pull request: "cmake: Revamp `FindLibevent` module"
(https://github.com/bitcoin/bitcoin/pull/31181#pullrequestreview-2427529805)
ACK 5a96767e3f531ba9e8a676eec47727421f9f589f
(https://github.com/bitcoin/bitcoin/pull/31181#pullrequestreview-2427529805)
ACK 5a96767e3f531ba9e8a676eec47727421f9f589f
💬 hodlinator commented on pull request "util: Narrow scope of args (-color, -version, -conf, -datadir)":
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468421728)
> > Disallow -noconf since a config file is required
>
> But it's not. Seems like `-noconf` should work to ignore the config file.
As mentioned in the commit message for 2d65e6ae0756b87e82dbafd032308e8687ec7941:
> ArgsManager::GetConfigFilePath() asserts that a path is always set. Explicitly negating it is therefore invalid."
>
> Previously, -noconf would result in an ifstream being opened to the ".bitcoin"-directory (not a file) in ArgsManager::ReadConfigFiles(), also circumventing t
...
(https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2468421728)
> > Disallow -noconf since a config file is required
>
> But it's not. Seems like `-noconf` should work to ignore the config file.
As mentioned in the commit message for 2d65e6ae0756b87e82dbafd032308e8687ec7941:
> ArgsManager::GetConfigFilePath() asserts that a path is always set. Explicitly negating it is therefore invalid."
>
> Previously, -noconf would result in an ifstream being opened to the ".bitcoin"-directory (not a file) in ArgsManager::ReadConfigFiles(), also circumventing t
...
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1836842757)
> I don't think it is right to use mockable time here. Also, GetTime is deprecated, so what about TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())?
Yeah sure. This is usually called before mocking the time, but agree to change it.
> Also, I think it would be better to switch test_name / rand_str to rand_str / test_name, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.
That won’t work as you expect. T
...
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1836842757)
> I don't think it is right to use mockable time here. Also, GetTime is deprecated, so what about TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())?
Yeah sure. This is usually called before mocking the time, but agree to change it.
> Also, I think it would be better to switch test_name / rand_str to rand_str / test_name, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.
That won’t work as you expect. T
...
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2468440155)
Rebased for renaming `TRACE5` -> `TRACEPOINT`.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2468440155)
Rebased for renaming `TRACE5` -> `TRACEPOINT`.
💬 sipa commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2468443487)
Concept ACK. If it's possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2468443487)
Concept ACK. If it's possible to have differently-compiled compilation units without needing separate libraries for each, then this does seem like a cleaner solution. I suspect we perhaps ended up in this state because in autotools separate libraries were the only way to achieve this?