💬 maflcko commented on pull request "Docs: fix typos in documentation files":
(https://github.com/bitcoin/bitcoin/pull/32011#issuecomment-2708875507)
lgtm ACK 5601bab4f8b01fdef7a54c9e397d513217ab1c1f

💬 maflcko commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31912#issuecomment-2708878173)
The pull request title and commit message looks off. Also there seems to be an ongoing discussion in https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2687917758

📝 glozow converted_to_draft a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829)
This PR is part of the orphan resolution project, see #27463.

We want to limit the CPU work and memory used by `TxOrphanage` to avoid denial of service attacks. On master, this is achieved by limiting the number of transactions in this data structure to 100, and the weight of each transaction to 400KWu (the largest standard tx) [0]. We always allow new orphans, but if the addition causes us to exceed 100, we evict one randomly. This is dead simple, but has problems:
- It makes the orphanage
...

💬 fanquake commented on pull request "cmake: Check for `makensis` tool before using it":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2708916206)
Now that `nsis` is required, the docs about installing `nsis` in `build-windows.md` should be updated.

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-2708921843)
Run into this again recently. Did it ever get reported upstream?

💬 hebasto commented on pull request "cmake: Check for `makensis` tool before using it":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2708949698)
> Now that `nsis` is required, the docs about installing `nsis` in `build-windows.md` should be updated.

Thanks for pointing this out!

Since the `deploy` target is optional, the NSIS tool shouldn't be a strict requirement for a successful configuration.

I've reworked this PR so that the error message "Error: NSIS not found" is only printed when attempting to build the `deploy` target.

📝 hebasto converted_to_draft a pull request: "cmake: Check for `makensis` tool before using it"
(https://github.com/bitcoin/bitcoin/pull/32019)
Fixes https://github.com/bitcoin/bitcoin/issues/32018.

👋 hebasto's pull request is ready for review: "cmake: Check for `makensis` tool before using it"
(https://github.com/bitcoin/bitcoin/pull/32019)

💬 l0rinc commented on pull request "RFC: Add `operator""_uint256` compile-time user-defined literal":
(https://github.com/bitcoin/bitcoin/pull/31991#discussion_r1986367249)
Removed the `consteval_ctor` change since godbolt and the MSVC docs have mixed signals about it

🚀 hebasto merged a pull request: "Docs: fix typos in documentation files"
(https://github.com/bitcoin/bitcoin/pull/32011)

💬 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)); }
```