🤔 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.
(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()`)
(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?
(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`?
(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.
(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()`
(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
...
(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?
(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
...
(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`.
(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)
```
(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?
(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
(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
...
(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.
(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.
(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)
(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
...
(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
...
(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
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2444741664)
nit: Declaration differs from definition, `block` vs `data` parameter name