💬 sipa commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1908927814)
@maflcko @l0rinc You're both right. No problem in that case.

I think the `+ 1min` still needs to go, though.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1908961700)
Won't that make it asymmetric to the hour?

💬 Pttn commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#issuecomment-2580503897)
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424
Solves the issue, looks good to me, thank you for working on this!

💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1908979740)
Which ones in particular are missing?

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1908981550)
Hmm yeah the `+ 1min` is just noise. If we want to increase the range by 1 minute, we could just update `DATABASE_WRITE_INTERVAL_MAX = 71min`.

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1908983240)
Removed the `+ 1min`.

💬 sipa commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1908985488)
Yes and no. They aren't skipped, the carry step is the equivalent of processing the inner limbs. They're just easier, because the modulus here can be represented as [1 << FINAL_LIMB_MODULUS_BITS, 0, 0, 0, ..., 0, 0, -MAX_PRIME_DIFF]. In the `modinv32_impl.h` code, the modulus is treated as generic, where any limb can be nonzero.

💬 furszy commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#issuecomment-2580524635)
> > we are enforcing this rules within the descriptors parsing logic.
>
> That sounds dumb. Why are we?

I assume this was simply because there was no need to represent them in the descriptors' language. They've been non-standard for at least the past 10 years now.

However, there's no documentation about it. This restriction has always been present; it was introduced in the first commit that added descriptors in #13697.

We could relax the parsing restriction if a real use case arises.
...

💬 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