💬 stickies-v 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_r1754555353)
This change was made because of [my suggestion](https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853) but you're right there can be a case difference which I did not consider. Sorry for that @l0rinc .

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

(Side note: `count_format_specifiers` also does not implement `*` parsing correctly, which I guess was never detected because it was never used)

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

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

💬 hebasto commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#issuecomment-2343712471)
cc @maflcko

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

💬 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_r1754587684)
[Done](https://github.com/bitcoin/bitcoin/compare/6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9..1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa), thanks both for the suggestions (this is how we get to the best outcome).

💬 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');
| ^