💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007597399)
not for this PR but there is no need to call `GetDescriptorScriptPubKeyMan()` here, can just return `AddWalletDescriptor()` output.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007604057)
We are no longer asserting that `spk_manager!=nullptr`.
Could unify these lines + the missing check:
```c++
auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
assert(spk_manager);
```

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007619742)
Same as above; would be good to not mix coding styles. No jump line after the condition statement.

And the `res` variable is shadowing another variable in this function. Would be better to rename it to `res_spkm` or `spkm_res`.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007630950)
The `res` variable is already used in this function, by declaring it here again you are shadowing the other one. Would be better to rename it to `res_spkm` or `spkm_res`.

Also, missing whitespace after `if`.

💬 furszy commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r2007611055)
Would be good to follow the same code style we have in this function. No jump line after the condition statement.

💬 sipa commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2743492906)
I do understand the desire to have accessible simple unit test cases for tapscript, and more testing certainly doesn't hurt. However, making things like this possible and easy to write was the goal of the taproot test framework, though), which has advantages like supporting combinations with non-trivial script trees, adding auto-generated signatures, integrating it all into transactions and blocks and running through the entire stack, plus having the ability to export the resulting cases to JSON
...

💬 BrandonOdiwuor commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2007717777)
fixed

💬 virtu commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2743564244)
ACK 4c8e9b4f35be4104002ece15b6f72c360756f5d9

Tested on Linux and MacOS.

Nice work. I'm looking to integrating some UTXO sats on my website, so it's nice to see people working on conveniently extracting this data.

💬 virtu commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2743579116)
Great work!

Curious about policy on tooling since I'm currently working on UTXO analysis: it would be nice to refactor the (1) reading of the compressed UTXO format and (2) storing it in SQL format into separate components, so there's a reference for reading dumped UTXO sets people can readily use (e.g. `import CompressedUTXOReader from contrib/utxo-tools`). Or should code like that not be a part of Core?

💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#issuecomment-2743639950)
Force-pushed to address @furszy's feedback

✅ maflcko closed an issue: "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan"
(https://github.com/bitcoin/bitcoin/issues/32013)

💬 maflcko commented on issue "Check if the wallet already contains the descriptor GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/issues/32013#issuecomment-2743685449)
Let's move the discussion to the pull request

⚠️ dergoegge opened an issue: "wallet: migratewallet crashes "Assertion `m_wallet_flags == 0' failed""
(https://github.com/bitcoin/bitcoin/issues/32111)
`migratewallet` crashes on malformed `wallet.dat`s which is unlikely to happen with wallets created by old Bitcoin Core versions but we should probably return an error to the user instead of crashing.

<details>

<summary>Base64 encoded `wallet.dat` that causes a crash</summary>

```
AAAAAAEAAAAAAAAAYjEFAAkAAAAAEAAAAAkAAAAAAAADAAAAAAAAAAAAAAAAAAAAIAAAAHsA+AEBAwEAQ0PCvWvWAgAAAAAAAAAAAAIAAAAAAAAAIAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
...

💬 maflcko commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2743692730)
> after this fix improved performance of importdescriptor part refs #32013.

If this is a performance improvement, please explain exact steps to reproduce the speedup and how much it is.

⚠️ dergoegge opened an issue: "wallet: migratewallet crashes "Assertion `legacy_spkm' failed""
(https://github.com/bitcoin/bitcoin/issues/32112)
`migratewallet` crashes on malformed `wallet.dat`s which is unlikely to happen with wallets created by old Bitcoin Core versions but we should probably return an error to the user instead of crashing.

<details>

<summary>Base64 encoded `wallet.dat` that causes a crash</summary>

```
AAAAAAEAAAAAAAAAYjEFAAkAAAAAEAAAAAkAAAAAAAADAAAAAAAAAAAAAAAAAAAAIAAAAHsA+AEBAwEAQ0PCvWvWAgAAAAAAAAAAAAIAAAAAAAAAIAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA
...

💬 yancyribbens commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2743737637)
> The long_term_fee_rate and fee_rate are the same for all UTXOs in a single coin selection attempt

Agree. I think that's a little confusing the way the C++ algorithm is structured and treats fee_rate and long_term_fee_rate per UTXO instead of just passing in the fee_rate params as a per sort argument.

> so if two UTXOs have the same fee, their inputs have the same weight

Right, since fee is derived from fee_rate and weight, and fee_rate is guaranteed to be the same, the only true variable i
...

📝 ajtowns opened a pull request: "fuzz: enable running fuzz test cases in Debug mode"
(https://github.com/bitcoin/bitcoin/pull/32113)
When building with

BUILD_FOR_FUZZING OFF
BUILD_FUZZ_BINARY ON
CMAKE_BUILD_TYPE Debug

allow the fuzz binary to execute given test cases (without actual fuzzing) to make it easier to reproduce fuzz test failures in a more normal debug environment.

💬 ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743752847)
Related historical PRs: #31191 #24472

💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2743792938)
I don't think the current patch will work. `G_FUZZING` influences more than just the behavior of the asserts/assumes. For example:

* POW checks are different
* The task runner is different
* The random seeding is different

So with this patch it looks like you are instead opting into non-reproducible fuzz behavior.

In debug mode the performance doesn't matter, so a possible alternative fix could be to just make G_FUZZING a runtime bool again?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2743809558)
ReACK 1601906941fa559ebbee7898453fa77f4606ad38