💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211725)
Done

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211791)
Done

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105211849)
Done

💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105212407)
Would this be more descriptive?
```suggestion
LogDebug(BCLog::COINDB, "Writing chainstate to disk: flush mode=%s, prune=%d, cache_large=%d, cache_critical=%d, periodic=%d",

```

💬 jonatack commented on issue "test: `tool_wallet.py` references no-longer used CI":
(https://github.com/bitcoin/bitcoin/issues/32576#issuecomment-2905417856)
@m3dwards those comments can be dropped completely; see #28116, which I will update and open.

💬 achow101 commented on pull request "walletdb: Log additional exception error messages for corrupted wallets":
(https://github.com/bitcoin/bitcoin/pull/32598#issuecomment-2905423874)
> Should also change [HasLegacyRecords()](https://github.com/bitcoin/bitcoin/blob/2df824f4e62b6bc569044819cd64f66f3839ba13/src/wallet/walletdb.cpp#L544) as it would log the same error twice with this change.

Done

💬 davidgumberg commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2105223515)
nit for here and above:
```suggestion
$installationPath = & $vswherePath -latest -property installationPath
```

or could drop the line where `$vswherePath` is defined, whatever your preference is.

💬 jonatack commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105227345)
That does seem nicer.

🤔 jonatack reviewed a pull request: "blocks: avoid recomputing block header hash in `ReadBlock`"
(https://github.com/bitcoin/bitcoin/pull/32487#pullrequestreview-2865412103)
Concept ACK

💬 jonatack commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2105238684)
This error log change no longer provides info about the index that doesn't match.

Examples from the unit test log:

`$ ./build/bin/test_bitcoin --run_test=blockmanager_tests -l test_suite -- DEBUG_LOG_OUT`

before
```
025-05-23T18:42:55.808136Z (mocktime: 2020-08-31T15:34:12Z) [test] [node/blockstorage.cpp:1033] [ReadBlock] [error] GetHash() doesn't match index for CBlockIndex(pprev=0x6000019085e0, nHeight=100, merkle=851b8e14be513191d6dd7df5ac3c997c2b0b1f58c99143905eac61b296d01169, has
...

💬 jonatack commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2105241036)
(Thank you for adding the unit test!)

🤔 jonatack reviewed a pull request: "walletdb: Log additional exception error messages for corrupted wallets"
(https://github.com/bitcoin/bitcoin/pull/32598#pullrequestreview-2865443270)
Concept ACK, would be great to add test coverage for this change.

💬 pinheadmz commented on issue "rpc: Wrong `gettransaction` info for a coinjoin":
(https://github.com/bitcoin/bitcoin/issues/14136#issuecomment-2905521838)
I'm digging in to this issue, it has two closed PRs: #16199 and #19002 and a test asserting the current, somewhat unexpected, behavior: #18919

There are two things to consider:

1. We don't store input amounts in the wallet, only output amounts -- and only output amounts we create. That means we can't compute the fee for a tx unless we created all the outputs the tx is spending.
2. In the context of a coinjoin a "fee" field could refer to either the actual network fee paid to the miner, OR just
...

💬 gmaxwell commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2905537657)
I don't recall if the vulnerability related to requesting the transaction was closed (relay pool limited its scope at least to recently announced txn) but there were complications related to orphan txn (e.g. that relaying a transaction ought to imply permission to fetch its parents if they are still in your memory pool). But if that vulnerablity hasn't been closed it ought to be closed.

Ignoring the unrequested CB is desirable for non-privacy reasons, specifically because if it doesn't someone
...

💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2105273746)
Yes, there is no index available, only the desired hash, wherever it comes from.
Would it help if we included the two different hashes in the error?

💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2105277605)
Good call, changed, rebased, added you as co-author.

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2105280293)
Ah. I've added a test which exercises this code.

💬 achow101 commented on pull request "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs":
(https://github.com/bitcoin/bitcoin/pull/32596#issuecomment-2905565617)
ACK e5cbea416b2f63e5d99819052f3e69a6383336d6

💬 ryanofsky commented on issue "subprocess: `sh -c 'echo err 1>&2 && false'` must return `1`":
(https://github.com/bitcoin/bitcoin/issues/32574#issuecomment-2905570392)
> The underlying issue is not specific to illumos.

I couldn't find an explanation of the underlying issue, but looking at this more, I think I maybe understand the problem. The test code that is failing looks like:

```c++
49 // Return non-zero exit code, with error message for stderr
50 const std::string command{"sh -c 'echo err 1>&2 && false'"};
51 const std::string expected{"err"};
52 BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std:
...

🚀 achow101 merged a pull request: "wallet, rpc, doc: various legacy wallet removal cleanups in RPCs"
(https://github.com/bitcoin/bitcoin/pull/32596)