💬 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)

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754662640)
Absolutely, but we may not want to spend time implementing positional parsing, if we agree that we don't want to encourage their usage

💬 maflcko commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2343771189)
I think a 4-line doc fixup can be a single commit. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754674269)
Personally I think it can be useful (`std::format` also allows them), but I don't really care. I am happy to put this pull into a draft while it is waiting on the other issue/pull. However, I don't think this pull is the right place to make such a change.

🤔 ryanofsky reviewed a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2297231104)
Code review ACK adc019860d7df6848b98aec58f1c3ff47936e06b. These are nice, well-organized improvements to the settings interface to make it more consistent, remove ability to write null values to settings.json, and add ability to atomically read and delete settings.

I think not allowing null values in `settings.json` is good change to encourage clearer representations of settings, maintain parity between settings.json / bitcoin.conf / command line settings representations, avoid making ArgsMan
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754696512)
I don't want to delay the PR, just to clarify if it makes sense to work on this feature

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1754702526)
In commit "build: Improve ccache performance for different build directories" (https://github.com/bitcoin/bitcoin/commit/66b73bbf375ff96bf4a77539c9cf3fe52fbc15d5)

Now that base_dir is set by the build system, would it be fine to remove this from the prod notes?

```
$ git grep --line-number base_dir doc/productivity.md
doc/productivity.md:42:base_dir = /home/yourname # or wherever you keep your source files
doc/productivity.md:45:Note: base_dir is required for ccache to share cached co
...