This commit does not change behavior because negation of -signetseednode and
-signetchallenge parameters has been disallowed since these were introduced in
#18267, so calling IsArgSet() is equivalent to checking if GetArgs() returns a
non-empty list.
This commit does not change behavior, it just drops unnecessary IsArgSet()
calls for -debug, -loglevel, and -vbparams options. The calls are unnecessary
because GetArgs() already returns empty arrays if these arguments are not
specified.
Treat specifying -noexternalip the same as not specifying -externalip, instead
of causing it to soft-set the -discover default to false.
Before this change, was -noexternalip basically an undocumented synonym for
-nodiscover.
After this change, specifying -noexternalip just clears previously specifed
-externalip options, restoring default behavior as if they were not were
specified.
The previous -noexternalip behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -nodiscover option, and inconsistent with
behavior of other list options.
Treat specifying -noonlynet the same as not specifying -onlynet, instead of
marking all networks unreachable.
Before this change, specifying -noonlynet cleared list of reachable networks
and did not allow connecting to any network. It was basically an undocumented
synonym for -noconnect.
After this change, specifying -nononlynet just clears previously specifed
-onlynet options and allows connecting to all networks, restoring default
behavior as if no -onlynet options were specified.
Before this change, there was no way to restore default behavior once an
-onlynet option was specified. So for example, if a config file specifed
onlynet settings, they couldn't be reset on the command line without disabling
the entire config file.
The previous -noonlynet behavior wasn't neccessarily bad, but it was
undocumented, redundant with the -noconnect option, inconsistent with behavior
of other list options, and inconsistent with being able to use the command line
to selectively override config options. It was also probably unintended,
arising from use of the IsArgSet() method and its interaction with negated
options.
Treat specifying -norpcwallet the same as not specifying any -rpcwallet option,
instead of treating it like -rpcwallet=0 with 0 as the wallet name.
This restores previous behavior before 743077544b
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
Treat specifying -norpcwhitelist the same as not specifying -rpcwhitelist,
instead of behaving almost the same but flipping the default
-rpcwhitelistdefault value.
This is confusing because before this change if -norpcwhitelist was specified
it would block users from calling any RPC methods.
Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.
Also add code comment to clarify what intent of existing code is.
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.
7afeaa2469 test: `-debug=0` and `-debug=none` behave similarly to `-nodebug` (Daniela Brozzoni)
a8fedb36a7 logging: Ensure -debug=0/none behaves consistently with -nodebug (Daniela Brozzoni)
d39d521d86 test: `-nodebug` clears previously set debug options (Daniela Brozzoni)
Pull request description:
Previously, -nodebug cleared all prior -debug configurations in the command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging, even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none behave like -nodebug: they now clear previously set debug configurations but do not disable debugging for categories specified later.
See https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1930956563
ACKs for top commit:
hodlinator:
re-ACK 7afeaa2469
ryanofsky:
Code review ACK 7afeaa2469. Nicely implemented change with test and release notes, and I like how the test is implemented as the first commit.
maflcko:
review ACK 7afeaa2469👡
Tree-SHA512: c69b17ff10da6c88636bd01918366dd408832e70f2d0a7b951e9619089e89c39282db70398ba2542d3aa69a2fe6b6a0a01638b3225aff79d234d84d3067f2caa
* `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`.
ConsumeData() will always try to return a name as long as the requested size. It is more useful, and
closer to how `getsockname` would actually behave in reality, to return a random length name
instead.
This was hindering coverage in the PCP fuzz target as the addr len was set to the size of the
sockaddr_in struct and would exhaust all the provided data from the fuzzer.
Thanks to Marco Fleon for suggesting this.
Co-Authored-by: marcofleon <marleo23@proton.me>
The fuzz provider's `ConsumeData` may return less data than necessary
to fill the sockaddr struct and still return success. Fix this to avoid
the caller using uninitialized memory.
12fa9511b5 build: simplify dependency graph (Cory Fields)
c4e498300c build: avoid unnecessary dependencies on generated headers (Cory Fields)
Pull request description:
These changes speed up my build (default config/options/targets) by roughly 10%. I suspect the difference may be more significant in other build configs.
Before:
> $ time cmake --build build -j24
> real3m26.932s
After:
> $ time cmake --build build -j24
> real3m7.556s
Generally they allow for jobservers (either `make -jX` or `ninja`) to be better utilized. This can be verified using `top` while building and looking at the number of compiles running at any given time before/after these changes. Before, it's easy to observe periods of stalling when only one or two compiles are happening. After these changes, the compiler process count should mostly match the number of jobs given (`-jX`) until it falls off at the end.
---
The first commit sets [DEPENDS_EXPLICIT_ONLY](https://cmake.org/cmake/help/latest/command/add_custom_command.html#command:add_custom_command) for commands which generate our test header files. Without this option, `test_bitcoin`'s generated headers won't be built until all of its other dependencies have been built. This introduces a significant stall in the build, though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.
Example from a generated `build.ninja`:
Before:
> \# Custom command for src/test/data/base58_encode_decode.json.h
>
> build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a
After:
> \# Custom command for src/test/data/base58_encode_decode.json.h
>
> build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake
---
The second commit is more significant. It sets [CMAKE_OPTIMIZE_DEPENDENCIES](https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html) globally, which allows the objects of static libs to be built in parallel when one lib depends on the other. This can be set as a per-lib property, ~but I don't see any need for that as we don't currently have any edge-cases where this wouldn't be ok. If those should arise, we could always disable on a per-lib basis~.
Edit: turns out this triggers an [upstream bug](https://gitlab.kitware.com/cmake/cmake/-/issues/24058), which I guess can be considered an edge-case until fixed in CMake. I've added 2 per-lib opt-outs as a result.
Example:
Before:
> \# Link the static library src/libbitcoin_cli.a
>
> build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o || src/univalue/libunivalue.a
After:
> \# Link the static library src/libbitcoin_cli.a
>
> build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o
>
ACKs for top commit:
l0rinc:
utACK 12fa9511b5
hebasto:
ACK 12fa9511b5.
Tree-SHA512: f85f507e70cdc06acd07542161d9f9b8edf9ba866f08c8ef17aaaed770fa11530a27521c4413456d863463a6e77d4d6983fa623a64e17bbd602c2bc70aacc112
0f716f2889 qa: cover PROTOCOL_ERROR variant in PCP unit tests (Antoine Poinsot)
fc700bb47f test: Add tests for PCP and NATPMP implementations (laanwj)
caf9521033 net: Use mockable steady clock in PCP implementation (laanwj)
03648321ec util: Add mockable steady_clock (laanwj)
ab1d3ece02 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 0f716f2889
i-am-yuvi:
Concept ACK 0f716f2889
achow101:
ACK 0f716f2889
Tree-SHA512: 6f91b24e6fe46a3fded7a13972efd77c98e6ef235f8898e4ae44068c5df32d1cdabb22cb66c351b338dc98cb2073b624e43607a28107f4999302bfbe7a138229
2ffea09820 build: disable bitcoin-node if daemon is not built (Sjors Provoost)
Pull request description:
When building for fuzzing with multiprocess enabled, we were still trying to build `bitcoin-node`. This PR fixes that, by applying a similar check as for `bitcoin-gui`.
Before:
```
cmake -B build -DBUILD_FOR_FUZZING=ON -DWITH_MULTIPROCESS=ON
...
Configure summary
=================
Executables:
bitcoind ............................ OFF
bitcoin-node (multiprocess) ......... ON
bitcoin-qt (GUI) .................... OFF
bitcoin-gui (GUI, multiprocess) ..... OFF
...
cmake --build build
...
[ 84%] Built target bitcoin-node
```
After:
```
bitcoin-node (multiprocess) ......... OFF
```
And no `bitcoin-node` target gets built (not to be confused with `bitcoin_node`).
ACKs for top commit:
hebasto:
ACK 2ffea09820.
ryanofsky:
Code review ACK 2ffea09820
laanwj:
Code review ACK 2ffea09820
Tree-SHA512: bdb0b62049f77929d5c084bf98a076e9933de91eb30853ed89edd23cc81b3d4aec4cd57c9a9e21cf1d6930885f8c408dda830db6884b4e326c7fb348f1fbab4c
Previously, -nodebug cleared all prior -debug configurations in the
command line while allowing subsequent debug options to be applied.
However, -debug=0 and -debug=none completely disabled debugging,
even for categories specified afterward.
This commit ensures consistency by making -debug=0 and -debug=none
behave like -nodebug: they now clear previously set debug configurations
but do not disable debugging for categories specified later.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
9b7023d31a Fuzz HRP of bech32 as well (LÅ‘rinc)
c1a5d5c100 Split out bech32 separator char to header (LÅ‘rinc)
Pull request description:
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
> The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-126]. HRP validity may be further restricted by specific applications.
Since `bech32::Encode` rejects uppercase letters, we're actually generating values in the `[33-126] - ['A'-'Z']` range.
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
ACKs for top commit:
sipa:
ACK 9b7023d31a
achow101:
ACK 9b7023d31a
marcofleon:
Code review ACK 9b7023d31a. The separation into two targets and the new `GenerateRandomHRP` seem fine to me.
brunoerg:
code review ACK 9b7023d31a
Tree-SHA512: 22a261b8e7b5516e98f4e7990811954454595438a49a10191ed4ca42b5c71c5054fcc73f2d94e23b498ea833c7f1d5adb225f537ef1a24d15b428259450cdf98
LegacySPKM would determine whether it could provide any script data to a
transaction through the use of the CanProvide function. Instead of
partially reversing signing logic to figure out the output scripts of
solvable things, we use the same candidate set approach in
GetScriptPubKeys() and instead filter the candidate set first for
things that are ISMINE_NO, and second with CanProvide(). This should
give a more accurate solvables wallet.
InferDescriptors can sometimes make descriptors which are actually
invalid and cannot be parsed. Detect and skip such descriptors by doing
a Parse() check before adding the descriptor to the wallet.
Instead of (partially) trying to reverse IsMine() to get the output
scripts that a LegacySPKM would track, we can preserve it in migration
only code and utilize it to get an accurate set of output scripts.
This is accomplished by computing a set of output script candidates from
map(Crypted)Keys, mapScripts, and setWatchOnly. This candidate set is an
upper bound on the scripts tracked by the wallet. Then IsMine() is used
to filter to the exact output scripts that LegacySPKM would track.
By changing GetScriptPubKeys() this way, we can avoid complexities in
reversing IsMine() and get a more complete set of output scripts.
b448b01494 test: add a mocked Sock that allows inspecting what has been Send() to it (Vasil Dimov)
f1864148c4 test: put the generic parts from StaticContentsSock into a separate class (Vasil Dimov)
4b58d55878 test: move the implementation of StaticContentsSock to .cpp (Vasil Dimov)
Pull request description:
Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations.
Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the socket using `DynSock::Send()` can later be found in the send-pipe. For convenience there are also two methods to send and receive `CNetMessage`s.
---
This is used in https://github.com/bitcoin/bitcoin/pull/26812 (first two commits from that PR).
Extracting as a separate PR suggested here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037.
ACKs for top commit:
Sjors:
re-ACK b448b01494
jonatack:
re-ACK b448b01494
pinheadmz:
ACK b448b01494
Tree-SHA512: 4a36f038192ec4ef63366cbe1a38ae70e7e015630c9f7c44926b756b20ab8c08138acae41801f23b30f6629c7059c1f81e001806e86584ff1bf1fa5b44d9caec
386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes#21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f
fjahr:
Code review ACK 386eecff5f
glozow:
utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
e107bf78f9 [fuzz] TxOrphanage::SanityCheck accounting (glozow)
22dccea553 [fuzz] txorphan byte accounting (glozow)
982ce10178 add orphanage byte accounting to TxDownloadManagerImpl::CheckIsEmpty() (glozow)
c289217c01 [txorphanage] track the total number of announcements (glozow)
e5ea7daee0 [txorphanage] add per-peer weight accounting (glozow)
672c69c688 [refactor] change per-peer workset to info map within orphanage (glozow)
59cd0f0e09 [txorphanage] account for weight of orphans (glozow)
Pull request description:
Part of orphan resolution project, see #27463.
Definitions:
- **Announcement** is a unique pair (wtxid, nodeid). We can have multiple announcers for the same orphan since #31397.
- **Size** is the weight of an orphan. I'm calling it "size" and "bytes" because I think we can refine it in the future to be memusage or be otherwise more representative of the orphan's actual cost on our memory. However, I am open to naming changes.
This is part 1/2 of a project to also add limits on orphan size and count. However, this PR **does not change behavior**, just adds internal counters/tracking and a fuzzer. I will also open a second PR that adds behavior changes, which requires updating a lot of our tests and careful thinking about DoS.
ACKs for top commit:
instagibbs:
reACK e107bf78f9
marcofleon:
reACK e107bf78f9
sipa:
utACK e107bf78f9
Tree-SHA512: 855d725d5eb521d131e36dacc51990725e3ca7881beb13364d5ba72ab2202bbfd14ab83864b13b1b945a4ec5e17890458d0112270b891a41b1e27324a8545d72
No change for now, moving from map of NodeId->workset to
NodeId->PeerOrphanInfo struct that holds the workset.
In future commits, we will start tracking more things per-peer in the
orphanage.
Some sources might be generated, and while they likely do not contain
any translatable strings, this change generalizes the approach to
include generated sources in the translation process as well.
e3622a9692 tracing: document that peer addrs can be >68 chars (0xb10c)
b19b526758 tracing: log_p2p_connections.bt example (0xb10c)
caa5486574 tracing: connection closed tracepoint (0xb10c)
b2ad6ede95 tracing: add misbehaving conn tracepoint (0xb10c)
68c1ef4f19 tracing: add inbound connection eviction tracepoint (0xb10c)
4d61d52f43 tracing: add outbound connection tracepoint (0xb10c)
85b2603eec tracing: add inbound connection tracepoint (0xb10c)
Pull request description:
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor connection lifetimes, re-connection frequency by IP and netgroup, misbehavior, peer discouragement, and eviction and more. Together with the two existing P2P message tracepoints they allow for a good overview of local P2P network activity. Also sort-of addresses https://github.com/bitcoin/bitcoin/pull/22006#discussion_r636775863.
I've been back and forth on which arguments to include. For example, `net:evicted_connection` could also include some of the eviction metrics like e.g. `last_block_time`, `min_ping_time`, ... but I've left them out for now. If wanted, this can be added here or in a follow-up. I've tried to minimize a potential performance impact by measuring executed instructions with `gdb` where possible (method described [here](https://github.com/bitcoin/bitcoin/pull/23724#issuecomment-996919963)). I don't think a few hundred extra instructions are too crucial, as connection opens/closes aren't too frequent (compared to e.g. P2P messages). Note: e.g. `CreateNodeFromAcceptedSocket()` usually executes between 80k and 90k instructions for each new inbound connection.
| tracepoint | instructions |
|----------------------------|--------------------------------------------------------|
| net:inbound_connection | 390 ins |
| net:outbound_connection | between 700 and 1000 ins |
| net:closed_connnection | 473 ins |
| net:evicted_inbound_connection | not measured; likely similar to net:closed_connnection |
| net:misbehaving_connection | not measured |
Also added a bpftrace (tested with v0.14.1) `log_p2p_connections.bt` example script that produces output similar to:
```
Attaching 6 probes...
Logging opened, closed, misbehaving, and evicted P2P connections
OUTBOUND conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, total_out=1
INBOUND conn from 127.0.0.1:45324: id=1, type=inbound, network=0, total_in=1
MISBEHAVING conn id=1, message='getdata message size = 50001'
CLOSED conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, established=1231006505
EVICTED conn to 127.0.0.1:45324: id=1, type=inbound, network=0, established=1612312312
```
ACKs for top commit:
laanwj:
re-ACK e3622a9692
vasild:
ACK e3622a9692
sipa:
utACK e3622a9692
Tree-SHA512: 1032dcac6fe0ced981715606f82c2db47016407d3accb8f216c978f010da9bc20453e24a167dcc95287f4783b48562ffb90f645bf230990e3df1b9b9a6d4e5d0
- The reserved weight of the coinbase transaction is an estimate and
may not reflect the exact value; it can be lower.
- It should be clear that `currentblockweight` includes the reserved coinbase transaction weight.
whereas `currentblocktx` does not account for the coinbase transaction count.
- Also clarify `m_last_block_num_txs` and `m_last_block_weight`
- This commit renamed coinbase_max_additional_weight to block_reserved_weight.
- Also clarify that the reservation is for block header, transaction count
and coinbase transaction.
