💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2916658444)
I'm interested in seeing benchmarks of this on various systems, especially high-end/modern ones (as their performance is likely most predictive of what future hardware will be like).
```
./build_dev_mode/bin/bench_bitcoin --filter="Linearize.*Example.*" -min-time=10000
```
Here are my own:
#### AMD Ryzen 9 5950X 16-Core Processor
| ns/cost | cost/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:-------
...
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-2916658444)
I'm interested in seeing benchmarks of this on various systems, especially high-end/modern ones (as their performance is likely most predictive of what future hardware will be like).
```
./build_dev_mode/bin/bench_bitcoin --filter="Linearize.*Example.*" -min-time=10000
```
Here are my own:
#### AMD Ryzen 9 5950X 16-Core Processor
| ns/cost | cost/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:-------
...
📝 mzumsande opened a pull request: "test: fix sync function in rpc_psbt.py"
(https://github.com/bitcoin/bitcoin/pull/32630)
Even though the block is created on `node2`, the sync is only between `node1` and `node0`. Accordingly the test fails if I put a sleep in `msg_type == NetMsgType::HEADERS` processing: In this, case `node1` and `node0` do not hear about the new block, the sync still passes because they are in sync with each other, and later on in the `test_input_confs_control` subtest, `node1` would generate a forked block instead of building on the previous one, leading to test failure.
Haven't seen this in
...
(https://github.com/bitcoin/bitcoin/pull/32630)
Even though the block is created on `node2`, the sync is only between `node1` and `node0`. Accordingly the test fails if I put a sleep in `msg_type == NetMsgType::HEADERS` processing: In this, case `node1` and `node0` do not hear about the new block, the sync still passes because they are in sync with each other, and later on in the `test_input_confs_control` subtest, `node1` would generate a forked block instead of building on the previous one, leading to test failure.
Haven't seen this in
...
💬 maflcko commented on pull request "test: fix sync function in rpc_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/32630#issuecomment-2916784818)
lgtm ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
(https://github.com/bitcoin/bitcoin/pull/32630#issuecomment-2916784818)
lgtm ACK 4df4df45d7bc2e8be99325d40cda936aab87c083
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111949140)
In "ipc: Add bitcoin-mine test program" 6841bae335d109e0207aed002635cd12e609f9a8
When their is no bitcoin-node process we don't spawn a new one but exit instead.
I attempt to call `spawnProcess` and pass `bitcoin-node` as executable.
The client crashes
```
2025-05-28T15:06:39Z Default data directory /Users/abubakarismail/Library/Application Support/Bitcoin
2025-05-28T15:06:39Z Using data directory /Users/abubakarismail/Library/Application Support/Bitcoin
2025-05-28T15:06:39Z Config
...
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111949140)
In "ipc: Add bitcoin-mine test program" 6841bae335d109e0207aed002635cd12e609f9a8
When their is no bitcoin-node process we don't spawn a new one but exit instead.
I attempt to call `spawnProcess` and pass `bitcoin-node` as executable.
The client crashes
```
2025-05-28T15:06:39Z Default data directory /Users/abubakarismail/Library/Application Support/Bitcoin
2025-05-28T15:06:39Z Using data directory /Users/abubakarismail/Library/Application Support/Bitcoin
2025-05-28T15:06:39Z Config
...
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111970868)
In "ipc: Add bitcoin-mine test program" 6841bae335d109e0207aed002635cd12e609f9a8
The valid address probably shouldn’t be hardcoded here. It would be better to have a function that returns all valid addresses, with a default constant for unix.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111970868)
In "ipc: Add bitcoin-mine test program" 6841bae335d109e0207aed002635cd12e609f9a8
The valid address probably shouldn’t be hardcoded here. It would be better to have a function that returns all valid addresses, with a default constant for unix.
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111981130)
In "ipc: Add bitcoin-mine test program" https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8
Maybe specify the default datadir
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2111981130)
In "ipc: Add bitcoin-mine test program" https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8
Maybe specify the default datadir
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112177316)
In "ipc: Add bitcoin-mine test program" https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8
Should validate the address first?
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112177316)
In "ipc: Add bitcoin-mine test program" https://github.com/bitcoin/bitcoin/commit/6841bae335d109e0207aed002635cd12e609f9a8
Should validate the address first?
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112219834)
I attempted it in https://github.com/ismaelsadeeq/bitcoin/commit/c714339f6bdce1be68ce127be98dffdae56dbaa4
With an added test to the `Echo` interface.
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2112219834)
I attempted it in https://github.com/ismaelsadeeq/bitcoin/commit/c714339f6bdce1be68ce127be98dffdae56dbaa4
With an added test to the `Echo` interface.
📝 marcofleon opened a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631)
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
(https://github.com/bitcoin/bitcoin/pull/32631)
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-2916823179)
In `txrequest` the `ByPeer` and `ByTxHash` indexes still sort announcements by the underlying uint256 txhash only, as opposed to using the GenTxid comparators which sort by type and then hash. The patch below can be used to make sure that there are no accidental switches from uint256 comparisons to GenTxid comparisons.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 011e3cd928..1a47a39dfd 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -206,6
...
(https://github.com/bitcoin/bitcoin/pull/32631#issuecomment-2916823179)
In `txrequest` the `ByPeer` and `ByTxHash` indexes still sort announcements by the underlying uint256 txhash only, as opposed to using the GenTxid comparators which sort by type and then hash. The patch below can be used to make sure that there are no accidental switches from uint256 comparisons to GenTxid comparisons.
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 011e3cd928..1a47a39dfd 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -206,6
...
💬 theuni commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2916839229)
Pushed a small additional commit that removes a now-unnecessary template instantiation. Just a little janitorial work, but it will make some future changes more obviously correct.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-2916839229)
Pushed a small additional commit that removes a now-unnecessary template instantiation. Just a little janitorial work, but it will make some future changes more obviously correct.
💬 hebasto commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2916853608)
There seems a silent conflict with https://github.com/bitcoin/bitcoin/pull/32396.
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2916853608)
There seems a silent conflict with https://github.com/bitcoin/bitcoin/pull/32396.
💬 dergoegge commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2112279755)
The return values for `GetHash` and `GetWitnessHash` need to be assigned to gtxid
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2112279755)
The return values for `GetHash` and `GetWitnessHash` need to be assigned to gtxid
💬 theStack commented on pull request "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs":
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-2917055444)
> > I think I'd intuitively prefer `raw` for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization
>
> Maybe `raw` and `rawle` then?
`raw`/`rawle` are a decent alternative that I'd slightly prefer over `rawhash`/`raw`, yes! Force-pushed accordingly. I'm fine with both though, so if other reviewers / potential users of that script feel strongly on `rawhash`/`raw`, I'm happy to revert.
> For future consideration `--spk=type` so that it only re
...
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-2917055444)
> > I think I'd intuitively prefer `raw` for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization
>
> Maybe `raw` and `rawle` then?
`raw`/`rawle` are a decent alternative that I'd slightly prefer over `rawhash`/`raw`, yes! Force-pushed accordingly. I'm fine with both though, so if other reviewers / potential users of that script feel strongly on `rawhash`/`raw`, I'm happy to revert.
> For future consideration `--spk=type` so that it only re
...
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2917116354)
Not sure were to report, so I will drop this here.
While writing a custom ipc-client, I noticed that the IPC will freeze when you shutdown the node while the ipc-client is still running.
How to reproduce
- Apply the following patch to this branch:
<details>
<summary>diff</summary>
```diff
diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index e31f666a68e..3b9e1b81964 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -16,6 +16,11 @@
#include <logging.h>
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2917116354)
Not sure were to report, so I will drop this here.
While writing a custom ipc-client, I noticed that the IPC will freeze when you shutdown the node while the ipc-client is still running.
How to reproduce
- Apply the following patch to this branch:
<details>
<summary>diff</summary>
```diff
diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index e31f666a68e..3b9e1b81964 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -16,6 +16,11 @@
#include <logging.h>
...
🤔 w0xlt reviewed a pull request: "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs"
(https://github.com/bitcoin/bitcoin/pull/32621#pullrequestreview-2875935564)
reACK https://github.com/bitcoin/bitcoin/pull/32621/commits/1b10e882f3478d82095aa194f5941e02fb4a68a8
(https://github.com/bitcoin/bitcoin/pull/32621#pullrequestreview-2875935564)
reACK https://github.com/bitcoin/bitcoin/pull/32621/commits/1b10e882f3478d82095aa194f5941e02fb4a68a8
💬 w0xlt commented on pull request "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs":
(https://github.com/bitcoin/bitcoin/pull/32621#discussion_r2112440482)
Does `rawle` mean `raw little-endian`? If so, it would be better to be explicit in the description.
```suggestion
parser.add_argument('--txid', choices=['hex', 'raw', 'rawle'], default='hex', help='encode txid as hex, raw bytes (sha256 byteorder), or reversed raw bytes (litle-endian)')
```
Although `rev_raw` or something similar would also work fine.
(https://github.com/bitcoin/bitcoin/pull/32621#discussion_r2112440482)
Does `rawle` mean `raw little-endian`? If so, it would be better to be explicit in the description.
```suggestion
parser.add_argument('--txid', choices=['hex', 'raw', 'rawle'], default='hex', help='encode txid as hex, raw bytes (sha256 byteorder), or reversed raw bytes (litle-endian)')
```
Although `rev_raw` or something similar would also work fine.
🤔 w0xlt reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-2876056497)
ACK https://github.com/bitcoin/bitcoin/pull/32606/commits/1fc95e080040a9d038a8291eeecf645e0413a5c8
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-2876056497)
ACK https://github.com/bitcoin/bitcoin/pull/32606/commits/1fc95e080040a9d038a8291eeecf645e0413a5c8
🤔 w0xlt reviewed a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876074131)
Approach ACK.
The first item "Adds a message to the beginning of PartiallyDownloadedBlock::InitData() so that that the logs indicate the amount of time it takes to populate a compact block from mempool transactions." could be improved by adding another message to the end of `PartiallyDownloadedBlock::InitData` indicating that execution has completed and how long it took.
(https://github.com/bitcoin/bitcoin/pull/32582#pullrequestreview-2876074131)
Approach ACK.
The first item "Adds a message to the beginning of PartiallyDownloadedBlock::InitData() so that that the logs indicate the amount of time it takes to populate a compact block from mempool transactions." could be improved by adding another message to the end of `PartiallyDownloadedBlock::InitData` indicating that execution has completed and how long it took.
💬 w0xlt commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112525323)
Is this variable being used ?
(https://github.com/bitcoin/bitcoin/pull/32582#discussion_r2112525323)
Is this variable being used ?