π¬ wnqqnw19 commented on issue "Batch tx broadcast RPC":
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3613286511)
The current submitpackage RPC requires callers to pre-construct a valid package in strict parentβchild order.
If transactions are given in the wrong order or structure, the call fails.
This is difficult for wallets, Lightning implementations, and APIs, because they need to manually detect parent/child relationships and build the exact shape Bitcoin Core expects.
It would be much more convenient if the RPC accepted an unordered batch of transactions and Bitcoin Core automatically figured out
...
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3613286511)
The current submitpackage RPC requires callers to pre-construct a valid package in strict parentβchild order.
If transactions are given in the wrong order or structure, the call fails.
This is difficult for wallets, Lightning implementations, and APIs, because they need to manually detect parent/child relationships and build the exact shape Bitcoin Core expects.
It would be much more convenient if the RPC accepted an unordered batch of transactions and Bitcoin Core automatically figured out
...
π¬ fanquake commented on pull request "chainparams: remove dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3613305317)
Backported to `30.x` in #33997.
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3613305317)
Backported to `30.x` in #33997.
π fanquake's pull request is ready for review: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997)
(https://github.com/bitcoin/bitcoin/pull/33997)
π ryanofsky approved a pull request: "mining: fix -blockreservedweight shadows IPC option"
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3541102064)
Code review ACK 54d3054db129d4dd5aaf9faa3f6e6922159059b0. Just some more review suggestions applied since last review. This is a pretty clean fix preventing the `-blockreservedweight` configuration value from overriding the IPC requested value.
The followup #33966 will make the code merging IPC options and configuration options more clear. And a different followup could give IPC clients ability to use the `-blockreservedweight` value instead of needing to provide their own value.
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3541102064)
Code review ACK 54d3054db129d4dd5aaf9faa3f6e6922159059b0. Just some more review suggestions applied since last review. This is a pretty clean fix preventing the `-blockreservedweight` configuration value from overriding the IPC requested value.
The followup #33966 will make the code merging IPC options and configuration options more clear. And a different followup could give IPC clients ability to use the `-blockreservedweight` value instead of needing to provide their own value.
π€ Crypt-iQ reviewed a pull request: "fuzz: Add fuzz target for block index tree and related validation events"
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3536286958)
Looks good, left some comments. There is some non-determinism I'm looking into that I didn't see before which is a little weird. It might be completely unrelated to this PR.
(https://github.com/bitcoin/bitcoin/pull/31533#pullrequestreview-3536286958)
Looks good, left some comments. There is some non-determinism I'm looking into that I didn't see before which is a little weird. It might be completely unrelated to this PR.
π¬ Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586114315)
nit: I think the `it->nHeight > 0` branch is unnecessary? If `nHeight` is 0 and we are here, then that means the fork point was before the genesis block which should be impossible?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586114315)
nit: I think the `it->nHeight > 0` branch is unnecessary? If `nHeight` is 0 and we are here, then that means the fork point was before the genesis block which should be impossible?
π¬ Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472)
Would it be a good idea to also call `InvalidChainFound(to_connect.front())` similar to [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3289)?
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472)
Would it be a good idea to also call `InvalidChainFound(to_connect.front())` similar to [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3289)?
π¬ Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586804372)
Should this be 2 instead? See [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.h#L1058).
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586804372)
Should this be 2 instead? See [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.h#L1058).
π¬ Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2589659925)
I hacked together a patch that does this, it hasn't found anything. Feel free to use if you want:
<details>
<summary> diff </summary>
```diff
diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
index 037d168e0e..a8c0dc98cf 100644
--- a/src/test/fuzz/block_index_tree.cpp
+++ b/src/test/fuzz/block_index_tree.cpp
@@ -49,6 +49,8 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
blocks.push_back(genesis);
bool abort_run{fa
...
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2589659925)
I hacked together a patch that does this, it hasn't found anything. Feel free to use if you want:
<details>
<summary> diff </summary>
```diff
diff --git a/src/test/fuzz/block_index_tree.cpp b/src/test/fuzz/block_index_tree.cpp
index 037d168e0e..a8c0dc98cf 100644
--- a/src/test/fuzz/block_index_tree.cpp
+++ b/src/test/fuzz/block_index_tree.cpp
@@ -49,6 +49,8 @@ FUZZ_TARGET(block_index_tree, .init = initialize_block_index_tree)
blocks.push_back(genesis);
bool abort_run{fa
...
π¬ Crypt-iQ commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586279490)
(Just a clarifying question)
Is this true of ABC? From my reading, this is true of ABCStep but not ABC since ABC will stop (besides failure or interrupt) only when our tip is the block with most work (see: [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3519)).
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586279490)
(Just a clarifying question)
Is this true of ABC? From my reading, this is true of ABCStep but not ABC since ABC will stop (besides failure or interrupt) only when our tip is the block with most work (see: [here](https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/validation.cpp#L3519)).
π wnqqnw19 opened a pull request: "Wnqqnw19"
(https://github.com/bitcoin/bitcoin/pull/34007)
```
## Changes Made
1. **Renamed `format` parameter to `format_version`** (line 18)
- Avoids shadowing Python's built-in `format()` function
- Improves code clarity and follows Python best practices
2. **Removed redundant local assignment of `ADDRMAN_NEW_BUCKET_COUNT`** (line 35)
- Now uses the imported constant from `test_framework.netutil` directly
- Ensures consistency with the centralized constant definition
- If the constant changes in the future, this code will a
...
(https://github.com/bitcoin/bitcoin/pull/34007)
```
## Changes Made
1. **Renamed `format` parameter to `format_version`** (line 18)
- Avoids shadowing Python's built-in `format()` function
- Improves code clarity and follows Python best practices
2. **Removed redundant local assignment of `ADDRMAN_NEW_BUCKET_COUNT`** (line 35)
- Now uses the imported constant from `test_framework.netutil` directly
- Ensures consistency with the centralized constant definition
- If the constant changes in the future, this code will a
...
β
fanquake closed a pull request: "Wnqqnw19"
(https://github.com/bitcoin/bitcoin/pull/34007)
(https://github.com/bitcoin/bitcoin/pull/34007)
π¬ purpleKarrot commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613330088)
> Some low-level code could benefit from being able to use `std::expected` from C++23.
Can you elaborate? How would it benefit? Over what alternative?
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613330088)
> Some low-level code could benefit from being able to use `std::expected` from C++23.
Can you elaborate? How would it benefit? Over what alternative?
π ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3541130613)
Code review ACK 5761dac2f5481c27550483279a007c2b850bc990, with the two suggestions applied since last review.
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3541130613)
Code review ACK 5761dac2f5481c27550483279a007c2b850bc990, with the two suggestions applied since last review.
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589928992)
> Why not simply call `NumToOpenAdd(num_for_rebroadcast)`
This would be the change:
<details>
<summary>Use only num_for_rebroadcast</summary>
```diff
void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
{
- // The following heuristic is subject to races, but that is ok: if it overshoots,
- // we will open some private connections in vain, if it undershoots, the stale
- // transactions will be picked on the next run.
-
- size_t active_connections{0
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589928992)
> Why not simply call `NumToOpenAdd(num_for_rebroadcast)`
This would be the change:
<details>
<summary>Use only num_for_rebroadcast</summary>
```diff
void PeerManagerImpl::ReattemptPrivateBroadcast(CScheduler& scheduler)
{
- // The following heuristic is subject to races, but that is ok: if it overshoots,
- // we will open some private connections in vain, if it undershoots, the stale
- // transactions will be picked on the next run.
-
- size_t active_connections{0
...
π sedited approved a pull request: "depends: update freetype and document remaining `bitcoin-qt` runtime libs"
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3541144045)
Guix build:
```
760ca6bb241b02b496785a0123cea4e9e49e03e27e2280824237bd4125ffb1d0 guix-build-41e657aacfa6/output/aarch64-linux-gnu/SHA256SUMS.part
432c63a2879ef00793c9378f64d5130752258c0d845b565c4fa7c708399be01a guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu-debug.tar.gz
15c65f8720598ff04dba8d15b0b49e5ab9064f6798159b331681806a62f28426 guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu.tar.gz
d53c1ef1fd1501f0452d
...
(https://github.com/bitcoin/bitcoin/pull/33952#pullrequestreview-3541144045)
Guix build:
```
760ca6bb241b02b496785a0123cea4e9e49e03e27e2280824237bd4125ffb1d0 guix-build-41e657aacfa6/output/aarch64-linux-gnu/SHA256SUMS.part
432c63a2879ef00793c9378f64d5130752258c0d845b565c4fa7c708399be01a guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu-debug.tar.gz
15c65f8720598ff04dba8d15b0b49e5ab9064f6798159b331681806a62f28426 guix-build-41e657aacfa6/output/aarch64-linux-gnu/bitcoin-41e657aacfa6-aarch64-linux-gnu.tar.gz
d53c1ef1fd1501f0452d
...
π¬ maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613425769)
> > Some low-level code could benefit from being able to use `std::expected` from C++23.
>
> Can you elaborate? How would it benefit? Over what alternative?
Sure. The alternatives were listed in doc diff in the commit, but I've pulled it out and put in the pull description as well.
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3613425769)
> > Some low-level code could benefit from being able to use `std::expected` from C++23.
>
> Can you elaborate? How would it benefit? Over what alternative?
Sure. The alternatives were listed in doc diff in the commit, but I've pulled it out and put in the pull description as well.
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589984428)
Marking as resolved, checked with @andrewtoth on IRC, thanks!
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2589984428)
Marking as resolved, checked with @andrewtoth on IRC, thanks!
π€ jaonoctus reviewed a pull request: "init: point out -stopatheight may be imprecise"
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3541279438)
re-ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
(https://github.com/bitcoin/bitcoin/pull/33993#pullrequestreview-3541279438)
re-ACK ff06e2468a5d3eeebeffe781904c34c9d1b44385
π¬ hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2590030400)
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2590030400)
Thanks! Reworked.