👍 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?
💬 hodlinator commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2101867408)
> do you see an explanation in the assembly why it would be faster?
Didn't check the assembly yet, do you mind sharing your workflow for that? I was using a combination of `objdump` with a bunch of flags + searching in `less` for the consteval hex stuff ~9 months ago but suspect `Obfuscation`-methods get heavily inlined?
Re-measured with Clang to make sure I wasn't running the wrong build, but can still reproduce the result:
At tip of PR 989537ff4026d7c3fa5ba99701e0a4b134d950f7:
|
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2101867408)
> do you see an explanation in the assembly why it would be faster?
Didn't check the assembly yet, do you mind sharing your workflow for that? I was using a combination of `objdump` with a bunch of flags + searching in `less` for the consteval hex stuff ~9 months ago but suspect `Obfuscation`-methods get heavily inlined?
Re-measured with Clang to make sure I wasn't running the wrong build, but can still reproduce the result:
At tip of PR 989537ff4026d7c3fa5ba99701e0a4b134d950f7:
|
...
👍 TheCharlatan approved a pull request: "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates"
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2860264039)
Re-ACK 843f7860298081d0a8d12294d23b962f5586a862
(https://github.com/bitcoin/bitcoin/pull/30479#pullrequestreview-2860264039)
Re-ACK 843f7860298081d0a8d12294d23b962f5586a862
👋 dergoegge's pull request is ready for review: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
(https://github.com/bitcoin/bitcoin/pull/32581)
💬 l0rinc commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900360861)
> This is because ASan is only aware of the memory chunks allocated by PoolResource but not the individual "sub-chunks" within
What's the depper explanation for this? Can we refactor the code or patch the sanitizers instead of annotating our code in places where we already know the code is problematic? Don't have a lot of experience in this area, I might be off, was just wondering if there's a more general solution.
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900360861)
> This is because ASan is only aware of the memory chunks allocated by PoolResource but not the individual "sub-chunks" within
What's the depper explanation for this? Can we refactor the code or patch the sanitizers instead of annotating our code in places where we already know the code is problematic? Don't have a lot of experience in this area, I might be off, was just wondering if there's a more general solution.
🤔 Sjors reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-2860239353)
Some initial observations on the early commits inline...
> Those are primarily bug fixes for existing issues that came up during testing.
It's a bit hard to follow, so I would suggest splitting (some of) them out.
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-2860239353)
Some initial observations on the early commits inline...
> Those are primarily bug fixes for existing issues that came up during testing.
It's a bit hard to follow, so I would suggest splitting (some of) them out.
💬 Sjors commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101930556)
In 129fdf7b9b7f52dc6e41f3562fd9d8601f75792c "wallet: Track whether a locked coin is persisted": this seems a bit cryptic. At the interface level we have `lockCoin()` which has a `write_to_db` argument, which determines whether the function here is called with a `batch`. And then here we have to translate that again to the `persist` bool of `LoadLockedCoin`.
Maybe LockCoin could have an explicit `persist` bool and batch as the third optional argument?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101930556)
In 129fdf7b9b7f52dc6e41f3562fd9d8601f75792c "wallet: Track whether a locked coin is persisted": this seems a bit cryptic. At the interface level we have `lockCoin()` which has a `write_to_db` argument, which determines whether the function here is called with a `batch`. And then here we have to translate that again to the `persist` bool of `LoadLockedCoin`.
Maybe LockCoin could have an explicit `persist` bool and batch as the third optional argument?