fa7592bfa8 rpc: Update server to use new RPCHelpMan (MarcoFalke)
aaaaad5627 rpc: Add option to hide RPCArg (MarcoFalke)
fa9708f94c rpc: Assert that passed arg names are equal to hardcoded ones (MarcoFalke)
faaeb2b0b3 rpc: Add CRPCCommand constructor which takes RPCHelpMan (MarcoFalke)
fa8ec00061 rpc: Check that left section is not multiline (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch the RPC methods in server. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
laanwj:
ACK fa7592bfa8
ryanofsky:
Code review ACK fa7592bfa8. Looks great! Just some hidden arg and Check() and comment cleanups since last review
Tree-SHA512: e64b6a212f4a3aeedeee47557559bde104d5fd40cdc1746b27eb2f3d4c8885d5e6e4dd287595ea11cdbc6a939654fe103cae765fd505875444d851f0abb11308
da73f1513a qt: Fix shutdown when waitfor* cmds are called from RPC console (Hennadii Stepanov)
Pull request description:
On master (7eed413e72), if the GUI has been started with`-server=1`, `bitcoin-qt` hangs on shutdown during calling any of the `waitfor*` commands in the GUI RPC console.
This PR suggests minimal changes to fix this bug.
Fix#17495
ACKs for top commit:
jonasschnelli:
utACK da73f1513a
Tree-SHA512: 469f5332945a5f2c57d19336cda5df79b123ccc494aea6d58a85eb1293be52708b2b9c5bb6bc2c402a90b7b4e9e8d7ab8fe84cf201cf7ce612c9290c57e43681
fae86c38bc util: Remove unused MilliSleep (MarcoFalke)
fa9af06d91 scripted-diff: Replace MilliSleep with UninterruptibleSleep (MarcoFalke)
fa4620be78 util: Add UnintrruptibleSleep (MarcoFalke)
Pull request description:
We don't use the interruptible feature of boost's sleep anywhere, so replace it with the sleep in `std::thread`
ACKs for top commit:
ajtowns:
ACK fae86c38bc quick code review
practicalswift:
ACK fae86c38bc -- patch looks correct
sipa:
Concept and code review ACK fae86c38bc
fanquake:
ACK fae86c38bc - note that an instance of `DHAVE_WORKING_BOOST_SLEEP_FOR` was missed in the [linter](https://github.com/bitcoin/bitcoin/blob/master/test/lint/extended-lint-cppcheck.sh#L69), but that can be cleaned up later.
Tree-SHA512: 7c0f8eb197664b9f7d9fe6c472c77d384f11c797c913afc31de4b532e3b4fd9ea6dd174f92062ff9d1ec39b25e0900ca7c597435add87f0f2477d9557204848c
This is safe because MilliSleep is never executed in a boost::thread,
the only type of thread that is interruptible.
* The RPC server uses std::thread
* The wallet is either executed in an RPC thread or the main thread
* bitcoin-cli, benchmarks and tests are only one thread (the main thread)
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/MilliSleep\((\S+)\);/UninterruptibleSleep(std::chrono::milliseconds{\1});/g' $(git grep -l MilliSleep)
-END VERIFY SCRIPT-
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
Harmonize the user-facing output of the `bitcoind -daemon`, `bitcoin-cli help stop`, `bitcoin-cli stop`, and `bitcoind -version` commands to be consistent with each other as well as with the "Bitcoin Core is probably already running" messages, e.g. `git grep 'probably already running.")'`.
b6fb617aaa rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm)
c7a9fc234f Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm)
5c5e32bbe3 rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm)
0ab8ba1ac6 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm)
Pull request description:
Every single RPC call has a helper-section at the start, which throws a help string if the user asks for help or if the user provided too few/many arguments.
```C++
const RPCHelpMan help{...};
if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
throw std::runtime_error(help.ToString());
}
```
or (older version)
```C++
if (request.fHelp || request.params.size() < min || request.params.size() > max)
throw std::runtime_error(
RPCHelpMan{...}.ToString()
);
```
It seems like an obvious improvement, and less copy-pasting, to make `RPCHelpMan` aware of `JSONRPCRequest`, and to let it handle the checks instead. Both of the above become
```C++
RPCHelpMan{...}.Check(request);
```
which means we save roughly 3 lines per RPC command, and the `RPCHelpMan` instance is never referenced afterwards, so the approach is a tiny fraction cleaner.
This is a complete update, sans a few special case locations that had special rules. 623 lines turn into 284 (which includes the addition to `RPCHelpMan`).
ACKs for top commit:
laanwj:
code rview and lightly tested ACK b6fb617aaa
MarcoFalke:
ACK b6fb617aaa, looked at the diff, verified move-only where applicable
Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1
8a6810d0d2 Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
4d074e84a2 [build] Move AnalyzePSBT from psbt.cpp to node/psbt.cpp (Russell Yanofsky)
fd509bd1f7 [docs] Document src subdirectories and different libraries (John Newbery)
9eaeb7fb8d [build] Move wallet load functions to wallet/load unit (John Newbery)
91a25d1e71 [build] Add several util units (John Newbery)
99517866b6 [build] Move several units into common libraries (John Newbery)
0509465542 [build] Move rpc rawtransaction util functions to rpc/rawtransaction_util.cpp (John Newbery)
1acc61f874 [build] Move rpc utility methods to rpc/util (John Newbery)
4a75c9d651 [build] Move policy settings to new src/policy/settings unit (John Newbery)
fdf8888b6f [build] Move CheckTransaction from lib_server to lib_consensus (John Newbery)
Pull request description:
This is a move-only commit. No code is changing and the moves can be easily verified with:
```sh
git log -p -n1 --color-moved=dimmed_zebra
```
This commit moves functions and variables that wallet code depends on out of libbitcoin_server.a, so the bitcoin-wallet tool can be built without libbitcoin_server.a in #15639, and attempting to access server state from wallet code will result in link errors instead of silently broken code.
List of moves:
- `CheckTransaction` moves from `consensus/tx_verify.cpp` to `consensus/tx_check.cpp`
- `urlDecode` moves from `httpserver.cpp` to `util/url.cpp`
- `TransactionErrorString` moves from `node/transaction.cpp` to `util/error.cpp`
- `StringForFeeReason` and `FeeModeFromString` move from `policy/fees.cpp` to `util/fees.cpp`
- `incrementalRelayFee` `dustRelayFee` and `nBytesPerSigOp` move from `policy/policy.cpp` to `policy/settings.cpp`
- `SignalsOptInRBF` moves from `policy/rbf.cpp` to `util/rbf.cpp`
- `fIsBareMultisigStd` moves from `validation.cpp` to `policy/settings.cpp`
- `ConstructTransaction` `TxInErrorToJSON` and `SignTransaction` move from `rpc/rawtransaction.cpp` to `rpc/rawtransaction_util.cpp`
- `RPCTypeCheck` `RPCTypeCheckArgument` `RPCTypeCheckObj` `AmountFromValue` `ParseHashV``ParseHashO` `ParseHexV` `ParseHexO` `HelpExampleCli` and `HelpExampleRpc` move from `rpc/server.cpp` to `rpc/util.cpp`
- `AmountHighWarn` and `AmountErrMsg` move from `ui_interface.cpp` to `util/error.cpp`
- `FormatStateMessage` and `strMessageMagic` move from `validation.cpp` to `util/validation.cpp`
- `VerifyWallets` `LoadWallets` `StartWallets` `FlushWallets` `StopWallets` and `UnloadWallets` move from `wallet/init.cpp` to `wallet/node.cpp`
ACKs for commit 4d074e:
jnewbery:
utACK 4d074e84a2 (checked by doing the rebase myself and verifying no difference between my branch and 4d074e84a2)
Tree-SHA512: 5e1604a9fb06475f2b96da0de0baa8330f4dda834dc20a0183ef11e1e4c27631d1d1bbb9abf0054efc03d56945fdf9920f63366b6a4f200f665b742a479ff75c
Moves the following utility methods to rpc/util and moves that unit to
libbitcoin_common so they can be accessed by all libraries.
- `RPCTypeCheck`
- `RPCTypeCheckArgument`
- `RPCTypeCheckObj`
- `AmountFromValue`
- `ParseHashV``ParseHashO`
- `ParseHexV`
- `ParseHexO`
- `HelpExampleCli`
- `HelpExampleRpc`
faa1522e5e RPCHelpMan: Pass through Result and Examples (MarcoFalke)
Pull request description:
Passing the rpc result and rpc examples through `RPCHelpMan` makes it clear in what order they appear in the stringified version. Future improvements could then autoformat or autogenerate them.
Tree-SHA512: b32a5c178cc80f50a7e9b93a38e2b26d5994188ecafe9e61bbc599941b44b9b0e4e4be6413d4464fac6e8e73661a191a77d34917f2e6293de19fb59519dd4487
a0ac15459a doc: Add getrpcinfo release notes (João Barbosa)
251a91c1bf qa: Add tests for getrpcinfo (João Barbosa)
d0730f5ce4 rpc: Add getrpcinfo command (João Barbosa)
068a8fc05f rpc: Track active commands (João Barbosa)
bf4383277d rpc: Remove unused PreCommand signal (João Barbosa)
Pull request description:
The new `getrpcinfo` command exposes details of the RPC interface. The details can be configuration properties or runtime values/stats.
This can be particular useful to coordinate concurrent functional tests (see #14958 from where this was extracted).
Tree-SHA512: 7292cb6087f4c429973d991aa2b53ffa1327d5a213df7d6ba5fc69b01b2e1a411f6d1609fed9234896293317dab05f65064da48b8f2b4a998eba532591d31882
cb53b825c2 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee)
2196c51821 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee)
Pull request description:
Replace boost::bind with std::bind
- In `src/rpc/server.cpp`, replace `std::transform` with simple loop.
- In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect.
- In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object.
Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0
28479f926f qa: Test bitcond shutdown (João Barbosa)
8d3f46ec39 http: Remove timeout to exit event loop (João Barbosa)
e98a9eede2 http: Remove unnecessary event_base_loopexit call (João Barbosa)
6b13580f4e http: Unlisten sockets after all workers quit (João Barbosa)
18e9685816 http: Send "Connection: close" header if shutdown is requested (João Barbosa)
02e1e4eff6 rpc: Add wait argument to stop (João Barbosa)
Pull request description:
Fixes#11777. Reverts #11006. Replaces #13501.
With this change the HTTP server will exit gracefully, meaning that all requests will finish processing and sending the response, even if this means to wait more than 2 seconds (current time allowed to exit the event loop).
Another small change is that connections are accepted even when the server is stopping, but HTTP requests are rejected. This can be improved later, especially if chunked replies are implemented.
Briefly, before this PR, this is the order or events when a request arrives (RPC `stop`):
1. `bufferevent_disable(..., EV_READ)`
2. `StartShutdown()`
3. `evhttp_del_accept_socket(...)`
4. `ThreadHTTP` terminates (event loop exits) because there are no active or pending events thanks to 1. and 3.
5. client doesn't get the response thanks to 4.
This can be verified by applying
```diff
// Event loop will exit after current HTTP requests have been handled, so
// this reply will get back to the client.
StartShutdown();
+ MilliSleep(2000);
return "Bitcoin server stopping";
}
```
and checking the log output:
```
Received a POST request for / from 127.0.0.1:62443
ThreadRPCServer method=stop user=__cookie__
Interrupting HTTP server
** Exited http event loop
Interrupting HTTP RPC server
Interrupting RPC
tor: Thread interrupt
Shutdown: In progress...
torcontrol thread exit
Stopping HTTP RPC server
addcon thread exit
opencon thread exit
Unregistering HTTP handler for / (exactmatch 1)
Unregistering HTTP handler for /wallet/ (exactmatch 0)
Stopping RPC
RPC stopped.
Stopping HTTP server
Waiting for HTTP worker threads to exit
msghand thread exit
net thread exit
... sleep 2 seconds ...
Waiting for HTTP event thread to exit
Stopped HTTP server
```
For this reason point 3. is moved right after all HTTP workers quit. In that moment HTTP replies are queued in the event loop which keeps spinning util all connections are closed. In order to trigger the server side close with keep alive connections (implicit in HTTP/1.1) the header `Connection: close` is sent if shutdown was requested. This can be tested by
```
bitcoind -regtest
nc localhost 18443
POST / HTTP/1.1
Authorization: Basic ...
Content-Type: application/json
Content-Length: 44
{"jsonrpc": "2.0","method":"stop","id":123}
```
Summing up, this PR:
- removes explicit event loop exit — event loop exits once there are no active or pending events
- changes the moment the listening sockets are removed — explained above
- sends header `Connection: close` on active requests when shutdown was requested which is relevant when it's a persistent connection (default in HTTP 1.1) — libevent is aware of this header and closes the connection gracefully
- removes event loop explicit break after 2 seconds timeout
Tree-SHA512: 4dac1e86abe388697c1e2dedbf31fb36a394cfafe5e64eadbf6ed01d829542785a8c3b91d1ab680d3f03f912d14fc87176428041141441d25dcb6c98a1e069d8
Just a preparatory commit to add the header to the includes and run
clang-format to sort the include lists.
Splitting this up into a separate commit makes future scripted-diffs
easier.
