💬 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.
👍 TheCharlatan approved a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2860315670)
Re-ACK fd290730f530a8b76a9607392f49830697cdd7c5
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2860315670)
Re-ACK fd290730f530a8b76a9607392f49830697cdd7c5
🚀 fanquake merged a pull request: "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`"
(https://github.com/bitcoin/bitcoin/pull/32551)
(https://github.com/bitcoin/bitcoin/pull/32551)
🤔 luke-jr reviewed a pull request: "rest: fetch spent transaction outputs by blockhash"
(https://github.com/bitcoin/bitcoin/pull/32540#pullrequestreview-2860390650)
Not sure we should export a custom binary format for undo data like this. At least there should be a JSON option?
(https://github.com/bitcoin/bitcoin/pull/32540#pullrequestreview-2860390650)
Not sure we should export a custom binary format for undo data like this. At least there should be a JSON option?
💬 luke-jr commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2102026426)
nit: braces required
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2102026426)
nit: braces required
💬 fanquake commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2900447124)
Only 800b7cc42ca63f2a6b245a4d327c7092289da6e1 needs backporting here.
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2900447124)
Only 800b7cc42ca63f2a6b245a4d327c7092289da6e1 needs backporting here.
💬 l0rinc commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2900452144)
Another one in https://github.com/bitcoin/bitcoin/pull/32404/checks?check_run_id=42637298331 - retriggering the build makes it pass.
<details>
<summary>CI logs</summary>
```bash
[13:30:22.121] node0 2025-05-21T17:30:07.606048Z [msghand] [logging/timer.h:58] [Log] [lock] Enter: lock contention connman.m_nodes_mutex, ./net.h:1648 started
[13:30:22.121] node0 2025-05-21T17:30:07.606205Z [msghand] [logging/timer.h:58] [Log] [lock] Enter: lock contention connman.m_nodes_mutex, ./net.h:1648 complete
...
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2900452144)
Another one in https://github.com/bitcoin/bitcoin/pull/32404/checks?check_run_id=42637298331 - retriggering the build makes it pass.
<details>
<summary>CI logs</summary>
```bash
[13:30:22.121] node0 2025-05-21T17:30:07.606048Z [msghand] [logging/timer.h:58] [Log] [lock] Enter: lock contention connman.m_nodes_mutex, ./net.h:1648 started
[13:30:22.121] node0 2025-05-21T17:30:07.606205Z [msghand] [logging/timer.h:58] [Log] [lock] Enter: lock contention connman.m_nodes_mutex, ./net.h:1648 complete
...
💬 dergoegge commented on pull request "allocators: Apply manual ASan poisoning to `PoolResource`":
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900452384)
> What's the depper explanation for this?
At a high level `PoolResource` allocates larger chunks of memory (by default 262144 bytes per chunk) and allows for smaller memory section within those chunks to be allocated (in a way that is optimized for node based containers, e.g. `std::unordered_map`).
ASan instruments calls to e.g. `malloc`/`free` and `new`/`delete` to detect UaF and other issues, so for `PoolResource` it'll be aware of the allocations of the larger chunks. However, it does n
...
(https://github.com/bitcoin/bitcoin/pull/32581#issuecomment-2900452384)
> What's the depper explanation for this?
At a high level `PoolResource` allocates larger chunks of memory (by default 262144 bytes per chunk) and allows for smaller memory section within those chunks to be allocated (in a way that is optimized for node based containers, e.g. `std::unordered_map`).
ASan instruments calls to e.g. `malloc`/`free` and `new`/`delete` to detect UaF and other issues, so for `PoolResource` it'll be aware of the allocations of the larger chunks. However, it does n
...