🤔 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.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101911)
This comment seems partly outdated now because, as I see, there is no more copying happening.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101919)
Overall the update count (in this file atleast) of the members of the class `FlatSigningProvider& out` has decreased now because there are fewer `PubkeyProvider`s and more `DescrImpl`s , which IMO is desirable.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101904)
Took me some time to gather what's going on here.

For reviewing and future reference as well, it would be easier if this commit 070e746c2f72db2cc24b1c267b91fafbcc586736 is split into 2:
Usage of just `info` object and the removal of `parent_info`, `final_info_out_tmp` objects with the changes related to it can be one commit. Rest of the commit changes can be part of the other.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987014769)
```diff
-const SigningProvider& arg
+const SigningProvider&
```

`arg` is not used in this function, though not caused by this PR.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1986101951)
IMO `GetPrivKey` commit 72694b8d1a9863771cf1095037caeafb3f1a5635 changes deserves a mention in the PR description, currently only the pubkey changes are mentioned.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987021149)
```diff
-// All subproviders must be inserting a valid origin already
+// `m_provider` must be inserting a valid origin already
```

Using the word subproviders here strips this function off its individuality and instead brings in context from its caller. At any time, there would just be one subprovider that is referenced by the `OriginPubkeyProvider` class member `m_provider`.

💬 rkrux commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1987033523)
+1 for removing the usage of `entries.back().first, entries.back().second` as the output parameters to the `GetPubKey()` - this was not intuitive to read for me.