Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 hebasto approved a pull request: "test: remove Boost SIGCHLD workaround."
(https://github.com/bitcoin/bitcoin/pull/32403#pullrequestreview-2812134260)
ACK 3add6ab9adcd722d57c6d488581358ae9b377f1a, I have reviewed the code and it looks OK.
📝 l0rinc opened a pull request: "fuzz: assert FastRandomContext range boundaries"
(https://github.com/bitcoin/bitcoin/pull/32404)
This PR addresses a few leftover nits found while reviewing https://github.com/bitcoin/bitcoin/pull/30611.
This was also needed to validate its behavior properly, because currently there's no way to visualize how often (and why) we're flushing.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2847346556)
`c52b673bf8...c9cd7d3aa9`: rebase and address suggestions
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071700058)
There is even a bigger problem with `if (!file.Commit() || file.fclose() != 0) {` -- if the commit fails when `fclose()` will not be called, it will return and the file destructor will be upset that the file has been written to and is not closed. Fixed.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071702348)
Changed to `LogError()`, but `__func__` is redundant with the config option `-logsourcelocations`, so I omitted it.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071702885)
Right. Removed the scope.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071703580)
Moved the comment before the actual closing. Reduced as well.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071703786)
Done.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071706458)
Removed this outdated comment.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2847360895)
`c9cd7d3aa9...66087035cf`: remove outdated comment
👍 hebasto approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2812173999)
ACK ace66e125c01448a4bde0889b044544937bad6ea.
💬 hebasto commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r2071707186)
`gmake` is used in `doc/build-freebsd.md`.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071707907)
I think it is better to stop the test right away if the closing fails.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071708123)
Done.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071708360)
Done.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071715279)
Hmm, the OS `feof()` returns nonzero if end-of-file. This makes it possible to use the natural:

```cpp
if (feof()) { // end-of-file has been reached
```

But `fclose()` returns zero if the file has been closed. I do not want to invert the return value because that would be confusing for people that know the interface of the OS `fclose()`. Or even more confusing, if the return is not inverted and converted to `bool`:

```
bool fclose(...);

if (fclose()) { // error!
```
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071716526)
Elaborated a little bit:

```cpp
// Callers that wrote to the file must have closed it explicitly
// with the fclose() method and checked that the close succeeded.
// This is because here from the destructor we have no way to signal
// error due to close which, after write, could mean the file is
// corrupted and must be handled properly at the call site.
// Destructors in C++ cannot signal an error to the callers be
...
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071717206)
I think it is better to stop the test immediately if closing a file gives an error.
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071717840)
What to be added to `AutoFile::write_buffer`?
💬 hebasto commented on issue "build: x86 afl++ ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2847384462)
My guess is that `/AFLplusplus/afl-clang-fast++` fails to handle the `-fcf-protection=full -fcf-protection=none` flags properly.