🚀 fanquake merged a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593)
(https://github.com/bitcoin/bitcoin/pull/26593)
💬 l0rinc commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1836449767)
I thought of that, but not sure how to make it work, I'm getting:
> note: non-constexpr function 'strchr' cannot be used in a constant expression
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1836449767)
I thought of that, but not sure how to make it work, I'm getting:
> note: non-constexpr function 'strchr' cannot be used in a constant expression
🚀 fanquake merged a pull request: "ci: make ctest stop on failure"
(https://github.com/bitcoin/bitcoin/pull/31257)
(https://github.com/bitcoin/bitcoin/pull/31257)
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2467876464)
Thanks for the comments @vasild. I plan to address your comments soon, when I rebase on https://github.com/bitcoin/bitcoin/pull/26593.
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2467876464)
Thanks for the comments @vasild. I plan to address your comments soon, when I rebase on https://github.com/bitcoin/bitcoin/pull/26593.
🚀 fanquake merged a pull request: "scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp}"
(https://github.com/bitcoin/bitcoin/pull/31163)
(https://github.com/bitcoin/bitcoin/pull/31163)
🚀 fanquake merged a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264)
(https://github.com/bitcoin/bitcoin/pull/31264)
💬 willcl-ark commented on pull request "doc: Fixup bitcoin-wallet manpage chain selection args":
(https://github.com/bitcoin/bitcoin/pull/31264#discussion_r1836491398)
We generally address these as and when files are touched (and folks remember).
(https://github.com/bitcoin/bitcoin/pull/31264#discussion_r1836491398)
We generally address these as and when files are touched (and folks remember).
💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2467888419)
I ran a `reindex-chainstate` until 860k blocks, before and after this change, 2 runs per commit:
<details>
<summary>benchmark</summary>
```bash
hyperfine \
--runs 2 \
--export-json /mnt/my_storage/reindex-chainstate-obfuscation-overhead.json \
--parameter-list COMMIT 866f4fa521f6932162570d6531055cc007e3d0cd,3efc72ff7cbdfb788b23bf4346e29ba99362c120,08db1794647c37f966c525411f931a4a0f6b6119 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build -
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2467888419)
I ran a `reindex-chainstate` until 860k blocks, before and after this change, 2 runs per commit:
<details>
<summary>benchmark</summary>
```bash
hyperfine \
--runs 2 \
--export-json /mnt/my_storage/reindex-chainstate-obfuscation-overhead.json \
--parameter-list COMMIT 866f4fa521f6932162570d6531055cc007e3d0cd,3efc72ff7cbdfb788b23bf4346e29ba99362c120,08db1794647c37f966c525411f931a4a0f6b6119 \
--prepare 'git checkout {COMMIT} && git clean -fxd && git reset --hard && cmake -B build -
...
🤔 Abdulkbk reviewed a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2427016825)
ACK 83fab3212c91d91fc5502f940c901a07772ff747
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2427016825)
ACK 83fab3212c91d91fc5502f940c901a07772ff747
📝 purpleKarrot opened a pull request: "cmake: add optional source files to bitcoin_crypto directly"
(https://github.com/bitcoin/bitcoin/pull/31268)
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.
(https://github.com/bitcoin/bitcoin/pull/31268)
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.
👍 Abdulkbk approved a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259#pullrequestreview-2427038611)
ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29
(https://github.com/bitcoin/bitcoin/pull/31259#pullrequestreview-2427038611)
ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29
🚀 fanquake merged a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259)
(https://github.com/bitcoin/bitcoin/pull/31259)
💬 willcl-ark commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1836522285)
I think you have a copy-paste error here. `x86` should be `arm`?
(https://github.com/bitcoin/bitcoin/pull/31268#discussion_r1836522285)
I think you have a copy-paste error here. `x86` should be `arm`?
💬 BrandonOdiwuor commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-2467950878)
@laanwj I updated the PR to include the suggestions
(https://github.com/bitcoin/bitcoin/pull/29457#issuecomment-2467950878)
@laanwj I updated the PR to include the suggestions
💬 brunoerg commented on pull request "fuzz: fix bad alloc in connman target":
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836536074)
> Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.
I agree. I can address it.
> so in current master nothing but 23 or 0 should ever be passed.
So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for `max_pct`. Sounds good?
(https://github.com/bitcoin/bitcoin/pull/31235#discussion_r1836536074)
> Yes, Assume() is some middle ground between assert() and the softer approach. Assume() + cap in release builds looks reasonable to me too.
I agree. I can address it.
> so in current master nothing but 23 or 0 should ever be passed.
So, we could adjust both addrman and connman fuzz targets to get 23 or 0 to use for `max_pct`. Sounds good?
💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836539245)
Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can't.
Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836539245)
Not sure about silently ignoring those. The goal of the script is to generate the manpages of the releases, and fail if it can't.
Allowing third parties to generate their own manpages can be implemented behind an option, or not at all.
💬 maflcko commented on pull request "doc: Fix gen-manpages to check build options":
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836536609)
Missing `HAVE_SYSTEM`?
(https://github.com/bitcoin/bitcoin/pull/29457#discussion_r1836536609)
Missing `HAVE_SYSTEM`?
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2468000391)
Less overhead for everyone not hooking into the tracepoints :partying_face:. Now that this is merged, here are a few ideas I had for future tracepoint interface work. Noting them for prosperity.
1. We _could_ internalize the relevant macro parts of systemtap's `sys/sdt.h` for the Linux tracepoints. This would allow us to drop the external dependency on systemtap, as we don't use 99% of it. Some prior commentary on this can be found here: https://github.com/hebasto/bitcoin/pull/162#issuecomment
...
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2468000391)
Less overhead for everyone not hooking into the tracepoints :partying_face:. Now that this is merged, here are a few ideas I had for future tracepoint interface work. Noting them for prosperity.
1. We _could_ internalize the relevant macro parts of systemtap's `sys/sdt.h` for the Linux tracepoints. This would allow us to drop the external dependency on systemtap, as we don't use 99% of it. Some prior commentary on this can be found here: https://github.com/hebasto/bitcoin/pull/162#issuecomment
...
🤔 rkrux reviewed a pull request: "tests: add functional test for miniscript decaying multisig"
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2427112510)
@mjdietzx I will review this PR again soon as I want to see a decaying multi-sig using miniscript example checked in. Would you be willing to rebase this over master to get the CMake build changes? The other option is for me (and other reviewers) to use the older build commands to test this PR locally, which is fine as well but at the cost of a much longer build time and having two building tools in my local.
Also, it would be nice to ensure this works with the latest build system though there
...
(https://github.com/bitcoin/bitcoin/pull/29156#pullrequestreview-2427112510)
@mjdietzx I will review this PR again soon as I want to see a decaying multi-sig using miniscript example checked in. Would you be willing to rebase this over master to get the CMake build changes? The other option is for me (and other reviewers) to use the older build commands to test this PR locally, which is fine as well but at the cost of a much longer build time and having two building tools in my local.
Also, it would be nice to ensure this works with the latest build system though there
...
👍 theStack approved a pull request: "test: report detailed msg during utf8 response decoding error"
(https://github.com/bitcoin/bitcoin/pull/31251#pullrequestreview-2427108686)
Tested ACK d65918c5da52c7d5035b4151dee9ffb2e94d4761
Makes sense to provide more detailed information in case of a decoding error.
master:
```
...
responsedata = http_response.read().decode('utf8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 42: invalid continuation byte
...
```
PR:
```
...
test_framework.authproxy.JSONRPCException: Cannot decode response in utf8 format, content: b'{"jsonrpc":"2.0","result":{"wallet_name":"\xc3(","backup_path":"/tmp/
...
(https://github.com/bitcoin/bitcoin/pull/31251#pullrequestreview-2427108686)
Tested ACK d65918c5da52c7d5035b4151dee9ffb2e94d4761
Makes sense to provide more detailed information in case of a decoding error.
master:
```
...
responsedata = http_response.read().decode('utf8')
UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position 42: invalid continuation byte
...
```
PR:
```
...
test_framework.authproxy.JSONRPCException: Cannot decode response in utf8 format, content: b'{"jsonrpc":"2.0","result":{"wallet_name":"\xc3(","backup_path":"/tmp/
...