💬 hugohn commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2553746288)
Hey guys, we've successfully integrated this into Nunchuk, so you should be able to test this out with a real UI/UX very soon.

MuSig2 key path spend: https://mempool.space/tx/69c75aa798e03dbe782c9a11eed316440fa2a4cb9c4645af2f5d8d566c04207b?mode=details

MuSig2 script path spend: https://mempool.space/tx/73f63684994477924105966e646427f7fc802352d9ba9d1baebf05b1f3dc3fab?mode=details

Sample descriptors:
`tr(musig([15d62cdf/87'/0'/0']xpub6CpM1svHYyNMTVdmDh5syFXCJHKctJNajbyLEdA8pAgAeg1jotmg9G
...

💬 maflcko commented on pull request "qa: Ignore "Reducing -maxconnections..." warning in stderr":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553780090)
It is nice to see code changes, but generally for review it would be good to explain the code changes and provide useful context and alternatives.

Specifically, it would be good to explain why you didn't just set maxconnections appropriately.

Silently ignoring the warning seems like a foot-gun to hit later on, when more connections than that are created and the test fails again on netbsd (or whatever), in which case someone has to create another pull.

📝 maflcko opened a pull request: "refactor: std::span compat fixes"
(https://github.com/bitcoin/bitcoin/pull/31540)
The `std::span` type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from `Span`. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts.

Fix all those issues by allowing either `Span` or `std::span` in any part of the codebase.

All of the changes are also required for the scripted-diff to replace `Span` with `std::span` in https://github.com/bitcoin/bitcoin/
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1891993661)
Thanks for [updating](https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2553418201) to option 4. Just to summarize what we talked about offline:

> but I wanted to keep a separate chainstate load function in case we ever land a "blocks-only read-only" chainstate manager

That makes sense with the current code organization, but I think we should aim to shift towards a more intuitive API over time. Operations that don't require any chainstate (such as blocks-only read-only) probably
...

💬 hebasto commented on pull request "qa: Ignore "Reducing -maxconnections..." warning in stderr":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553859322)
> Specifically, it would be good to explain why you didn't just set maxconnections appropriately.

I couldn't determine an appropriate `-maxconnections` value. It depends on [tests](https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553506832) and may vary in the future. You recent [suggestion](https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553487975) ("maybe double or triple that") seems quite arbitrary as well.

> Silently ignoring the warning seems like a foot-gun to
...

💬 maflcko commented on pull request "qa: Ignore "Reducing -maxconnections..." warning in stderr":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2553899790)
You can simply set it to the max value in netbsd, which doesn't seem arbitrary. A higher value can not be used anyway in tests. The tests certainly would fail if more connections are tried to be made. If you disagree, please share a log on netbsd where you are creating more than 128 connections and the tests pass, with exact steps to reproduce.

💬 l0rinc commented on pull request "optimization: cache block[undo] serialized size for consecutive calls":
(https://github.com/bitcoin/bitcoin/pull/31490#issuecomment-2553915546)
> we could potentially take this further by caching the size even earlier

Absolutely, but that's a bigger change (would cache the serialized sizes in `CBlock`, guarding against any other mutation (which requires better encapsulation), storing `GetSerializeSize` for `TX_NO_WITNESS()` and `TX_WITH_WITNESS()` lazily, similarly to the existing `checked*` flags)... but that's a big change, affecting a lot of consensus code, I'm still working on that and will push in a separate PR.

🤔 sipa reviewed a pull request: "refactor: std::span compat fixes"
(https://github.com/bitcoin/bitcoin/pull/31540#pullrequestreview-2514640077)
utACK fa163e04ca62b832650c9c942ebe240ad46351c8

💬 sipa commented on pull request "refactor: std::span compat fixes":
(https://github.com/bitcoin/bitcoin/pull/31540#discussion_r1892051234)
Would `in.data()` work here instead of `&in.front()`? That expresses the intent better I think, because we don't actually just want the first element.

👍 hodlinator approved a pull request: "fuzz: Fix misplaced SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31521#pullrequestreview-2514441974)
ACK fadd568931a2d21e0f80e1efaf2281f5164fa20e

Prior to commit fae63bf13033adec80c7e6d73144a21ea3cfbc6d (#31486):
* `utxo_total_supply` fuzz target was initializing `ChainTestingSetup` which would call `SeedRandomStateForTest(SeedRand::FIXED_SEED)`.
* `SeedRandomStateForTest(SeedRand::FIXED_SEED)` would seed randomness using a value from a static variable initialized once during process startup.
* This was making each fuzz target execution (input) use the same value.

After fae63bf13033ade
...

💬 hodlinator commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#discussion_r1891892020)
(nit: Slight preference for the old text that doesn't dictate specific lines).

💬 maflcko commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#discussion_r1892105596)
Will leave as-is for now, to not invalidate the 3 acks. Is there a reason why the first line could not be used for this with the current codebase in master?

💬 fjahr commented on pull request "Make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#issuecomment-2554086181)
re-ACK 81cea5d4ee0e226f7c7145072de85829f4166ec9

💬 hodlinator commented on pull request "fuzz: Fix misplaced SeedRand::ZEROS":
(https://github.com/bitcoin/bitcoin/pull/31521#discussion_r1892213050)
Was thinking there may creep up something else that also needs initializing really early and either is unaffected by randomness or somehow needs to affect randomness seeding (yes, contrived).

Then there is the idea to have a mixin seeding the randomness internally, which would change this part regardless.

💬 maflcko commented on pull request "refactor: std::span compat fixes":
(https://github.com/bitcoin/bitcoin/pull/31540#discussion_r1892385830)
thx, done

💬 maflcko commented on pull request "qa: Ignore "Reducing -maxconnections..." warning in stderr":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2554483808)
lgtm ACK c7e21e32e3e59dcd0c6169fb179e7927e805d8e6

💬 hebasto commented on pull request "qa: Limit `-maxconnections` in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2554490122)
> You can simply set it to the max value in netbsd, which doesn't seem arbitrary. A higher value can not be used anyway in tests. The tests certainly would fail if more connections are tried to be made.

Thanks @maflcko! Your suggestion has been implemented.

💬 hebasto commented on pull request "qa: Limit `-maxconnections` in tests":
(https://github.com/bitcoin/bitcoin/pull/31537#issuecomment-2554496497)
Fixed white space linter warning.

🚀 ryanofsky merged a pull request: "Make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325)

🚀 ryanofsky merged a pull request: "fuzz: Fix misplaced SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31521)