📝 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?
💬 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_r2101952111)
In 52583d3269cc747ff58b6d0b8c213a1ac521aeb3 "wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()": would be nice to have to test that illustrates a case where this matters
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101952111)
In 52583d3269cc747ff58b6d0b8c213a1ac521aeb3 "wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()": would be nice to have to test that illustrates a case where this matters
💬 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_r2101947190)
In c61c87a453c1e5c1ad2f7d9c5b0e04c2d6df87ec "wallet, rpc: Push the normalized parent descriptor": typo in the commit message "prividing".
I'm a bit surprised such a change doesn't break a test.
Would also be good to explain why this needs to happen.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101947190)
In c61c87a453c1e5c1ad2f7d9c5b0e04c2d6df87ec "wallet, rpc: Push the normalized parent descriptor": typo in the commit message "prividing".
I'm a bit surprised such a change doesn't break a test.
Would also be good to explain why this needs to happen.
💬 fanquake commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2900376297)
> (please note that the src/util/subprocess.h code is C++11 compatible).
It seems a bit odd that we've imported a dependency that is pinned to an older C++ version than what we want to use, especially when we are trying to upstream changes to it. This being a nuisance has come up multiple times already (also here: https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844382194). What is the longer term plan here?
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2900376297)
> (please note that the src/util/subprocess.h code is C++11 compatible).
It seems a bit odd that we've imported a dependency that is pinned to an older C++ version than what we want to use, especially when we are trying to upstream changes to it. This being a nuisance has come up multiple times already (also here: https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-2844382194). What is the longer term plan here?
💬 fanquake commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900383024)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900383024)
Concept ACK
💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2900383285)
> It would be even better to use ftruncate on openbsd.
I haven't got anything OpenBSD handy to test on, but happy for someone to followup with these changes.
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2900383285)
> It would be even better to use ftruncate on openbsd.
I haven't got anything OpenBSD handy to test on, but happy for someone to followup with these changes.