💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1908987982)
Same comment as above. The modulus here is `2^3072 - MAX_PRIME_DIFF`, which is represented in signed-limb representation as [1 << FINAL_LIMB_MODULUS_BITS, 0, 0, 0, ..., 0, -MAX_PRIME_DIFF], so we only need to do something for the bottom limb (where our modulus is negative) and the top limb.

💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#issuecomment-2580527041)
Rebased, addressed a few comments, and changed some `assert()` to `Assume()`.

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

💬 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