💬 PiRK commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3420788196)
@l0rinc Yes, #32313 fixes it.

Tested with this patch:
```
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index a1152d245f..a3ee441c42 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -880,6 +880,7 @@ Coin MakeCoin()
{
Coin coin;
coin.out.nValue = m_rng.rand32();
+ coin.out.scriptPubKey.assign(m_rng.randbits(6), 0);
coin.nHeight = m_rng.randrange(4096);
coin.fCoinBase = 0;
return coin;
```
coins_tests fails as
...

💬 PiRK commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3420846050)
AFAICT the issue my patch raised was only reachable after suppressing a `std::logic_error`, which shouldn't happen in production code.

💬 ubbabeck commented on pull request "test: multisig verify spend from 100 of 999 taproot multisig wallet":
(https://github.com/bitcoin/bitcoin/pull/33658#issuecomment-3420984051)
Closing this as it's rather trivial to add once the https://github.com/bitcoin/bitcoin/issues/29098 and rather focus on testing and reviewing it.

✅ ubbabeck closed a pull request: "test: multisig verify spend from 100 of 999 taproot multisig wallet"
(https://github.com/bitcoin/bitcoin/pull/33658)

📝 halilyucel opened a pull request: "Improve fatal error message for block read failures in BaseIndex::ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/33659)
When a block read operation fails in BaseIndex::ProcessBlock, the current error message only reports:
Failed to read block %s from disk

This message doesn't provide enough context to help developers or node operators diagnose the problem.

What changed?
The fatal error message now includes:
The block hash (already existed)
The block height
A clearer description of the issue
A hint to check whether the block exists in the database

Before:
```
FatalErrorf("Failed to read block %s f
...

📝 NadiaDjarbo opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/33660)
<br>## Summary<br>As a new contributor to Bitcoin Core, I added a beginner-friendly welcome note to the README.md to highlight node setup for the original cryptocurrency. This helps newcomers understand the peer-to-peer electronic cash system! <br><br>## Motivation<br>New users often struggle with initial node sync in issues—this note provides a quick intro to reduce barriers for Bitcoin enthusiasts.<br><br>## Changes<br>- Added a new subsection "## Welcome to Bitcoin Node Setup" at the end of R
...

💬 optout21 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2444741664)
nit: Declaration differs from definition, `block` vs `data` parameter name

💬 optout21 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2444750382)
This way both `block` and `blockpart` api's take the same paramaters, including `"offset"` and `"size"`, right? Is that intended?

📝 optout21 opened a pull request: "test: Add test on skip heights in CBlockIndex"
(https://github.com/bitcoin/bitcoin/pull/33661)
The skip height values, used by `CBlockIndex::BuildSkip()` and `GetSkipHeight` are not tested and not well documented. (noticed while reviewing #33515, recently merged).

The motivation is to document the skip value computation through a test.

The first commit adds a test for the properties of the distribution of skip values, namely that they have non-uniform distribution: most values are small but there are some large ones as well.

The second commit adds low-level tests to the `GetSkipH
...

🤔 maflcko reviewed a pull request: "fix: invalid plist format and update values to meet macOS 1.0 standard"
(https://github.com/bitcoin/bitcoin/pull/33654#pullrequestreview-3356260624)
is this llm generated?

💬 maflcko commented on pull request "fix: invalid plist format and update values to meet macOS 1.0 standard":
(https://github.com/bitcoin/bitcoin/pull/33654#discussion_r2444851592)
Isn't 10.14 different from 14.0?

💬 stickies-v commented on pull request "Improve fatal error message for block read failures in BaseIndex::ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/33659#issuecomment-3421841236)
NACK, feels like busywork, don't see how this improves things.

✅ halilyucel closed a pull request: "Improve fatal error message for block read failures in BaseIndex::ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/33659)

💬 famouswizard commented on pull request "fix: invalid plist format and update values to meet macOS 1.0 standard":
(https://github.com/bitcoin/bitcoin/pull/33654#discussion_r2444875498)
[maflcko](https://github.com/maflcko), yes, exactly - the previous value `14` was incorrect because it refers to macOS 14 (Sonoma). the correct minimum version is `10.14.0` (Mojave), using the proper X.Y.Z format required by LSMinimumSystemVersion.

🤔 naiyoma reviewed a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3356353069)
utACK 84b2ad03349bf6af3d6057f3dbcdf84c17f11bcb

💬 w0xlt commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2444943716)
nit: Could reuse one temporary provider to reduce alloc churn.

```suggestion
FlatSigningProvider tmp_provider;
for (const auto& pubkey : m_pubkey_args) {
tmp_provider.keys.clear();
pubkey->GetPrivKey(0, arg, tmp_provider);
if (tmp_provider.keys.empty()) return false;
}
```

🤔 cedwies reviewed a pull request: "ci: Only write docker build images to Cirrus cache"
(https://github.com/bitcoin/bitcoin/pull/33639#pullrequestreview-3356419572)
utACK fa29ecd
Reviewed the code and the new CI flow. The change to limit cache writes to Cirrus (while still reading cache on GHA) and moving the build step into Python with `shlex.split(os.getenv("DOCKER_BUILD_CACHE_ARG", ""))` looks good to me. I don’t see any unintended behavior changes beyond what is described in the PR.

💬 ajtowns commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2445023085)
CTxMemPoolEntry to only need to be movable for `mempool_entries.emplace_back(ConsumeTxMempoolEntry(...))` in test/fuzz/policy_estimator.cpp.

I'm a bit surprised `Ref::operator=(Ref&&)` isn't virtual -- moving a Ref that might actually a mempool entry seems dangerous; but otoh, moving by operator= only seems to be used in fuzz/txgraph.cpp, which means we don't actually move anything that's not just a Ref.

💬 laanwj commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445067273)
Mind that `codecvt_utf8_utf16` (and the entirety of codecvt) is deprecated in C++17 and will be removed in C++26. We might not want to introduce another instance.
(see [codecvt_utf8_utf16](https://en.cppreference.com/w/cpp/locale/codecvt_utf8_utf16.html) and #32361)

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2445073249)
Done.

✅ l0rinc closed a pull request: "log: unify `UpdateTip` values"
(https://github.com/bitcoin/bitcoin/pull/32996)