Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
6f7052a7b96f058568af9aed2f014ae7a25e0f68 threading: semaphore: move CountingSemaphoreGrant to its own header (Cory Fields)
fd1546989293b110ad8d86d71f362a11dab3611c threading: semaphore: remove temporary convenience types (Cory Fields)
1f89e2a49a2170a57b14d993f181f29233b7d250 scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones (Cory Fields)
f21365c4fc7f6f45194f5b725192f0054e2daf13 threading: replace CountingSemaphore with std::counting_semaphore (Cory Fields)
1acacfbad780f95d1596010ba446dd9ea268fa10 threading: make CountingSemaphore/CountingSemaphoreGrant template types (Cory Fields)
e6ce5f9e78741ef7f88a8ad237f4b772da921dc3 scripted-diff: rename CSemaphore and CSemaphoreGrant (Cory Fields)
793166d3810ef3c08cc55c16a17d6d77ae6fabb5 wallet: change the write semaphore to a BinarySemaphore (Cory Fields)
6790ad27f1570926cef81ef097edaa8b8e70b270 scripted-diff: rename CSemaphoreGrant and CSemaphore for net (Cory Fields)
d870bc94519a68a861bb0ceca19f96c6ba22fbd7 threading: add temporary semaphore aliases (Cory Fields)
7b816c4e00e286a6dcdf0d9e09c710e1d745a0db threading: rename CSemaphore methods to match std::semaphore (Cory Fields)
Pull request description:
This is relatively simple, but done in a bunch of commits to enable scripted diffs.
I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)
This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSemaphoreGrant` and `BinarySemaphoreGrant` to match. Those have been moved out of `sync.h` to their own file.
ACKs for top commit:
purpleKarrot:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
achow101:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
TheCharlatan:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68
hebasto:
ACK 6f7052a7b96f058568af9aed2f014ae7a25e0f68, I have reviewed the code and it looks OK.
Tree-SHA512: 5975d13aa21739174e3a22c544620ae3f36345f172b51612346d3b7baf0a07c39ef6fd54f647c87878c21a67951b347a5d4a5f90e897f3f6c0db360a3779d0df
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
`CConnman::AlreadyConnectedToAddress()` searches the existent nodes by
address or by address-and-port:
```cpp
FindNode(static_cast<CNetAddr>(addr)) || FindNode(addr.ToStringAddrPort())
```
but:
* if there is a match by just the address, then the address-and-port
search will not be evaluated and the whole condition will be `true`
* if the there is no node with the same address, then the second search
by address-and-port will not find a match either.
The search by address-and-port is comparing against `CNode::m_addr_name`
which could be a hostname, e.g. `"node.foobar.com:8333"`, but
`addr.ToStringAddrPort()` is always going to be numeric.
6869fb417096b43ba7f74bf767ca3e41b9894899 net: Block v2->v1 transport downgrade if !CConnman::fNetworkActive (Hodlinator)
Pull request description:
We might have just set `CNode::fDisconnect` in the first loop because of `!CConnman::fNetworkActive`.
Attempting to reconnect using v1 transport just because `fNetworkActive` was set to `false` at the "right" stage in the v2 handshake does not make sense.
Issue [discovered](https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1930908304) by davidgumberg.
ACKs for top commit:
davidgumberg:
Tested and Reviewed ACK 6869fb417096b43ba7f7
mabu44:
ACK 6869fb417096b43ba7f74bf767ca3e41b9894899
stratospher:
ACK 6869fb4. I've reviewed the code but don't have strong preference for this branch vs master since only functional change is just a single log not being printed in a low probability scenario (we happen to be attempting v2 connection when P2P network activity is being turned off).
vasild:
ACK 6869fb417096b43ba7f74bf767ca3e41b9894899
Tree-SHA512: 54f596e54c5a6546f2c3fec2609aa8d10dec3adcf1001ca16666d8b374b8d79d64397f46c90d9b3915b4e91a5041b6ced3044fd2a5b4fb4aa7282eb51f61296a
eb0724f0dee307d6d14e47ebd3077b7ffd50f507 doc: banman: reference past vuln due to unbounded banlist (Antoine Poinsot)
ad616b6c013e69221f61b695c4ae09a3471c3f7c doc: net: mention past vulnerability as rationale to limit incoming message size (Antoine Poinsot)
4489117c3f6720ef92a328d3462cec8c0f466ae5 doc: txrequest: point to past censorship vulnerability in tx re-request handling (Antoine Poinsot)
68ac9542c451c9088c59a3ec6124d87cfd3382a3 doc: net_proc: reference past DoS vulnerability in orphan processing (Antoine Poinsot)
c02d9f6dd53989f41375f13a2d39270fa5d58a04 doc: net_proc: reference past defect regarding invalid GETDATA types (Antoine Poinsot)
5e3d9f21df21a822dc210d73a000faba084e6067 doc: validation: add a reference to historical header spam vulnerability (Antoine Poinsot)
Pull request description:
It is useful when reading code to have context about why it is written or behaves the way it does. Some instances in this PR may seem obvious but i think nonetheless offer important context to anyone willing to change (or review a change to) this code.
ACKs for top commit:
ryanofsky:
Code review ACK eb0724f0dee307d6d14e47ebd3077b7ffd50f507. No changes since last review other than rebase
Tree-SHA512: 271902f45b8130d44153d793bc1096cd22b6ce05494e67c665a5bc45754e3fc72573d303ec8fc7db4098d473760282ddbf0c1cf316947539501dfd8d7d5b8828
We might have just set CNode::fDisconnect in the first loop because of being offline.
Also caches CConnman::fNetworkActive in case it's changed concurrently with our own thread.
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
a85e8c0e6158fad2408bda5cb1e36da707eb081b doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa17829c3f8ae4da739f526531c91f3ed87 doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a11b57cb5af0e8903b50927723fbb3fcd6 refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c3c65e47fc05545d9b9c919be945851d51 test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920ec98fc7d9f77abfd08fea17211b9ca7099 refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d05668922a28e4e2c78dab2d4737433cd52d6302 refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca53a05e7d4735a2207d1b200e1dcddc534 Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d4c1e7f310c6ba3b58373bc30679b491df Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19f863737518944950fc73f97d9c1399a46 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f433fc3f753a20833aebe54692cdfe5ed75 Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab3508064cd3135e1a356c884ae1269cda5250 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389917a8d744f1b1ada4556d3d4fe63c310e Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c70f7472d39e45d189df6c0cf6b676b761 Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899bc209921fb4bde02840359c3253663766 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66fd91c303d04004d861ecad183ff177823 Fix nonsensical -noseednode behavior (Ryan Ofsky)
Pull request description:
The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.
Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.
Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.
ACKs for top commit:
achow101:
ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
danielabrozzoni:
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
hodlinator:
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
Treat specifying -noseednode the same as not specifying any -seednode value,
instead of enabling the seed node timeout and log messages, and waiting longer
to add other seeds.
* `CConnman::CalculateKeyedNetGroup()` needs `CNetAddr`, not `CAddress`,
thus change its argument.
* Both callers of `CConnman::CreateNodeFromAcceptedSocket()` create a
dummy `CAddress` from `CService`, so use `CService` instead.
* `GetBindAddress()` only needs to return `CService`.
* `CNode::addrBind` only needs to be `CService`.
0f716f28896c6edfcd4e2a2b25c88f478a029c7b qa: cover PROTOCOL_ERROR variant in PCP unit tests (Antoine Poinsot)
fc700bb47fd8b6ac58f612b932aef0e361686cc3 test: Add tests for PCP and NATPMP implementations (laanwj)
caf952103317a7fa8bd2bceb35d4e8ace5968906 net: Use mockable steady clock in PCP implementation (laanwj)
03648321ecb704b69e47eed7e3df6a779aee8f11 util: Add mockable steady_clock (laanwj)
ab1d3ece026844e682676673b8a461964a5b3ce4 net: Add optional length checking to CService::SetSockAddr (laanwj)
Pull request description:
Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.
Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
Includes "net: Add optional length checking to CService::SetSockAddr" from #31014 as a prerequisite.
ACKs for top commit:
darosior:
re-ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b
i-am-yuvi:
Concept ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b
achow101:
ACK 0f716f28896c6edfcd4e2a2b25c88f478a029c7b
Tree-SHA512: 6f91b24e6fe46a3fded7a13972efd77c98e6ef235f8898e4ae44068c5df32d1cdabb22cb66c351b338dc98cb2073b624e43607a28107f4999302bfbe7a138229
551a09486c495e1a3cfc296eafdf95e914856bff net: Switch to DisconnectMsg in CConnman (Hodlinator)
bbac17608d1ad3f8af5b32efad5d573c70989361 net: Bring back log message when resetting socket (Hodlinator)
04b848e4827f502d0784c5975bc8e652fc459cc8 net: Specify context in disconnecting log message (Hodlinator)
0c4954ac7d9676774434e5779bb5fd88e789bbb6 net_processing: Add missing use of DisconnectMsg (Hodlinator)
Pull request description:
- Add missing calls to `DisconnectMsg()` - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
- Specify context when stopping nodes - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890780754
- Bring back log message when resetting socket in case new entrypoints are added - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890795074
- Use `DisconnectMsg()` in `CConnman` as well - https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791797716
ACKs for top commit:
Sjors:
re-utACK 551a09486c495e1a3cfc296eafdf95e914856bff
l0rinc:
utACK 551a09486c495e1a3cfc296eafdf95e914856bff
davidgumberg:
Tested and Review ACK 551a09486c
achow101:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
danielabrozzoni:
ACK 551a09486c495e1a3cfc296eafdf95e914856bff
Tree-SHA512: 95ab8e7436e20ca3abc949ea09697facb6fbeb19981ddc7e0bf294e7ec914e72cbf836c21184a2a887f04cb264f26daf5b0cbcbebc9db633a7b1672b4e488063
In almost all cases (the only exception is `getifaddrs`), we know the
size of the data passed into SetSockAddr, so we can check this to be
what is expected.
06443b8f28bcec4315cec262eb03343dee5465a6 net: clarify if we ever sent or received from peer (Sjors Provoost)
1d01ad4d73e0eaae36c31153884e3d394ebc66b7 net: add LogIP() helper, use in net_processing (Sjors Provoost)
937ef9eb408e377cde4cab4dfcd27120afdedf1b net_processing: use CNode::DisconnectMsg helper (Sjors Provoost)
ad224429f823a66b431401d44bae21ed254a97e1 net: additional disconnection logging (Sjors Provoost)
Pull request description:
While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful.
All cases where we disconnect now come with a log message that has the word `disconnecting`:
* all calls to `CloseSocketDisconnect()` log `disconnecting peer=…`
* wherever we set `pnode->fDisconnect = true;`
* for all `InactivityCheck` cases (which in turn sets `fDisconnect`)
* replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…`
A few exceptions are listed here: https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so.
This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though.
The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use.
Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`.
ACKs for top commit:
davidgumberg:
reACK 06443b8f28
vasild:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
danielabrozzoni:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
hodlinator:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
4120c7543ee32efed7396d7153411ecbbd588ad3 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 #18533, #18937, #24078, #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 4120c7543ee32efed7396d7153411ecbbd588ad3 🛒
fjahr:
Code review ACK 4120c7543ee32efed7396d7153411ecbbd588ad3
rkrux:
tACK 4120c7543ee32efed7396d7153411ecbbd588ad3
Tree-SHA512: 7b4dd30136392a145da95d2f3ba181c18c155ba6f3158e49e622d76811c6a45ef9b5c7539a979a04d8404faf18bb27f11457aa436d4e2998ece3deb2c9e59748
0de3e96e333090548a43e5e870c4cb8941d6baf1 tracing: use bitcoind pid in bcc tracing examples (0xb10c)
411c6cfc6c2e488e407f057b646730e63806ed8a tracing: only prepare tracepoint args if attached (0xb10c)
d524c1ec06643208c189089089e84f6e1cd0abad 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](1596a09b5d/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 0de3e96e333090548a43e5e870c4cb8941d6baf1
laanwj:
re-ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
jb55:
utACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
vasild:
ACK 0de3e96e333090548a43e5e870c4cb8941d6baf1
Tree-SHA512: 0e5e0dc5e0353beaf5c446e4be03d447e64228b1be71ee9972fde1d6fac3fac71a9d73c6ce4fa68975f87db2b2bf6eee2009921a2a145e24d83a475d007a559b
Add a method CNetMessage::GetMemoryUsage and use this for accounting of
the size of the process receive queue instead of the raw message size.
This ensures that allocation and deserialization overhead is taken into
account.
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.
33381ea530ad79ac1e04c37f5707e93d3e0509ca scripted-diff: Modernize nLocalServices to m_local_services (Fabian Jahr)
Pull request description:
The type of the `nLocalServices` variable was changed to `std::atomic<ServiceFlags>` in #30807 and I suggested the variable name to get updated with a scripted diff along with it. It wasn't included in the PR but I am still suggesting to do it as a follow-up since I had already prepared the commit.
ACKs for top commit:
sipa:
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
achow101:
ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
furszy:
utACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
jonatack:
ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
theStack:
ACK 33381ea530ad79ac1e04c37f5707e93d3e0509ca
Tree-SHA512: 407ea9eac694f079aa5b5c1611b5874d7a0897ba6bc3aa0570be94afe1bf3a826657b6890b6597c03c063e95b9dc868f0bdfbfc41e77ec7e06f5b045bf065c71
e4e3b44e9cc7227b3ad765397c884999f57bac2e net: call `Select` with reachable networks in `ThreadOpenConnections` (brunoerg)
829becd990b504a2e8a57fa8a6ff6ac6ae8ff900 addrman: change `Select` to support multiple networks (brunoerg)
f698636ec86c004ab331994559c163b7319e6423 net: add `All()` in `ReachableNets` (brunoerg)
Pull request description:
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.
I did an experiment of calling `Select` without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).
ACKs for top commit:
achow101:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
vasild:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
naumenkogs:
ACK e4e3b44e9cc7227b3ad765397c884999f57bac2e
Tree-SHA512: e8466b72b85bbc2ad8bfb14471eb27d2c50d4e84218f5ede2c15a6fa3653af61b488cde492dbd398f7502bd847e95bfee1abb7e01092daba2236d3ce3d6d2268
