💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2126699985)
> 2) to modify all call-sites to pass in a rate-limiting argument is cumbersome

Absolutely agree that would not be an improvement.

> I think using a boolean for rate-limiting vs no-rate-limiting is better than a uint64_t

I think either will work fine, the uint64_t generalizes a bit nicer but we probably won't be needing the generalization in the future, so whichever ends up being the most elegant implementation now is what I think we should go for.

> I am partial to introducing a new
...

🤔 rkrux reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-2896780371)
Partial code review 981b2abd79e3a7a4701c9d8794057b5523acb177
I just started and am still reviewing.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126630415)
```diff
- "musig() is only allowed in tr()"
+ "musig() is only allowed in tr() and rawtr()"
```

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126634254)
```diff
- musig(): musig() derivation requires all participants to be xpubs
+ musig(): derivation requires all participants to be xpubs or xprvs
```

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126642131)
Bitwise operations on bools, sorry to be the one raising this multiple times. :)

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126700491)
Can add `Assume` and throw if the derivation type is hardened, it's not allowed because IIUC, `m_derive` refers to the derivation type of this musig provider only.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126695356)
`IsRangedParticipants` is called multiple times in this class, can evaluate it once in the constructor itself and store the result in a const. I don't suppose a `MuSigPubkeyProvider` is updated with more `m_participants` after creation.

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126684020)
Currently, [BIP 390](https://github.com/bitcoin/bips/blob/master/bip-0390.mediawiki) doesn't have a mention of multipath participant keys. As per [BIP 389](https://github.com/bitcoin/bips/blob/master/bip-0389.mediawiki):

> Descriptors that contain multiple Key Expressions that each have a /<NUM;NUM;...;NUM> must have tuples of exactly the same length so that they are derived in lockstep in the same way that /* paths in multiple Key expressions are handled.

By using this lockstep approach,
...

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126702224)
```diff
- Either way, we can passthrough to it's GetPubKey
+ Either way, we can passthrough to its GetPubKey
```

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126663894)
> There are some scenarios where I think it's ambiguous whether they should be allowed.

"Repeated participant public keys are not allowed. The aggregate public key is produced by using the KeyAgg algorithm on all KEYs specified in the expression after performing all specified derivation."

The current language^ in [BIP 390](https://github.com/bitcoin/bips/blob/master/bip-0390.mediawiki) gives me an impression that the public key derived from the KEY expression should not be repeated that su
...

💬 rkrux commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2126734205)
Nit: The calculation of aggregate public key & its provider can be done in the constructor itself unless it's anticipated that for some reason dummy MuSigPubkeyProviders will be used later.

👍 fanquake approved a pull request: "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`"
(https://github.com/bitcoin/bitcoin/pull/32651#pullrequestreview-2897024505)
ACK 18cf727429e967053d6a7468c9a40dc14185af34

💬 hMsats commented on issue "bitcoind 29.0 much slower than 28.0 on my system: cause found":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2940296584)
That's an impressive amount of data analyses that will take me some time to fully understand but thanks a lot!

I did some more testing but for my system 2 MiB is a clear winner and the good news is that then my server runs flawlessly but I need to let it run for more blocks. On the other hand, there are other people running version 29.0 and (with google) I haven't seen any complains yet.

> In your case (with compact blocks) an empty mempool would also introduce some variation.

During testing
...

🚀 fanquake merged a pull request: "cmake: Replace deprecated `qt6_add_translation` with `qt6_add_lrelease`"
(https://github.com/bitcoin/bitcoin/pull/32651)

🤔 hodlinator reviewed a pull request: "headerssync: Keep tests ahead of increasing params"
(https://github.com/bitcoin/bitcoin/pull/32579#pullrequestreview-2896798619)
Latest push (42e746096b93b8026569817d8925750089d6daf4, [compare](https://github.com/bitcoin/bitcoin/compare/bbe5609b84211c49a522da9dd5d987ab454ef912..42e746096b93b8026569817d8925750089d6daf4)):
* Relaxes the hardcoding of the header commitment period and the redownload buffer size in `HeadersSyncState`.
- This makes things more consistent with the constructor already taking `Consensus::Params`.
- Moves specification of the above constants from the top of *headerssync.cpp* into *kernel/cha
...

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2126641973)
Added generation date in latest push as it was making modifications to the Python script anyway.

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2126653972)
Implemented in latest push.

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2126644715)
Latest push has:
```C++
struct HeadersSyncParams {
//! Distance in blocks between header commitments.
size_t commitment_period;
...
```
in *src/kernel/chainparams.h*.

✅ maflcko closed an issue: "Windows Python: Intermittent issue in p2p_unrequested_blocks.py", line 98, in run_test test_node.send_and_ping(msg_block(blocks_h2[0])) assert self.is_connected AssertionError / AcceptBlock FAILED (time-too-old, block's timestamp is too early) "
(https://github.com/bitcoin/bitcoin/issues/29897)

💬 maflcko commented on issue "Windows Python: Intermittent issue in p2p_unrequested_blocks.py", line 98, in run_test test_node.send_and_ping(msg_block(blocks_h2[0])) assert self.is_connected AssertionError / AcceptBlock FAILED (time-too-old, block's timestamp is too early) ":
(https://github.com/bitcoin/bitcoin/issues/29897#issuecomment-2940318094)
According to https://github.com/python/cpython/commit/1d95451be1f3080904c00cc4c4a6cc519efdf321 the precision on Windows of `time.time()` was only 15.6 ms! This was fixed in python3.13, which is available on GHA runners for a few months now: https://github.com/actions/runner-images/commit/c8b6f67c08223abbce0b553ad4a2346ea03bdfd5#diff-d3119a278ce9ce80fc4b5af63a1112fb3a044aec3e6002d838e8eb26d7a3a499R60

(For reference the MS STL commit that dropped support for windows 7 is https://github.com/micros
...