💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2507937182)
I updated the documentation per out discussion here to read:
`
If the maximum weight is exceeded, the OutputGroups with the lowest effective value are removed from the selection until weight is acceptable. By removing the lowest effective value, the average effective value per weight of the selection is increased and thus reducing the average selection size. If a solution is found, the resulting selection will produce a change output with an amount of at least CHANGE_LOWER.
`
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2507937182)
I updated the documentation per out discussion here to read:
`
If the maximum weight is exceeded, the OutputGroups with the lowest effective value are removed from the selection until weight is acceptable. By removing the lowest effective value, the average effective value per weight of the selection is increased and thus reducing the average selection size. If a solution is found, the resulting selection will produce a change output with an amount of at least CHANGE_LOWER.
`
🤔 TheCharlatan reviewed a pull request: "kernel: Add block header support and validation"
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3439843515)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/33822#pullrequestreview-3439843515)
Approach ACK
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508027260)
Can you add `BOOST_CHECK_THROW(BlockHeader{hex_string_to_byte_vec("00")}, std::runtime_error);`?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508027260)
Can you add `BOOST_CHECK_THROW(BlockHeader{hex_string_to_byte_vec("00")}, std::runtime_error);`?
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2507927303)
I think the variable name `state` should just be kept.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2507927303)
I think the variable name `state` should just be kept.
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508070029)
This (and `btck_block_validation_state_copy` needs `BITCOINKERNEL_WARN_UNUSED_RESULT`
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508070029)
This (and `btck_block_validation_state_copy` needs `BITCOINKERNEL_WARN_UNUSED_RESULT`
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508041569)
I saw that in your own patch, you wrapped this call in a try-catch block. As far as I can tell we should not run into an exception here, but maybe it is still better to add it for defensive reasons?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508041569)
I saw that in your own patch, you wrapped this call in a try-catch block. As far as I can tell we should not run into an exception here, but maybe it is still better to add it for defensive reasons?
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508065115)
One thing that could be considered here is returning the `BlockValidationState` directly instead of having an in/out param. To safely do that I think we'd need to refactor `ProcessNewBlockHeaders` though, similarly to what was done in https://github.com/bitcoin/bitcoin/commit/74690f4ed82b1584abb07c0387db0d924c4c0cab. Not sure that is worth it.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508065115)
One thing that could be considered here is returning the `BlockValidationState` directly instead of having an in/out param. To safely do that I think we'd need to refactor `ProcessNewBlockHeaders` though, similarly to what was done in https://github.com/bitcoin/bitcoin/commit/74690f4ed82b1584abb07c0387db0d924c4c0cab. Not sure that is worth it.
💬 TheCharlatan commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508035777)
I wouldn't add this length check here and would instead just rely on the exception. We do the same thing in the rpc too: `submitheader` just calls `DecodeHexBlockHeader`, which doesn't have a length check either.
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508035777)
I wouldn't add this length check here and would instead just rely on the exception. We do the same thing in the rpc too: `submitheader` just calls `DecodeHexBlockHeader`, which doesn't have a length check either.
👍 TheCharlatan approved a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3440065494)
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3440065494)
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
💬 yancyribbens commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508162294)
Isn't this something the fuzz harness can help answer?
(https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508162294)
Isn't this something the fuzz harness can help answer?
💬 Aa777263100 commented on pull request "kernel: Add block header support and validation":
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3508516523)
نعم اريد سحب
hsan saed abd alwhap
في الأحد، 9 نوفمبر 2025 7:43 م yancy ***@***.***> كتب:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/kernel/bitcoinkernel.cpp
> <https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508162294>:
>
> > @@ -1225,6 +1258,16 @@ int btck_chainstate_manager_process_block(
> return result ? 0 : -1;
> }
>
> +int btck_chainstate_manager_process_block_header(
> + btck_ChainstateManager* chainst
...
(https://github.com/bitcoin/bitcoin/pull/33822#issuecomment-3508516523)
نعم اريد سحب
hsan saed abd alwhap
في الأحد، 9 نوفمبر 2025 7:43 م yancy ***@***.***> كتب:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/kernel/bitcoinkernel.cpp
> <https://github.com/bitcoin/bitcoin/pull/33822#discussion_r2508162294>:
>
> > @@ -1225,6 +1258,16 @@ int btck_chainstate_manager_process_block(
> return result ? 0 : -1;
> }
>
> +int btck_chainstate_manager_process_block_header(
> + btck_ChainstateManager* chainst
...
📝 777genius opened a pull request: "refactor: Add missing include in util/result.h"
(https://github.com/bitcoin/bitcoin/pull/33832)
## Summary
Add missing `#include <type_traits>` for `std::conditional_t` and `std::is_same_v` used in the `Result` template class.
## Motivation
Following the same principle as #33825, headers should directly include standard library headers for types they use in their public interface, rather than relying on transitive includes.
Currently, `src/util/result.h` uses `std::conditional_t` and `std::is_same_v` (line 38) but does not directly include `<type_traits>`. While the code compiles due t
...
(https://github.com/bitcoin/bitcoin/pull/33832)
## Summary
Add missing `#include <type_traits>` for `std::conditional_t` and `std::is_same_v` used in the `Result` template class.
## Motivation
Following the same principle as #33825, headers should directly include standard library headers for types they use in their public interface, rather than relying on transitive includes.
Currently, `src/util/result.h` uses `std::conditional_t` and `std::is_same_v` (line 38) but does not directly include `<type_traits>`. While the code compiles due t
...
📝 777genius opened a pull request: "script: Remove dead nullptr check in VerifyScript"
(https://github.com/bitcoin/bitcoin/pull/33833)
## Summary
Change `VerifyScript` witness parameter from pointer to reference, removing the `nullptr` check and static `emptyWitness` that were never used in practice.
## Motivation
Following the same principle as #33786, this PR removes dead code from `VerifyScript`.
Analysis of all call sites shows that:
- Production code always passes `&obj` (address of existing witness)
- Test code that needs empty witness now uses `EMPTY_SCRIPT_WITNESS` constant
The `nullptr` check was dead code, as evi
...
(https://github.com/bitcoin/bitcoin/pull/33833)
## Summary
Change `VerifyScript` witness parameter from pointer to reference, removing the `nullptr` check and static `emptyWitness` that were never used in practice.
## Motivation
Following the same principle as #33786, this PR removes dead code from `VerifyScript`.
Analysis of all call sites shows that:
- Production code always passes `&obj` (address of existing witness)
- Test code that needs empty witness now uses `EMPTY_SCRIPT_WITNESS` constant
The `nullptr` check was dead code, as evi
...
✅ ilkinsufi closed a pull request: "Increase tooltip wrap threshold from 80 to 100 characters"
(https://github.com/bitcoin-core/gui/pull/905)
(https://github.com/bitcoin-core/gui/pull/905)
📝 777genius opened a pull request: "refactor: Remove redundant boolean comparisons in txgraph.cpp"
(https://github.com/bitcoin/bitcoin/pull/33834)
## Summary
Replace explicit boolean comparisons (`== true`, `== false`) with idiomatic C++ boolean evaluation in `txgraph.cpp`.
## Motivation
Explicit comparisons with boolean literals reduce code readability without providing any benefit. This PR modernizes the code to follow C++ best practices.
## Changes
**In `src/txgraph.cpp`:**
- Replace `clusterset.m_oversized == true` with `clusterset.m_oversized` (6 instances)
- Replace `clusterset.m_oversized == false` with `!clusterset.m_oversized
...
(https://github.com/bitcoin/bitcoin/pull/33834)
## Summary
Replace explicit boolean comparisons (`== true`, `== false`) with idiomatic C++ boolean evaluation in `txgraph.cpp`.
## Motivation
Explicit comparisons with boolean literals reduce code readability without providing any benefit. This PR modernizes the code to follow C++ best practices.
## Changes
**In `src/txgraph.cpp`:**
- Replace `clusterset.m_oversized == true` with `clusterset.m_oversized` (6 instances)
- Replace `clusterset.m_oversized == false` with `!clusterset.m_oversized
...
📝 777genius opened a pull request: "refactor: Remove redundant ternary operator in wallet HaveChain()"
(https://github.com/bitcoin/bitcoin/pull/33835)
## Summary
Replace redundant ternary operator in `CWallet::HaveChain()` with explicit nullptr comparison.
## Motivation
The code `return m_chain ? true : false` is a redundant pattern where a ternary operator converts a boolean expression back to the same boolean value it already represents.
## Changes
**In `src/wallet/wallet.h:497`:**
```cpp
// Before:
bool HaveChain() const { return m_chain ? true : false; }
// After:
bool HaveChain() const { return m_chain != nullptr; }
```
## Benefits
...
(https://github.com/bitcoin/bitcoin/pull/33835)
## Summary
Replace redundant ternary operator in `CWallet::HaveChain()` with explicit nullptr comparison.
## Motivation
The code `return m_chain ? true : false` is a redundant pattern where a ternary operator converts a boolean expression back to the same boolean value it already represents.
## Changes
**In `src/wallet/wallet.h:497`:**
```cpp
// Before:
bool HaveChain() const { return m_chain ? true : false; }
// After:
bool HaveChain() const { return m_chain != nullptr; }
```
## Benefits
...
📝 777genius opened a pull request: "refactor: Remove redundant boolean comparisons across codebase"
(https://github.com/bitcoin/bitcoin/pull/33836)
## Summary
Replace explicit boolean comparisons (`== true`, `== false`) with idiomatic C++ boolean evaluation across multiple files.
## Motivation
Explicit comparisons with boolean literals reduce code readability without providing any benefit. This PR modernizes the code to follow C++ best practices across different subsystems.
## Changes
**Four files modified with one line each:**
1. **src/script/interpreter.cpp:2029**
```cpp
// Before:
if (CastToBool(stack.back()) == false)
...
(https://github.com/bitcoin/bitcoin/pull/33836)
## Summary
Replace explicit boolean comparisons (`== true`, `== false`) with idiomatic C++ boolean evaluation across multiple files.
## Motivation
Explicit comparisons with boolean literals reduce code readability without providing any benefit. This PR modernizes the code to follow C++ best practices across different subsystems.
## Changes
**Four files modified with one line each:**
1. **src/script/interpreter.cpp:2029**
```cpp
// Before:
if (CastToBool(stack.back()) == false)
...
💬 l0rinc commented on pull request "refactor: Remove redundant boolean comparisons in txgraph.cpp":
(https://github.com/bitcoin/bitcoin/pull/33834#discussion_r2508240436)
NACK, `m_oversized` is a `std::optional<bool>`
(https://github.com/bitcoin/bitcoin/pull/33834#discussion_r2508240436)
NACK, `m_oversized` is a `std::optional<bool>`
✅ 777genius closed a pull request: "refactor: Remove redundant boolean comparisons in txgraph.cpp"
(https://github.com/bitcoin/bitcoin/pull/33834)
(https://github.com/bitcoin/bitcoin/pull/33834)
💬 777genius commented on pull request "refactor: Remove redundant boolean comparisons in txgraph.cpp":
(https://github.com/bitcoin/bitcoin/pull/33834#issuecomment-3508667595)
Closing as this is purely stylistic change without clear technical benefit. Bitcoin Core discourages style-only refactoring PRs per CONTRIBUTING.md. Will focus on more substantive contributions.
(https://github.com/bitcoin/bitcoin/pull/33834#issuecomment-3508667595)
Closing as this is purely stylistic change without clear technical benefit. Bitcoin Core discourages style-only refactoring PRs per CONTRIBUTING.md. Will focus on more substantive contributions.