💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1908989552)
Done.

💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1908989948)
Added a constant for it.

👍 ryanofsky approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2540166691)
Code review ACK 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a. Since last review avoided undefined -dbcache overflow behavior and changed the warning into an error. Also simplified and moved MiB conversion function and added test, and converted most constants to use byte sizes instead of MiB sizes (MIN_DB_CACHE still uses MiB not bytes, I think because that constant is not used by the kernel and is used by qt)

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1909014627)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908758174

> Are there places where this would be re-used? I'd be happy to add something more generic if there is demand for it.

FWIW, all of the approaches discussed in this thread seem fine to me. If I were writing this PR, I think I would drop the MiBToBytes function and just write the constants as byte counts:

```
static constexpr size_t MAX_BLOCK_DB_CACHE{2 << 20};
static constexpr size_t MAX_COINS_DB_CACHE{8 << 20};

...

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1908969637)
In commit "init: Use size_t consistently for cache sizes" (82706e217f34d1f09cbd30dde6b8ae5ac0385f0a)

Code could be simplified to make it clear what is causing the new error:

<details><summary>diff</summary>
<p>

```diff
--- a/src/node/caches.cpp
+++ b/src/node/caches.cpp
@@ -25,14 +25,10 @@ static constexpr size_t MAX_FILTER_INDEX_CACHE{MiBToBytes(1024)};
namespace node {
std::optional<CacheSizes> CalculateCacheSizes(const ArgsManager& args, size_t n_indexes)
{
- int64_t db
...

📝 instagibbs opened a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628)
Not entirely persuaded this needs coverage, but was behavior I hadn't considered before.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1909039107)
You're right, I suggested the `+ 1min` back when I also thought that the range is in minutes only.

👍 l0rinc approved a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2540272419)
ACK b7f3b6fd96ca2d5d1f9538affef682f778bd07e8

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1909042923)
It's not a max now (i.e. not a closed range), but a threshold, but the difference may not be important anymore

💬 ryanofsky commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909056496)
re: https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1908760710

> I made it into a `unsigned int` (I don't particularly like it, but it's not that important).

Thanks, your change makes sense and to be clear I don't like code writing `unsigned int` everywhere either, even though I do think `unsigned int` is a reasonable type for this code to be using. I think more ideally src/flatfile.h would define something like:

```c++
using FilePos = unsigned int;
```

and then code cou
...

👍 ryanofsky approved a pull request: "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2540290494)
Code review ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa. Since last review, "Save" was renamed to "Write", uint32_t references were dropped, some log statements and comments were improved as suggested, and a lot of tweaks made to commits and commit messages which should make this easier to review.

🤔 stickies-v reviewed a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2540319978)
Approach ACK fa6d9a29162fb508385201c926ec745d071086fc. Apologies for the slow re-review, but I like the latest form of this PR much more than the previous one I looked at, nice! I'll complete my full re-review in the next couple of days.

💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1909076664)
nit: `this supports std::string for runtime string` - perhaps worth updating to reflect that it now requires to be wrapped in `RuntimeFormat`?

💬 ryanofsky commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909082218)
In commit "bench: add SaveBlockBench" (86b85bb11f8999eb59e34bd026b0791dc866f2eb)

Could rename SaveBlock to WriteBlock here too

💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1909088406)
I was hoping that "runtime string formatting" is clear enough to show the connection to `RuntimeFormat` and then looking at the `RuntimeFormat` constructor, one can see `std::string`. So I didn't change it, but I am happy to take suggestions.

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1909093808)
Right, if I need to edit, I'll rename this as well

💬 luke-jr commented on pull request "RPC: Fix invalid txid in `gettransaction` example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-2580718388)
That's why I was suggesting pulling a txid from the current mempool

💬 maflcko commented on pull request "refactor: modernize recent `ByteType` usages and read/write functions":
(https://github.com/bitcoin/bitcoin/pull/31601#issuecomment-2580767132)
> > And doing that all over would be a recipe for never-ending churn
>
> Isn't that what we're doing regularly though? Endlessly making the legacy codebase a tiny bit more readable, a tiny bit more modern, a tiny bit more maintainable and consistent?

`uint8_t` and `unsigned char` are the exact same type. If not, the compilation would fail in other places. I don't think it is helpful to create 2432 pull requests to change each instance one-by-one in a separate pull request. If this change i
...

💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2580783610)
> As I said in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450539742 this seems like an optimization that can wait.

Right, and in the next comment I replied with 4 reasons why its relevant, only two of which are specifically performance-related :). Sure, things can be built when they're built, but just adding a way to wait for the next block does not meaningfully address the reasons why we want this.

👍 ryanofsky approved a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2540386980)
Code review ACK 384c73d4fcda4af7c2b4bfb945a248cb93dba47d but it seems like a fuzz check is failing in CI. Since last review, BlockManagerOpts uses DBParams, so no longer needs to duplicate most of its fields, and blockstorage m_opts no longer need to be public so some other changes could be reverted. One other set of changes that were reverted had do to with `m_block_tree_db_in_memory` / `opts.block_tree_db_in_memory;` which seems more correct now.