💬 musaHaruna commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553077585)
Fixed

💬 musaHaruna commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553095074)
Yeah, I think it should be, I will look into how I can do that, am open to suggestions on how that can be done!

💬 yuvicc commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3566677137)
Rebased to fix the CI error. For reference check https://github.com/bitcoin/bitcoin/issues/33884.

💬 musaHaruna commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553121377)
This assertion ensures that the newly created watch-only wallets start from a completely clean state with no prior transaction history or birthtime. That way, we can be sure that any transactions discovered after the import come solely from the descriptor import and the UTXO scan, not from pre-existing wallet data.

💬 musaHaruna commented on pull request "wallet, rpc: add UTXO set check and incremental rescan to importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/33392#discussion_r2553122533)
I think this is part of backupwallet RPC and I don't think I touch the backupwallet rpc codenthe entire PR.

👍 benthecarman approved a pull request: "Revert "gui, qt: brintToFront workaround for Wayland""
(https://github.com/bitcoin-core/gui/pull/914#pullrequestreview-3496739218)
fixes for me!

🤔 l0rinc reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3496417072)
Latest push:
* rebases against latest master;
* reverts code back to simply change `leaves.resize(block.vtx.size())` constructs before calling `ComputeMerkleRoot` to `.reserve((block.vtx.size() + 1) & ~1ULL)`;
** added godbolt reproducer for each important combination of compiler and architecture
* Separates the test changes to a separate commit explaining the motivation in the commit message
* updated commit messages and PR descriptions.

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2552816773)
We're comparing it against the results of `BlockWitnessMerkleRoot` which returns `GetWitnessHash`.
https://github.com/bitcoin/bitcoin/pull/16865 didn't specify why they're comparing `GetHash` against `GetWitnessHash` here, I deduced it's a coincidence. I don't mind keeping `GetHash` if you think it was deliberate.

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553107836)
> Accessing txs.size() one time less at runtime

The compiler should be able to deduce that it's the same (see assembly below): https://en.wikipedia.org/wiki/Common_subexpression_elimination

-----

I have considered a few alternatives:
```C++
BOOST_AUTO_TEST_CASE(merkle_test_even_sizing)
{
for (size_t size = 0; size <= 1'000'000; ++size) {
size_t reference = size + (size & 1U);

BOOST_CHECK_EQUAL(reference, (size + 1) & ~size_t{1});
BOOST_CHECK_EQUAL(re
...

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553128138)
Thanks, ended up reverting `ToMerkleLeaves` completely to simplify the review

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553128897)
Reverted to the original implementation, it's the least-invasive change with simplest diff

💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3566713903)
Thanks for the explanation @achow101!

> renaming PRs are generally limited to things that are particularly misleading and actively causing confusion.

I assumed that was the case here - if not, maybe the parent PR could also just avoid the rename in the first place.

> I get that having smaller PRs can make it feel like things are moving faster for a project, but it also means that there is an intermediate state where we will have merged a bunch of prep work but not the feature itself.

...

🤔 hebasto reviewed a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3496784212)
I've tested fa2fd0ba1fbd4085df24a2c5472636957db28521 on a Windows 11 laptop using VS 2022 17.14.21.

Functional tests work as expected.

🤔 sipa reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3496807348)
ACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2553198152)
In commit "Rewrite GatherClusters to use the txgraph implementation"

Nit for a follow-up: this 500 limit doesn't really help with anything if it's only enforced after doing all the work. I would think it can either be dropped, or moved into the loop above after every `GetCluster` call.

📝 hebasto opened a pull request: "qa: Remove no longer needed `feature_dirsymlinks.py`"
(https://github.com/bitcoin/bitcoin/pull/33924)
This PR follows up on https://github.com/bitcoin/bitcoin/pull/33842, which removed (1) the [unused `create_directories` workaround](https://github.com/bitcoin/bitcoin/pull/33842/commits/fa9dacdbde7dc18d134019bdad24f47e4dea1dda) and (2) the corresponding [unit test](https://github.com/bitcoin/bitcoin/pull/33842/commits/fa3854e43295f71f5dad8557dd621f0f799b0ee0), but left the related functional test in place. The latter is now being removed.

For historical context, see:
- https://github.com/bit
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553215114)
By "logic" you mean

```cpp
case node::TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
what = "for private broadcast without adding to the mempool";
```
normally "yes" but the thing is that the `switch` without the case will fail to compile. Would you prefer something like:

```cpp
case node::TxBroadcast::NO_MEMPOOL_PRIVATE_BROADCAST:
what = "for private broadcast without adding to the mempool";
throw std::runtime_error("Remove this throw when callers exist
...

💬 plebhash commented on issue "should concurrent IPC requests directed to the same thread cause a crash?":
(https://github.com/bitcoin/bitcoin/issues/33923#issuecomment-3566852691)
I guess the main thing I wanted to report here is a duplicate of https://github.com/bitcoin-core/libmultiprocess/pull/214, so it's good to know these crashes won't happen on `v30.x`!

> how many requests should be allowed? Should it allow 5, 10, or 100 requests before returning "thread busy"? Or an arbitrary amount? Or a configurable amount?

it would be up to the client to know what are the reasonable bounds for backpressure so that it can avoid the need to handle errors, so I'd say that a conf
...

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553224623)
Changed to
```
if ...
if ...
return ok
return notok
```
while avoiding `if (int x = expression; x == 5)`.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2553227771)
Changed to have nested `if`s.