💬 fanquake commented on pull request "cmake: Check for `makensis` tool before using it":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2708956458)
> the NSIS tool shouldn't be a strict requirement for a successful configuration.

Then the same should be done for macOS and zip, otherwise it's still inconsistent.

💬 l0rinc commented on pull request "refactor: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2708960491)
@maflcko, I've reorganized the PR as a refactor where the happy path is now easier to reason about (i.e. no change to the incoming string most of the time) - where the speed gain is just a side-effect of the simpler path taken (it's why the benchmarks are now `PriorityLevel::LOW`).

Please let me know if this fits better with the priorities.

💬 Chand-ra commented on pull request "refactor: simplify GetAncestor":
(https://github.com/bitcoin/bitcoin/pull/31778#issuecomment-2708960914)
Concept ACK [5983f1](https://github.com/bitcoin/bitcoin/pull/31778/commits/5983f166c94dd5ab172e38bf12a3330a3ed9004c)
`CBlockIndex::GetAncestor` _is_ easier to comprehend with this change, but I'm not sure if duplicating the two versions in `test/blockchain_tests.cpp` is the best way to test the change. A single test to verify `GetAncestor` agnostic of its implementation might be better?

💬 Chand-ra commented on issue "Bug: Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in `AddWalletDescriptor`":
(https://github.com/bitcoin/bitcoin/issues/31728#issuecomment-2708961954)
Hey @asklokesh, are you still working on this or can I chime in to help as well?

💬 furszy commented on issue "Bug: Non-Ranged Descriptors with Range [0,0] Trigger Unexpected Wallet Errors in `AddWalletDescriptor`":
(https://github.com/bitcoin/bitcoin/issues/31728#issuecomment-2708965049)
> Hey [@asklokesh](https://github.com/asklokesh), are you still working on this or can I chime in to help as well?

You can tackle it. asklokesh comment is from chatGPT and makes no sense in our code. We should delete it.

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2709025950)
> > the NSIS tool shouldn't be a strict requirement for a successful configuration.
>
> Then the same should be done for macOS and zip, otherwise it's still inconsistent.

Sure thing! Added.

The PR description has been updated.

💬 mabu44 commented on pull request "torcontrol: Limit reconnect timeout to max seconds and log delay in whole seconds":
(https://github.com/bitcoin/bitcoin/pull/31979#issuecomment-2709111641)
Tested with same results as @eval-exec. After reaching the maximum "reconnect_timeout" value I started tor and it re-connected successfully, then I stopped tor again to check that reconnect_timeout restarts from 1 second.
ACK f708498293c27f63507cfa08c74909ba3d1aa675

🤔 l0rinc reviewed a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2669535869)
Found a few more static extent opportunities, but I personally don't mind doing these in separate PRs either.

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1986432102)
we could retain the static extents here as well:
```suggestion
s.write(std::as_bytes(std::span<uint8_t, 1>{&obj, 1}));
```

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1986432892)
`last(Bytes)` seems to create a dynamic extent, we could keep the sizes with something like:
```C++
s.write(std::as_bytes(std::span{&raw, 1}).template last<Bytes>());
```
and similarly later:
```C++
s.read(std::as_writable_bytes(std::span{&raw, 1}).last<Bytes>());
```

which seems to eliminate the need for
```C++
template <typename Stream, BasicByte B> void Unserialize(Stream& s, std::span<B> span) { s.read(std::as_writable_bytes(span)); }
```

💬 l0rinc commented on pull request "optimization: speed up block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2709143828)
Drafting until https://github.com/bitcoin/bitcoin/pull/31519 is merged, as recommended in https://github.com/bitcoin/bitcoin/pull/31868#discussion_r1956480589

📝 l0rinc converted_to_draft a pull request: "optimization: speed up block serialization"
(https://github.com/bitcoin/bitcoin/pull/31868)
This PR contain a few different optimization I found by IBD profiling, and via the newly added block seralization benchmarks.

The commits merge similar (de)serialization methods, and separates them internally with `if constexpr` - similarly to how it has been [done here before](https://github.com/bitcoin/bitcoin/pull/28203). This enabled further `SizeComputer` optimizations as well.

Other than these, since single byte writes are used very often (used for every `(u)int8_t` or `std::byte` o
...

💬 l0rinc commented on pull request "optimization: speed up block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r1986458092)
Rebased on top of https://github.com/bitcoin/bitcoin/pull/31519 and experimented with static extents - the speed is not the same as with bare `std::byte` parameters, but close enough and the code is more general.
Drafting until the span PR is merged - suggestions for further investigations are welcome.

📝 RiceChuan opened a pull request: "docs: remove repetitive words"
(https://github.com/bitcoin/bitcoin/pull/32022)
docs: remove repetitive words

🤔 jonatack reviewed a pull request: "docs: remove repetitive words"
(https://github.com/bitcoin/bitcoin/pull/32022#pullrequestreview-2669776693)
NACK. This patch is trying to touch a subtree repo that would need to be changed upstream and not here, as the failing CI is confirming.

✅ fanquake closed a pull request: "docs: remove repetitive words"
(https://github.com/bitcoin/bitcoin/pull/32022)

📝 saikiran57 opened a pull request: "removed duplicate call to GetDescriptorScriptPubKeyMan "
(https://github.com/bitcoin/bitcoin/pull/32023)
https://github.com/bitcoin/bitcoin/issues/32013

🤔 rkrux reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2669096676)
Few comments.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986049686)
This new checks here makes the flow more robust and avoids relying on implicit assumptions of what's going on inside the `Expand` function.

> Instead of relying on implicit assumptions about whether pubkeys show up
after expanding a descriptor, just explicitly allow only single
key descriptors to import keys into a legacy wallet's keypool.

The commit message 6a31eb5dca1f8eb5fd0507852b49c7452031269e, however, was difficult for me to parse. The second clause, which I understand (and made m
...

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101902)
This comment is vaguely correct now, technically ok but doesn't gel well with the new object name, causes confusion. IMO best to update it.