💬 l0rinc commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#discussion_r1823396896)
As mentioned in the comments, it seems to me that `16` may be a better default value based on the measured IBDs - basically just as fast as `128`, without having to worry about the increase in e.g. `MaxGrandParentOverlapBytes` and `ExpandedCompactionByteSizeLimit` (10x and 25x this value) called e.g. in `IsTrivialMove` with a warning: `"the move could create a parent file that will require a very expensive merge later on"` (or any other such surprise) - which we likely want to avoid:
```sugges
...

🚀 achow101 merged a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015)

💬 achow101 commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-2448430410)
ACK bbbbaa0d9ac9ae9c9b8109503aa30213eed543b9

🚀 achow101 merged a pull request: "Fix unsigned integer overflows in interpreter"
(https://github.com/bitcoin/bitcoin/pull/24214)

⚠️ Sjors opened an issue: "Gracefully handle dropped UPnP support "
(https://github.com/bitcoin-core/gui/issues/843)
https://github.com/bitcoin/bitcoin/pull/31130 dropped UPnP support. It's now recommended that
users use PCP (with NATPMP fallback).

But this is not explained in a friendly manner:

<img width="496" alt="error" src="https://github.com/user-attachments/assets/d35891ac-82e0-4f61-8e9b-671aaa1d0d75">

A user would have to figure out that they need to manually edit settings.json or delete it and redo all their settings.

We should probably automatically delete it from settings.json. And the
...

💬 Sjors commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2448504460)
Done in https://github.com/bitcoin-core/gui/issues/843

💬 Sjors commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#issuecomment-2448519062)
TIL about the `-test=<option>` hook, that's a nice solution. Post-merge concept ACK.

✅ Sjors closed an issue: "macOS 13.7 depends build can't find qt"
(https://github.com/bitcoin/bitcoin/issues/31050)

💬 Sjors commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2448527957)
That seems plausible. I'll keep an eye on that issue. Once that's resolved and there's still a macOS specific problem remaining, I'll reopen.

🤔 hodlinator reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2405166236)
> In case you do want a test with different behavior, I think you can use %n specifier which is not supported by tinyformat

Could document non-parity like so (unless you prefer I do it as part of #30933):
```C++
// Non-parity
int n{};
BOOST_CHECK_EXCEPTION(tfm::format(std::string{"%n"}, n), tfm::format_error,
HasReason{"tinyformat: %n conversion spec not supported"});
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%n");
```

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822782509)
(One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992)
Should this be removed?

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424)
Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811)
Could also add
```C++
PassFmt<1>("%.f");
```
maybe grouped with the `<1>` calls above. Confirmed tinyformat allows for it, see how the success status returned by `parseWidthOrPrecision` is ignored:
https://github.com/bitcoin/bitcoin/blob/e53829d3952c6ed275507a66e77636aad67d106b/src/tinyformat.h#L778-L788

💬 github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2448576849)
> > In my main machine, is ryzen with 32gb ddr4 with 2 sticks of memory. The other machine is Xeon, with 32gb memory, in quad channel ddr3. So i dont think its memory, because the 2 computers are stable. I can run prime95 in both for 2 hours with no problem.
>
> it's true, I just ran disk diagnostics, no issues. this is a new 4tb ssd. going to run a memory diagnostic... but my system has been stable so I would be very surprised if it was that. I'm also running on ZFS. I keep hitting this, eve
...

👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2404680812)
Code review ACK 3b5b2457f713014fd5073da34b76a5a8cd796e4a. Nice change that should make the API more safe and also makes tests more complete and easier to maintain.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822513330)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (e8e70a4ae3cbd7a648629c2df4b5a7f6f0686438)

Would be better to leave this outside of the try loop so it is clearer this is not expected to throw std::logic_error, and if it it did, the test framework would catch it.

Same for CheckWriteCoins test below.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822507538)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)

Not very important, but this commit would be easier to understand if AddFlags function were not moving at the same time as it being modified. Specifically it is harder to see the new `Assume(flags & (DIRTY | FRESH))` change and related changes because of this.

Would suggest not moving AddFlags this commit, and instead moving it in the next commit "coins: Remove direct GetFlags access" g
...

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1822525401)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)

For this commit and other commits, it would be helpful if there was an indication about whether they were supposed to change behavior or test coverage. Would suggest including "refactor" in the subject for commits which are not supposed to change behavior like "coins, refactor: Split up AddFlags to remove invalid states" or could just say behavior isn't changing in the commit description.
...

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1823231744)
In commit "coins: Split up AddFlags to remove invalid states" (8e1381a991e325395fbc10df2fb091c508efa0f3)

Not directly related to this PR, but would it make sense for SetClean to set m_prev and m_next to null here?

It seems that would be better than keeping dangling or invalid pointers. This would also make it possible to add asserts in AddFlags to confirm that the linked list state is valid when it is called.

Not suggesting any change for this commit, just wondering if this idea makes s
...