✅ romanz closed a pull request: "index: store per-block transaction locations for efficient lookups"
(https://github.com/bitcoin/bitcoin/pull/32541)

💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3419729543)
Closing this PR, the "index-less" approach will be implemented in https://github.com/bitcoin/bitcoin/pull/33657.

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3419748327)
Updated to use `std::barrier` for the completion synchronization instead of acquiring a semaphore for each thread, as suggested by @l0rinc .

🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3353593203)
I have started a per-commit review. These are mostly related to the first commit (dc504f57) + CI comments submitted earlier.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2443264406)
Isn't it better to set `status` to `SCRIPT_VERIFY_OK` internally (if not null) after above checks rather than expecting callers to pre-initialize it? (also adapting the doc in the header for `btck_script_pubkey_verify()` and removing the two lines that revert the status value to `OK` in `run_verify_test()`)

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2443282691)
Duplicate declarations?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2443266860)
Why we are skipping value `1`?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2443366124)
A bit confused by this part.
For example for the first assertion, I guess we wanted to compare `object2.get() != distinct_object.get()` to verify the copy constructor worked? And maybe for the second assertion we wanna check if `object2` and `distinct_object` have the same bytes size?
Also for the `Copy assignment` section assertions that comes after.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2442427401)
Missing `BITCOINKERNEL_WARN_UNUSED_RESULT`.

Also for:
- `btck_transaction_out_point_get_txid()`
- `btck_chain_contains()`
- `btck_transaction_out_point_get_index()`
- `btck_txid_equals()`
- `btck_block_hash_equals()`

Not sure about the following (they return 0 on success):
- `*_to_bytes()`
- `btck_chainstate_manager_options_set_wipe_dbs()`
- `btck_chainstate_manager_import_blocks()`

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