GitHub skip branch #70
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
….{h,cpp}
-BEGIN VERIFY SCRIPT-
sed -i s/COMMAND_SIZE/MESSAGE_TYPE_SIZE/g $(git grep -l COMMAND_SIZE)
sed -i s/pszCommand/msg_type/g $(git grep -l pszCommand)
sed -i s/pchCommand/m_msg_type/g $(git grep -l pchCommand)
sed -i s/GetCommand/GetMessageType/g ./src/net.cpp ./src/protocol.cpp ./src/protocol.h ./src/test/fuzz/protocol.cpp
sed -i s/IsCommandValid/IsMessageTypeValid/g $(git grep -l IsCommandValid)
sed -i "s/command/message type/g" ./src/protocol.h ./src/protocol.cpp
-END VERIFY SCRIPT-
This deduplicates the TRACEx macros by using systemtaps STAP_PROBEV[0] variadic macro instead of the DTrace compability DTRACE_PROBE[1] macros. Bitcoin Core never had DTrace tracepoints, so we don't need to use the drop-in replacement for it. As noted in pr25541[2], these macros aren't compatibile with DTrace on macOS anyway. This also renames the TRACEx macro to TRACEPOINT to clarify what the macro does: inserting a tracepoint vs tracing (logging) something. [0]: https://sourceware.org/git/?p=systemtap.git;a=blob;f=includes/sys/sdt.h;h=24d5e01c37805e55c36f7202e5d4e821b85167a1;hb=ecab2afea46099b4e7dfd551462689224afdbe3a#l407 [1]: https://sourceware.org/git/?p=systemtap.git;a=blob;f=includes/sys/sdt.h;h=24d5e01c37805e55c36f7202e5d4e821b85167a1;hb=ecab2afea46099b4e7dfd551462689224afdbe3a#l490 [2]: https://github.com/bitcoin/bitcoin/pull/25541/files#diff-553886c5f808e01e3452c7b21e879cc355da388ef7680bf310f6acb926d43266R30-R31 Co-authored-by: Martin Leitner-Ankerl <[email protected]>
Before this commit, we would always prepare tracepoint arguments regardless of the tracepoint being used or not. While we already made sure not to include expensive arguments in our tracepoints, this commit introduces gating to make sure the arguments are only prepared if the tracepoints are actually used. This is a win-win improvement to our tracing framework. For users not interested in tracing, the overhead is reduced to a cheap 'greater than 0' compare. As the semaphore-gating technique used here is available in bpftrace, bcc, and libbpf, users interested in tracing don't have to change their tracing scripts while profiting from potential future tracepoints passing slightly more expensive arguments. An example are mempool tracepoints that pass serialized transactions. We've avoided the serialization in the past as it was too expensive. Under the hood, the semaphore-gating works by placing a 2-byte semaphore in the '.probes' ELF section. The address of the semaphore is contained in the ELF note providing the tracepoint information (`readelf -n ./src/bitcoind | grep NT_STAPSDT`). Tracing toolkits like bpftrace, bcc, and libbpf increase the semaphore at the address upon attaching to the tracepoint. We only prepare the arguments and reach the tracepoint if the semaphore is greater than zero. The semaphore is decreased when detaching from the tracepoint. This also extends the "Adding a new tracepoint" documentation to include information about the semaphores and updated step-by-step instructions on how to add a new tracepoint.
BCC needs the PID of a bitcoind process to attach to the tracepoints (instead of the binary path used before) when the tracepoints have a semaphore. For reference, we already use the PID in our tracepoint interface tests. See 220a5a2.
This change generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former.
Issue: The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option. Correction: Added `blank=true` to the command to match the options described in the text. Explanation: The `blank=true` option is necessary to create a blank wallet. Including this option ensures the command matches the options specified in the text.
Co-authored-by: Martin Zumsande <[email protected]>
The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed.
Wastes less resources and notifies the developer faster when a failure occurs.
This pull request addresses a minor issues in the REST-interface.md documentation: Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax.
Sjors
force-pushed
the
github-skip-branch
branch
2 times, most recently
from
3d9e794
to
d55d4b4
Compare
…actually tracing 0de3e96 tracing: use bitcoind pid in bcc tracing examples (0xb10c) 411c6cf tracing: only prepare tracepoint args if attached (0xb10c) d524c1e tracing: dedup TRACE macros & rename to TRACEPOINT (0xb10c) Pull request description: Currently, if the tracepoints are compiled (e.g. in depends and release builds), we always prepare the tracepoint arguments regardless of the tracepoints being used or not. We made sure that the argument preparation is as cheap as possible, but we can almost completely eliminate any overhead for users not interested in the tracepoints (the vast majority), by gating the tracepoint argument preparation with an `if(something is attached to this tracepoint)`. To achieve this, we use the optional semaphore feature provided by SystemTap. The first commit simplifies and deduplicates our tracepoint macros from 13 TRACEx macros to a single TRACEPOINT macro. This makes them easier to use and also avoids more duplicate macro definitions in the second commit. The Linux tracing tools I'm aware of (bcc, bpftrace, libbpf, and systemtap) all support the semaphore gating feature. Thus, all existing tracepoints and their argument preparation is gated in the second commit. For details, please refer to the commit messages and the updated documentation in `doc/tracing.md`. Also adding unit tests that include all tracepoint macros to make sure there are no compiler problems with them (e.g. some varadiac extension not supported). Reviewers might want to check: - Do the tracepoints still work for you? Do the examples in `contrib/tracing/` run on your system (as bpftrace frequently breaks on every new version, please test master too if it should't work for you)? Do the CI interface tests still pass? - Is the new documentation clear? - The `TRACEPOINT_SEMAPHORE(event, context)` macros places global variables in our global namespace. Is this something we strictly want to avoid or maybe move to all `TRACEPOINT_SEMAPHORE`s to a separate .cpp file or even namespace? I like having the `TRACEPOINT_SEMAPHORE()` in same file as the `TRACEPOINT()`, but open for suggestion on alternative approaches. - Are newly added tracepoints in the unit tests visible when using `readelf -n build/src/test/test_bitcoin`? You can run the new unit tests with `./build/src/test/test_bitcoin --run_test=util_trace_tests* --log_level=all`. <details><summary>Two of the added unit tests demonstrate that we are only processing the tracepoint arguments when attached by having a test-failure condition in the tracepoint argument preparation. The following bpftrace script can be used to demonstrate that the tests do indeed fail when attached to the tracepoints.</summary> `fail_tests.bt`: ```c #!/usr/bin/env bpftrace usdt:./build/src/test/test_bitcoin:test:check_if_attached { printf("the 'check_if_attached' test should have failed\n"); } usdt:./build/src/test/test_bitcoin:test:expensive_section { printf("the 'expensive_section' test should have failed\n"); } ``` Run the unit tests with `./build/src/test/test_bitcoin` and start `bpftrace fail_tests.bt -p $(pidof test_bitcoin)` in a separate terminal. The unit tests should fail with: ``` Running 594 test cases... test/util_trace_tests.cpp(31): error: in "util_trace_tests/test_tracepoint_check_if_attached": check false has failed test/util_trace_tests.cpp(51): error: in "util_trace_tests/test_tracepoint_manual_tracepoint_active_check": check false has failed *** 2 failures are detected in the test module "Bitcoin Core Test Suite" ``` </details> These links might provide more contextual information for reviewers: - [How SystemTap Userspace Probes Work by eklitzke](https://eklitzke.org/how-sytemtap-userspace-probes-work) (actually an example on Bitcoin Core; mentions that with semaphores "the overhead for an untraced process is effectively zero.") - [libbpf comment on USDT semaphore handling](https://github.com/libbpf/libbpf/blob/1596a09b5de2a50ab8d44218fc29b6d42f886305/src/usdt.c#L83-L92) (can recommend the whole comment for background on how the tracepoints and tracing tools work together) - https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation#Semaphore_Handling ACKs for top commit: willcl-ark: utACK 0de3e96 laanwj: re-ACK 0de3e96 jb55: utACK 0de3e96 vasild: ACK 0de3e96 Tree-SHA512: 0e5e0dc5e0353beaf5c446e4be03d447e64228b1be71ee9972fde1d6fac3fac71a9d73c6ce4fa68975f87db2b2bf6eee2009921a2a145e24d83a475d007a559b
36a22e5 ci: make ctest stop on failure (furszy) Pull request description: Make `ctest` stops when the first failure happens. Wasting less resources and notifying the developer faster when a failure occurs. ACKs for top commit: maflcko: lgtm ACK 36a22e5 tdb3: code review and test ACK 36a22e5 Tree-SHA512: 3abdb330e76aa312f7a5432e3d447a654e6689fc56e067b8e4d07ed8d677fc92f836e603aab0b2f175a6c039a5d50e5fd1160d503164321c1af44ad902f09605
…rminology in protocol.{h,cpp}
4120c75 scripted-diff: get rid of remaining "command" terminology in protocol.{h,cpp} (Sebastian Falbesoner)
Pull request description:
The confusing "command" terminology for the 12-byte field in the (v1) p2p message header was replaced with the more proper term "message type" in other modules already years ago, see eg bitcoin#18533, bitcoin#18937, bitcoin#24078, bitcoin#24141. This PR does the same for the protocol.{h,cpp} module to complete the replacements. Note that "GetCommand" is a method name also used in the `ArgsManager` (there it makes much more sense), so the scripted-diff lists for this replacement the files explicitly, rather than using `$(git grep -l ...)`.
ACKs for top commit:
maflcko:
review ACK 4120c75 🛒
fjahr:
Code review ACK 4120c75
rkrux:
tACK 4120c75
Tree-SHA512: 7b4dd30136392a145da95d2f3ba181c18c155ba6f3158e49e622d76811c6a45ef9b5c7539a979a04d8404faf18bb27f11457aa436d4e2998ece3deb2c9e59748
…n args fa729ab doc: Fixup bitcoin-wallet manpage chain selection args (MarcoFalke) Pull request description: The sentence is missing `-testnet4` and `-chain`. Instead of duplicating the full list (and having to keep it in sync), just refer to them as `(test)chain selection arguments`. ACKs for top commit: willcl-ark: utACK fa729ab tdb3: Code Review ACK fa729ab rkrux: crACK fa729ab Tree-SHA512: e2cb6e2dd778a34e6c7e8ccde9794ab601e68bad68fe110f41cd73ac12ac3c5d0632fb59a48355f03ef0909f77ec5afd7ea50f301a998cb3ec76e115969f3e7e
…nterface.md 5e3b444 doc: Fix missing comma in JSON example in REST-interface.md (secp512k2) Pull request description: This pull request addresses a minor issues in the REST-interface.md documentation: Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax. ACKs for top commit: maflcko: lgtm ACK 5e3b444 Abdulkbk: ACK 5e3b444 Tree-SHA512: d2d479c8a991d3380d16b7b140a375a90dca0fce0a024a4b8ccf842d703398fde14ae972349f5fbd2e0ce26aa6cd6d07c0262d9c09ddc4c6c466527cfbe0e1f1
…proxy connections 99d9a09 test: clarify log messages when handling SOCKS5 proxy connections (Vasil Dimov) Pull request description: Clarify log messages when handling SOCKS5 proxy connections. Suggested in bitcoin#29420 (comment) ACKs for top commit: mzumsande: Code Review ACK 99d9a09 tdb3: code review ACK 99d9a09 Tree-SHA512: 06bc0e63fbc9fdd8144a161d65d02e6c99565960064e65782b9b4b2fdfdf18539a1cd9513e17a911eef1506525e411e8422b7b805ce4c2392fcca6620112e172
…signing-tutorial.md ec375de doc: Add missing 'blank=true' option in offline-signing-tutorial.md (secp512k2) Pull request description: Issue: The text mentions that the `createwallet` command should use the options `disable_private_keys=true, blank=true`, but the provided command only includes `disable_private_keys=true`, missing the `blank=true` option. Correction: Added `blank=true` to the command to match the options described in the text. Explanation: The `blank=true` option is necessary to create a blank wallet. Including this option ensures the command matches the options specified in the text. ACKs for top commit: fanquake: ACK ec375de Tree-SHA512: 8c145e3ef1598c5e13f2aa89e496f76bfe2fc6f47d5e740963acad18dd1f782655a822dc234862af8e5a08060ab7fe1039a3dcfa68455a9143fe2d849975927c
Fix typos in miniscript.h
faf2162 refactor: Drop deprecated space in operator""_mst (MarcoFalke) Pull request description: The space is deprecated according to https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators and compilers will start to warn about this. For example, GCC-15 should warn when compiling under C++23, according to https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#index-Wdeprecated-literal-operator and Clang-20 will do so by default, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-literal-operator. Fix it by removing the unused and deprecated space. Also, fix the iwyu include list, while touching the module. ACKs for top commit: TheCharlatan: ACK faf2162 l0rinc: ACK faf2162 Tree-SHA512: 888a7b57c91114e1f71b6278fa13783bde16a9b51f4df10ae4b6c7d62bf016d6c022128250e6962b459f66743ddeab774b4109064281654892ecdb5bc11b6be6
5a96767 depends, libevent: Do not install *.pc files and remove patches for them (Hennadii Stepanov) ffda355 cmake, refactor: Move `HAVE_EVHTTP_...` to `libevent` interface (Hennadii Stepanov) b619bdc cmake: Revamp `FindLibevent` module (Hennadii Stepanov) Pull request description: This PR generalizes the use of `find_package` / `pkg_check_modules`, prioritizing the former. Addresses bitcoin#30903 (comment): > We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely. Similar to bitcoin#30903. ACKs for top commit: fanquake: ACK 5a96767 Tree-SHA512: 181020c16ccd2821e718c73f264badcdc5e62980c4a8d9691e759efe2ea00da2326e26308d1dcfdeac01e9e27930428ecace9f36941deee951b751b138d7266c
726cbee doc: correct typos (fanquake) 9fdfb73 doc: fix typos (Afanti) Pull request description: Includes bitcoin#31253. Includes bitcoin#31239 (review). Fixes remaining lint output. ACKs for top commit: l0rinc: ACK 726cbee rkrux: crACK 726cbee tdb3: ACK 726cbee Tree-SHA512: 51978343f11fb5f0c6b824d92dbfc9999952373a9f790ab79ef8750f920f1c020c092ca874c9e39f478d12d85cdadcfd8c63dda0cbb02745bc55fda28d371e4c
…temultisig 83fab32 test: Add combinerawtransaction test to rpc_createmultisig (Ava Chow) Pull request description: The only coverage of combinerawtransaction is in a legacy wallet only test. So also use it in rpc_createmultisig so that this RPC remains tested after the legacy wallet is removed. Split from bitcoin#28710 ACKs for top commit: maflcko: re-ACK 83fab32 BrandonOdiwuor: Re-ACK 83fab32 Abdulkbk: ACK 83fab32 brunoerg: code review ACK 83fab32 rkrux: tACK 83fab32 Tree-SHA512: 383d88ff6c9b54337ed81c714026e527b0fed41d976959fd5c6863b49d0defa4ea13fdc3d984885c86a2b6380825cd66c17842cc31f20fbec4bc42d86aecbbfa
Consistent with Cirrus behavior introduced in e9bfbb5.
Sjors
force-pushed
the
github-skip-branch
branch
from
d55d4b4
to
e441070
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing bitcoin#30487