⚠️ hMsats reopened an issue: "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system"
(https://github.com/bitcoin/bitcoin/issues/32455)
I always compile the bitcoin software myself (`Ubuntu 24.04.2 LTS`). For `bitcoin-29.0` I compiled it (of course) for the first time with `cmake` (with and without `berkeley-db`).
For both (with and without `berkeley-db`), `bitcoind-29.0` is much slower than `bitcoin-28.0` which gives me problems on my server.
To investigate I wrote a shell script that determines the time difference in seconds between "Saw new header" and "UpdateTip":
```
2025-05-09T05:24:11Z Saw new header hash=000000000000
...
(https://github.com/bitcoin/bitcoin/issues/32455)
I always compile the bitcoin software myself (`Ubuntu 24.04.2 LTS`). For `bitcoin-29.0` I compiled it (of course) for the first time with `cmake` (with and without `berkeley-db`).
For both (with and without `berkeley-db`), `bitcoind-29.0` is much slower than `bitcoin-28.0` which gives me problems on my server.
To investigate I wrote a shell script that determines the time difference in seconds between "Saw new header" and "UpdateTip":
```
2025-05-09T05:24:11Z Saw new header hash=000000000000
...
💬 hMsats commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899388960)
@sipa After a long and systematic research of my problem, I was probably able to find the cause:
In validate.cpp `else if (!check())` was changed into `else if (auto result = check(); result.has_value())`.
Shouldn't that be `else if (auto result = check(); !result.has_value())`?
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899388960)
@sipa After a long and systematic research of my problem, I was probably able to find the cause:
In validate.cpp `else if (!check())` was changed into `else if (auto result = check(); result.has_value())`.
Shouldn't that be `else if (auto result = check(); !result.has_value())`?
💬 achow101 commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899407390)
> In validation.cpp `else if (!check())` was changed into `else if (auto result = check(); result.has_value())`.
> Shouldn't that be `else if (auto result = check(); !result.has_value())`?
Why do you think that's the error?
If this were incorrect, you wouldn't be seeing that validation be slow; it would instead be failing on a bunch of blocks.
This line is
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899407390)
> In validation.cpp `else if (!check())` was changed into `else if (auto result = check(); result.has_value())`.
> Shouldn't that be `else if (auto result = check(); !result.has_value())`?
Why do you think that's the error?
If this were incorrect, you wouldn't be seeing that validation be slow; it would instead be failing on a bunch of blocks.
This line is
💬 hMsats commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899420381)
@achow101 somewhere near that commit I think my server becomes slow. I didn't fully understand the code, so sorry. I will investigate further until I do find a problematic commit. Thanks!
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899420381)
@achow101 somewhere near that commit I think my server becomes slow. I didn't fully understand the code, so sorry. I will investigate further until I do find a problematic commit. Thanks!
💬 hMsats commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899433923)
For now I think something bad happens between commit:
52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
and
ebe4cac38bf6c510b00b8e080acab079c54016d6
I'll try to narrow it down.
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2899433923)
For now I think something bad happens between commit:
52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
and
ebe4cac38bf6c510b00b8e080acab079c54016d6
I'll try to narrow it down.
💬 ryanofsky commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2899448860)
> I've restored the `shell` option in the `subprocess` API, and employed it on non-Windows systems.
Hmm the shell support implemented in eb160602a50bebcca3f53cdae741e1732738d51a actually seems some broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.
It actually might not work too badly in that commit as long as the original string doesn't contain any tab characters, becau
...
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2899448860)
> I've restored the `shell` option in the `subprocess` API, and employed it on non-Windows systems.
Hmm the shell support implemented in eb160602a50bebcca3f53cdae741e1732738d51a actually seems some broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.
It actually might not work too badly in that commit as long as the original string doesn't contain any tab characters, becau
...
👍 theStack approved a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2859331801)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tested the wrapper more in-depth on Debian Linux 12, both with monolithic and multiprocess binaries this time, and also tried calling the wrapper without path separator to test PATH searching. Everything worked fine. Reviewed the code, though I only lightly looked at the Windows-specific parts in the `exec.cpp` module. Left some small refactoring and a comment improvement nit below, nothing blocking.
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2859331801)
ACK a5ac43d98d1ad3ebed934f2c50208a85aae17e5e
Tested the wrapper more in-depth on Debian Linux 12, both with monolithic and multiprocess binaries this time, and also tried calling the wrapper without path separator to test PATH searching. Everything worked fine. Reviewed the code, though I only lightly looked at the Windows-specific parts in the `exec.cpp` module. Left some small refactoring and a comment improvement nit below, nothing blocking.
💬 theStack commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124)
in commit 5076d20f: nit: since the error code `ec` isn't used, could use the shorter one-parameter variant of `is_regular_file` (denoted as (2) in https://en.cppreference.com/w/cpp/filesystem/is_regular_file)
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101317124)
in commit 5076d20f: nit: since the error code `ec` isn't used, could use the shorter one-parameter variant of `is_regular_file` (denoted as (2) in https://en.cppreference.com/w/cpp/filesystem/is_regular_file)
💬 theStack commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101326882)
in commit nit: since the error code `ec` isn't used, could use the shorter variant of `weakly_canonical` (denoted as (3) in https://en.cppreference.com/w/cpp/filesystem/canonical)
```suggestion
fs::path wrapper_dir{fs::weakly_canonical(wrapper_path)};
```
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101326882)
in commit nit: since the error code `ec` isn't used, could use the shorter variant of `weakly_canonical` (denoted as (3) in https://en.cppreference.com/w/cpp/filesystem/canonical)
```suggestion
fs::path wrapper_dir{fs::weakly_canonical(wrapper_path)};
```
💬 theStack commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101323251)
in commit 9c8c6889: nit: the directory list seems outdated
```suggestion
//! using bin and libexec directory paths relative to this executable, where
```
(don't know if it's really that important, but could even mentioned the "daemon" directory on Windows)
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2101323251)
in commit 9c8c6889: nit: the directory list seems outdated
```suggestion
//! using bin and libexec directory paths relative to this executable, where
```
(don't know if it's really that important, but could even mentioned the "daemon" directory on Windows)
💬 yancyribbens commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2101381865)
> Write scripts in Python or Rust rather than bash, when possible.
*technically* only Python is a "scripting language" for writing "scripts". I'm not sure you could say programs written in Rust are scripts, although I'm sure people will know what is intended by this sentence.
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2101381865)
> Write scripts in Python or Rust rather than bash, when possible.
*technically* only Python is a "scripting language" for writing "scripts". I'm not sure you could say programs written in Rust are scripts, although I'm sure people will know what is intended by this sentence.
📝 davidgumberg opened a pull request: "log: Additional compact block logging"
(https://github.com/bitcoin/bitcoin/pull/32582)
This PR adds some additional logging to help measure performance of compact block reconstruction.
1. 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.
2. Logs the size of the transactions which a node did not have in its mempool and was forced to request.
3. Logs the size and number of transactions that a node sends to it's peer in a `BLOCKTXN` to fulfill
...
(https://github.com/bitcoin/bitcoin/pull/32582)
This PR adds some additional logging to help measure performance of compact block reconstruction.
1. 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.
2. Logs the size of the transactions which a node did not have in its mempool and was forced to request.
3. Logs the size and number of transactions that a node sends to it's peer in a `BLOCKTXN` to fulfill
...
💬 adyshimony commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2899720171)
tACK
https://github.com/romanz/bitcoin/commit/8cb0465defdf96a86b7485ff098d2ba70843e942
Test:
```
$ ab -k -c 1 -n 100 http://localhost:8332/rest/spenttxouts/00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054.bin
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/
Benchmarking localhost (be patient).....done
Server S
...
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2899720171)
tACK
https://github.com/romanz/bitcoin/commit/8cb0465defdf96a86b7485ff098d2ba70843e942
Test:
```
$ ab -k -c 1 -n 100 http://localhost:8332/rest/spenttxouts/00000000000000000002a7c4c1e48d76c5a37902165a270156b7a8d72728a054.bin
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/
Benchmarking localhost (be patient).....done
Server S
...
💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2101589452)
"All checks have passed", thank you :heart:
(https://github.com/bitcoin/bitcoin/pull/29418#discussion_r2101589452)
"All checks have passed", thank you :heart:
⚠️ romanz opened an issue: "Support `Accept` HTTP header in REST API"
(https://github.com/bitcoin/bitcoin/issues/32583)
### Please describe the feature you'd like to see added.
Following https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2888533229, I would like to add support for the [HTTP `Accept` header](https://www.rfc-editor.org/rfc/rfc9110#name-accept), in addition to current "suffix-based" data format detection:
https://github.com/bitcoin/bitcoin/blob/87ec923d3a7af7b30613174b41c6fb11671df466/src/rest.cpp#L137-L160
WDYT?
### Is your feature related to a problem, if so please describe it.
_No res
...
(https://github.com/bitcoin/bitcoin/issues/32583)
### Please describe the feature you'd like to see added.
Following https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2888533229, I would like to add support for the [HTTP `Accept` header](https://www.rfc-editor.org/rfc/rfc9110#name-accept), in addition to current "suffix-based" data format detection:
https://github.com/bitcoin/bitcoin/blob/87ec923d3a7af7b30613174b41c6fb11671df466/src/rest.cpp#L137-L160
WDYT?
### Is your feature related to a problem, if so please describe it.
_No res
...
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2900073169)
Due to my anti-`shared_ptr` arguments in https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166 and the main point part of my `unique_ptr` alternative https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2809672862 (with some support https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2806161332) not being incorporated I have a hard time A-C-K:ing this as-is. Not planning to open a competing PR at this point though.
Curious what Cory comes up with (https://github.co
...
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2900073169)
Due to my anti-`shared_ptr` arguments in https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2049571166 and the main point part of my `unique_ptr` alternative https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2809672862 (with some support https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2806161332) not being incorporated I have a hard time A-C-K:ing this as-is. Not planning to open a competing PR at this point though.
Curious what Cory comes up with (https://github.co
...
💬 davidgumberg commented on pull request "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage":
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2101382426)
In commit 11fed833b3ed6d5c96957de5addc4f903b2cee6c (*threading: add LOCK_ARGS macro*):
---
Not blocking: `LOCK_ARGS` would also be useful for `LOCK` and `LOCK2` above, and it would make the differences among the four locking macros more obvious.
(https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2101382426)
In commit 11fed833b3ed6d5c96957de5addc4f903b2cee6c (*threading: add LOCK_ARGS macro*):
---
Not blocking: `LOCK_ARGS` would also be useful for `LOCK` and `LOCK2` above, and it would make the differences among the four locking macros more obvious.
💬 vasild commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2101815542)
I don't see this discussion as a blocker.
I just want to get to the bottom of it. Checked the source code of `http.client.HTTPConnection()` - it does not open any connections, just sets some internal member variables of the `HTTPConnection` class. So, starting the timer before or after `http.client.HTTPConnection()` shouldn't make a difference.
`conn.connect()` is what makes the connection. And the timer should be started before it. Why would not the followng test the right thing?
* sta
...
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2101815542)
I don't see this discussion as a blocker.
I just want to get to the bottom of it. Checked the source code of `http.client.HTTPConnection()` - it does not open any connections, just sets some internal member variables of the `HTTPConnection` class. So, starting the timer before or after `http.client.HTTPConnection()` shouldn't make a difference.
`conn.connect()` is what makes the connection. And the timer should be started before it. Why would not the followng test the right thing?
* sta
...
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2099792264)
Still this there may be a case for this change as it is a private function and the input well covered by tests. `Assume()` will abort on failure in `BUILD_FOR_FUZZING` configurations in release.
WIP: Changing this gave a 1-2% difference in benchmarks.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2099792264)
Still this there may be a case for this change as it is a private function and the input well covered by tests. `Assume()` will abort on failure in `BUILD_FOR_FUZZING` configurations in release.
WIP: Changing this gave a 1-2% difference in benchmarks.
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2101870685)
nit: Might be appropriate to rename the file to src/bench/obfuscation.cpp?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2101870685)
nit: Might be appropriate to rename the file to src/bench/obfuscation.cpp?