💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754588210)
I'm full of surprises - [done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754589089)
Thanks, rephrased!

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754589421)
[Done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754591363)
Thanks, took it and extended the `@note` a bit.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754600019)
I didn't `const` them since they're only used in the next line (i.e. very narrow scoped), seems like noise in this case - but if you insist, I'll add it

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754600910)
They are needed, see https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333713098

If you want to prohibit them, a separate pull request may better suited to remove them.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754603796)
A single count and two booleans are three symbols, whereas two counts are two symbols, which seems easier

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2343727228)
`ed7040a9da...abec00bb80`: rebase due to conflicts

🤔 l0rinc requested changes to a pull request: "sync: improve CCoinsViewCache ReallocateCache - 2nd try"
(https://github.com/bitcoin/bitcoin/pull/30370#pullrequestreview-2296162831)
concept NACK, this seems to completely contradict what `ReallocateCache` was introduced in the first place.
I left comments explaining my concerns, please let me know if I'm mistaken.

💬 l0rinc commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#discussion_r1753738717)
I would prefer not having a default value, I'd like each call to specify exactly why it needs a reallocation or not, i.e. adding each call in a separate commit, explaining why that instance of `Flush` was called with a `true`/ `false` (we can leave the default value in the first commit, add explicit parameter for the usages and remove the default value in the last commit).
E.g. I want to understand why `FlushSnapshotToDisk` calls are always reallocated (or at least see that it was thoroughly in
...

💬 l0rinc commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#discussion_r1753754940)
shouldn't we at least assert the return value, given that the doc states:
> If false is returned, the state of this cache (and its backing view) will be undefined.

💬 l0rinc commented on pull request "sync: improve CCoinsViewCache ReallocateCache - 2nd try":
(https://github.com/bitcoin/bitcoin/pull/30370#discussion_r1753749012)
doesn't this contradict the purpose of cache reallocation, i.e. https://github.com/bitcoin/bitcoin/blob/master/src/coins.h#L469-L473?

So it's either
> the map's allocator may be hanging onto a lot of memory despite having called .clear()

or

> It is likely that the map gets full again so flush is necessary, and it will happen around the same memory usage

Which begs the question: why not remove `ReallocateCache` completely?
(might be related to https://github.com/bitcoin/bitcoin/pul
...

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754612375)
Your suggestion does not compile, so I'll leave this as-is for now.

```
src/util/string.h:47:17: note: non-constexpr function 'isdigit' cannot be used in a constant expression
47 | if (std::isdigit(*it)) maybe_num = maybe_num * 10 + (*it - '0');
| ^

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754614739)
The error messages indicate two separate concerns, it would make sense logically to separate them as well

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754628343)
I know they're *used*, but I'd argue they're not *needed*, i.e. instead of
```C++
const std::string error = strprintf(
"Data directory %1$s contains a %2$s file which is ignored, because a different configuration file "
"%3$s from %4$s is being used instead. Possible ways to address this would be to:\n"
"- Delete or rename the %2$s file in data directory %1$s.\n"
"- Change datadir= or conf= options to specify one configuration file, not two, and use "
"includeconf= to
...

💬 laanwj commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2343744987)
> Why is it necessary though? Aren't we already setting umask:

Yes, but too late. The git tree is already (by definition) checked out at that point. It won't work retroactively on files that already exist.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754643534)
Must be a compiler difference, you can use `if ('0' <= *it && *it <= '9')` instead:
```C++
constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
{
unsigned count{0};
unsigned maybe_num{0};
bool inside_format_specifier = false;
bool has_normal = false, has_positional = false;
for (auto it = str.begin(); it < str.end(); ++it) {
if (*it == '%') inside_format_specifier = !inside_format_specifier;
else if (inside_format_specifier) {
...

💬 maflcko commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#discussion_r1754645983)
unrelated nit in the same section: I think ` * [Issue #12691: Enable -fsanitize flags in Travis](https://github.com/bitcoin/bitcoin/issues/12691)` can be removed. Not sure why it would be important or relevant to read today.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754654470)
If you want to prohibit them, a separate pull request may better suited to remove and prohibit them.

💬 kevkevinpal commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#discussion_r1754658100)
agreed, I remove it in this commit [12f9d33](https://github.com/bitcoin/bitcoin/pull/30870/commits/12f9d336108f78ad7a0c711085de12937aca2e73)