💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#issuecomment-3419973259)
Some of the PR description is reused from the previous PR, but is now stale.

> This PR is adding support for using the transaction's position within its block to be able to fetch it directly using REST API, using the following HTTP request (to fetch the N-th transaction from BLOCKHASH):

This does not support fetching an N-th transaction, only a slice of a block.

> If binary response format is used, the transaction data will be read directly from the storage and sent back to the client, withou
...

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2443539559)
Do we want to support zero `*part_size`? Should probably return false too?

🤔 Copilot reviewed a pull request: "Replace cluster linearization algorithm with SFL"
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-3354761107)
## Pull Request Overview

This PR replaces the existing cluster linearization algorithm (LIMO with candidate-set search) with a new spanning-forest linearization (SFL) algorithm that offers significantly better performance for complex clusters. The new algorithm uses a fundamentally different approach based on maintaining spanning trees within chunks and performing split/merge operations.

**Key Changes:**
- Replaces search-based linearization with spanning-forest based approach
- Simplifies qua
...

💬 Copilot commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2443583986)
The new `is_topological` parameter defaults to `true`, but when `old_linearization` is empty, this parameter is ignored. This could be confusing for API consumers. Consider documenting this behavior more explicitly in the function comment, or validate that when `old_linearization` is empty, `is_topological` is not set to `false`.

💬 Copilot commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2443583989)
[nitpick] The function signature changed from accepting `const std::array<uint8_t, N>&` to `const std::vector<uint8_t>&`. This removes compile-time size checking. While this makes the function more flexible, it also removes a safety guarantee. Consider if the added flexibility is worth the loss of compile-time size verification.
```suggestion
template <size_t N>
void BenchLinearizeOptimally(benchmark::Bench& bench, const std::array<uint8_t, N>& serialized)
```

💬 l0rinc commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3420329732)
@PiRK, can you please check if this was already fixed by https://github.com/bitcoin/bitcoin/pull/32313?

💬 yuvicc commented on pull request "test: [move-only] binary utils to utils.py":
(https://github.com/bitcoin/bitcoin/pull/33633#issuecomment-3420744948)
LGTM ACK fa75ef4328f638221bcf85fcbefa885122084622

💬 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)