When the detailed peers list is requested, return the shortened services in the
-netinfo header in the same format as the "serv" column, instead of the full names
list in the report.
Checking for IsArgSet before calling GetArg while providing the args
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
However, ignoring the fallback value is fragile, because it would not be
sanitized.
Fix all issues by sanitizing the fallback value.
for consistency with the other CLI commands (-netinfo, -addrinfo, -generate).
This can be considered a bugfix because IsArgSet() returns whether an arg has
been set even if it has been negated. After this change, we no longer treat
-nogetinfo and -getinfo=0 the same as -getinfo and -getinfo=1, and instead as if
-getinfo was not specified.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
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 743077544b5420246ef29e0b708c90e3a8dfeeb6
from https://github.com/bitcoin/bitcoin/pull/18594, which inadvertently changed
it.
47f50c7af5572520fd986b313a63a44a76d3c859 doc: add bitcoin-qt man description (willcl-ark)
40b82e3ab0a1889ddb816ca66f80512bdba93ef6 doc: add bitcoin-util man description (willcl-ark)
a7bf80f3a2db17ca6cbbaf45c49fbd6011633fa5 doc: add bitcoin-tx man description (willcl-ark)
3f9a5168323070a18b6c8b7bffeafe1d2965f50b doc: add bitcoin-wallet man description (willcl-ark)
d8c0bb23ef83f207394a3f714f4498ab8abf88c1 doc: add bitcoin-cli man description (willcl-ark)
09abccfa7729b53d057c9c0a7836367d6cc0233d doc: add bitcoind man description (willcl-ark)
Pull request description:
Closes#29552
Add better descriptions to help string for all binaries. Use format which is correctly detected by help2man when generating manpages.
Examples:
Before:
After:
Demonstration using `bitcoin-cli` also highlights removal of inline usage explanations which were being incorrectly formatted by `help2man`. This results in the following changed format to `bitcoin-cli --help`:
ACKs for top commit:
achow101:
ACK 47f50c7af5572520fd986b313a63a44a76d3c859
tdb3:
re ACK 47f50c7af5572520fd986b313a63a44a76d3c859
rkrux:
tACK 47f50c7af5572520fd986b313a63a44a76d3c859
maflcko:
ACK 47f50c7af5572520fd986b313a63a44a76d3c859 📠
Tree-SHA512: 124a8877077b7d47758ea970949d472b2444e3ba65d2bfeb47ebbdb1f5f8d3bf0abe2c88714bb6c92ba0e36583f0b36aa6f016ea88b65f011c610096ea872182
87532fe55856efc063cf81244800da37a015ba75 netinfo: allow setting an outbound-only peer list (Jon Atack)
681ebcceca78caeb9731f97d339ef926432e29ef netinfo: rename and hoist max level constant to use in top-level help (Jon Atack)
e7d307ce8cf7d4eaa83152ff46bf7232c54412bf netinfo: clarify relaytxes and addr_relay_enabled help docs (Jon Atack)
eef2a9d4062c5e194b513d1b7296463709bc73fa netinfo: add peer services column (Jon Atack)
Pull request description:
Been using this since May 2023.
- add a peer services column (considered displaying the p2p_v2 flag as "p" or "2"; proposing "2" here for continuity with the "v" column, but "p" is fine for me as well)
- clarify in the help that "relaytxes" and "addr_relay_enabled" are from getpeerinfo
- hoist (and rename) the max level constant to use in top-level help, to avoid overlooking to update the top-level help if the value of the constant changes (as caught by Larry Ruane in review below)
- add an optional "outonly" (or "o") argument for an outbound-only peer list, as suggested by Vasil Dimov in his review below. Several people have requested this, to keep the output within screen limits when running netinfo as a live dashboard (i.e. with `watch`) on a node with many peers. While doing this, also permit passing "h" for the help in addition to "help".
ACKs for top commit:
achow101:
ACK 87532fe55856efc063cf81244800da37a015ba75
rkrux:
tACK 87532fe55856efc063cf81244800da37a015ba75
tdb3:
cr re ACK 87532fe55856efc063cf81244800da37a015ba75
brunoerg:
crACK 87532fe55856efc063cf81244800da37a015ba75
Tree-SHA512: 35b1b0de28dfecaad58bf5af194757a5e0f563553cf69ea4d76f2e1963f8d662717254df2549114c7bba4a041bf5282d5cb3fba8d436b2807f2a00560787d64c
by passing an additional argument of "outonly" or "o".
This has been requested in order to keep the output within screen limits when running -netinfo
as a live dashboard, i.e. with `watch`.
Also allow passing "h" in addition to "help" to see the help documentation.
a16917fb5981d1465ffd4c036586f8729e683b73 rpc, net: improve `mapped_as` doc for getrawaddrman/getpeerinfo (brunoerg)
bdad0243be80c4c72f8cefd1233fd7c057a1945b rpc, net: getrawaddrman "mapped_as" follow-ups (brunoerg)
Pull request description:
- Change `addrman` to reference to const since it isn't modified (https://github.com/bitcoin/bitcoin/pull/30062#discussion_r1612272793).
- Improve documentation of `mapped_as`/`source_mapped_as` in `getrawaddrman` RPC by mentioning that both fields will be only available if asmap flag is set. It is the same message for `mapped_as` field in `getpeerinfo`.
ACKs for top commit:
fjahr:
re-ACK a16917fb5981d1465ffd4c036586f8729e683b73
0xB10C:
re-ACK a16917fb5981d1465ffd4c036586f8729e683b73
laanwj:
re-ACK a16917fb5981d1465ffd4c036586f8729e683b73
Tree-SHA512: c66b2ee9d24da93d443be83f6ef3b2d39fd5bf3f73e2974574cad238ffb82035704cf4fbf1bac22a63734948e285e8e091c2884bb640202efdb473315e770233
Before, we did not explicity say that both fields
`{source_}mapped_as` (that are optional in getrawaddrman)
will be only available if the asmap config flag is set.
Co-authored-by: Jon Atack <jon@atack.com>
The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.
This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
c8e6771af002eaf15567794fcdc57fdb0e3fb140 test: restrict multiple CLI arguments (naiyoma)
8838c4f171f3c3e568d237b216167fddb521d0a7 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140
tdb3:
cr re ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
24bc46c83b39149f4845a575a82337eb46d91bdb cli: Add warning for duplicate port definition (tdb3)
e208fb5d3bea4c1fb750cb0028819635ecdeb415 cli: Sanitize ports in rpcconnect and rpcport (tdb3)
Pull request description:
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.
ACKs for top commit:
S3RK:
light code review ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
achow101:
ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
cbergqvist:
re ACK 24bc46c83b39149f4845a575a82337eb46d91bdb
Tree-SHA512: c83ab6a30a08dd1ac8b368a7dcc2b4f23170f0b61dd67ffcad7bcda05096d333bcb9821fba11018151f55b2929c0a333bfec15b8bb863d83f41fc1974c6efca5
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
cbc6c440e3811d342fa570713702900b3e3e75b9 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2b68684eae96070875ea13a60e3e7b0 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662427fbf1a43bb951364eface469bdb7 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f4785de74b548f7c4a256069e2aaf43 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3a7217eaa8c5972515bf622bedadfce rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6feaf1856e7886820b70874209fd90b rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec445e4d6bb277400758083b0b4e8b174 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da37a3203e05a9f39f303d7df19b6d81 test: refactor interface_rpc.py (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2960
Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found:
- returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error)
- returning both `"error"` and `"result"` fields together in a response object.
- different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors)
https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility.
https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned.
The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially:
- always return HTTP 200 "OK" unless there really is a server error or malformed request
- either return `"error"` OR `"result"` but never both
- same behavior for single and batch requests
If this is merged next steps can be:
- Refactor bitcoin-cli to always use strict 2.0
- Refactor the python test framework to always use strict 2.0 for everything
- Begin deprecation process for 1.0/1.1 behavior (?)
If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.
ACKs for top commit:
cbergqvist:
re ACK cbc6c440e3811d342fa570713702900b3e3e75b9
ryanofsky:
Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.
tdb3:
re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9
Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
992c714451676cee33d3dff49f36329423270c1c common: Don't terminate on null character in UrlDecode (Fabian Jahr)
099fa571511f113e0056d4bc27b3153a42f9dc65 scripted-diff: Modernize name of urlDecode function and param (Fabian Jahr)
8f39aaae417c33490e0e41fb97620eb23ced3d05 refactor: Remove hooking code for urlDecode (Fabian Jahr)
650d43ec15f7a3ae38126f65ef8fa0b1fd3ee936 refactor: Replace libevent use in urlDecode with our own code (Fabian Jahr)
46bc6c2aaa613eef526b21a06bf21e8edde31a88 test: Add unit tests for urlDecode (Fabian Jahr)
Pull request description:
Fixes#29654 (as a side-effect)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.
This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](e0a4574ba2/http.c (L3542)) we were using in `urlDecode` could be easily emulated in fewer LOC. This also ports the [applicable test vectors over from libevent](https://github.com/libevent/libevent/blob/master/test/regress_http.c#L3430).
ACKs for top commit:
achow101:
ACK 992c714451676cee33d3dff49f36329423270c1c
theStack:
Code-review ACK 992c714451676cee33d3dff49f36329423270c1c
maflcko:
ACK 992c714451676cee33d3dff49f36329423270c1c 👈
stickies-v:
ACK 992c714451676cee33d3dff49f36329423270c1c
Tree-SHA512: 78f76ae7ab3b6710eab2aaac20f55eb0da7803e057eaa6220e865f328666a5399ef1a479702aaf630b2f974ad3aa15e2b6adac9c11bc8c3d4be21e8af1667fea
The point of this was to be able to build bitcoin-tx and bitcoin-wallet without libevent, see #18504.
Now that we use our own implementation of urlDecode this is not needed anymore.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
c3e632b44153e314ef946f342c68c2758b1cbc4d Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type (Luke Dashjr)
Pull request description:
"v" would dereference beyond the string length, and "v10" would show as '1'
Turn both of these cases into a blank, like anything else unexpected currently is.
ACKs for top commit:
sipa:
utACK c3e632b44153e314ef946f342c68c2758b1cbc4d.
hernanmarino:
utACK c3e632b44153e314ef946f342c68c2758b1cbc4d
alfonsoromanz:
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d
achow101:
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d
Tree-SHA512: f641e4412521adae7c8c8e1f268bdaaa223d9048d8286e3df4b13905faaa0d601155ce581cd649f760cab2acc4122356fa94a44714f1f190845552100105eda0
CLI -netinfo will currently break when calling it on a node that is running
pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a transport_protocol_type
field.
Fix this by adding an `IsNull()` check as already done for other fields, and also:
- avoid checking for the full string "detecting", and instead do the cheaper
check for the most frequent case of the string starting with "v"
- drop displaying the "v" prefix in all the rows, as it doesn't add useful
information, and instead use "v" for the column header
- display nothing during peer setup, like for the -netinfo mping and ping columns
Since the kernel library no longer depends on the system file, move it
to the common library instead in accordance to the diagram in
doc/design/libraries.md.
fa266c4bbf564308ddbc12653527226506902084 Temporarily work around gcc-13 warning bug in interfaces_tests (MarcoFalke)
fa28850562bda0c2049aaff42103dfa6f05395dc Fix clang-tidy performance-unnecessary-copy-initialization warnings (MarcoFalke)
faaa60a30e7738a1490c9c2bb8963420f53c7f2d Remove unused find_value global function (MarcoFalke)
fa422aeec2909df0151177816dc1ff5eb5a1fbab scripted-diff: Use UniValue::find_value method (MarcoFalke)
fa548ac872c094edc94c2afda5cc9b0d84f73af0 Add UniValue::find_value method (MarcoFalke)
Pull request description:
The global function has issues:
* It causes gcc-13 warnings, see https://github.com/bitcoin/bitcoin/issues/26926
* There is no rationale for it being a global function, when it acts like a member function
* `performance-unnecessary-copy-initialization` clang-tidy isn't run on it
Fix all issues by making it a member function.
ACKs for top commit:
achow101:
ACK fa266c4bbf564308ddbc12653527226506902084
hebasto:
re-ACK fa266c4bbf564308ddbc12653527226506902084
Tree-SHA512: 6c4e25da3122cd3b91c376bef73ea94fb3beb7bf8ef5cb3853c5128d95bfbacbcbfb16cc843eb7b1a7ebd350c2b6311f8085eeacf9aeeab3366987037d209e44
