2081442c421cc4376e5d7839f68fbe7630e89103 test: Add test for rpc_whitelist (Emil Engler)
7414d3820c833566b4f48c6c120a18bf53978c55 Add RPC Whitelist Feature from #12248 (Jeremy Rubin)
Pull request description:
Summary
====
This patch adds the RPC whitelisting feature requested in #12248. RPC Whitelists help enforce application policies for services being built on top of Bitcoin Core (e.g., your Lightning Node maybe shouldn't be adding new peers). The aim of this PR is not to make it advisable to connect your Bitcoin node to arbitrary services, but to reduce risk and prevent unintended access.
Using RPC Whitelists
====
The way it works is you specify (in your bitcoin.conf) configurations such as
```
rpcauth=user1:4cc74397d6e9972e5ee7671fd241$11849357f26a5be7809c68a032bc2b16ab5dcf6348ef3ed1cf30dae47b8bcc71
rpcauth=user2:181b4a25317bff60f3749adee7d6bca0$d9c331474f1322975fa170a2ffbcb176ba11644211746b27c1d317f265dd4ada
rpcauth=user3:a6c8a511b53b1edcf69c36984985e$13cfba0e626db19061c9d61fa58e712d0319c11db97ad845fa84517f454f6675
rpcwhitelist=user1:getnetworkinfo
rpcwhitelist=user2:getnetworkinfo,getwalletinfo, getbestblockhash
rpcwhitelistdefault=0
```
Now user1 can only call getnetworkinfo, user2 can only call getnetworkinfo or getwalletinfo, while user3 can still call all RPCs.
If any rpcwhitelist is set, act as if all users are subject to whitelists unless rpcwhitelistdefault is set to 0. If rpcwhitelistdefault is set to 1 and no rpcwhitelist is set, act as if all users are subject to whitelists.
Review Request
=====
In addition to normal review, would love specific review from someone working on LN (e.g., @ roasbeef) and someone working on an infrastructure team at an exchange (e.g., @ jimpo) to check that this works well with their system.
Notes
=====
The rpc list is spelling sensitive -- whitespace is stripped though. Spelling errors fail towards the RPC call being blocked, which is safer.
It was unclear to me if HTTPReq_JSONRPC is the best function to patch this functionality into, or if it would be better to place it in exec or somewhere else.
It was also unclear to me if it would be preferred to cache the whitelists on startup or parse them on every RPC as is done with multiUserAuthorized. I opted for the cached approach as I thought it was a bit cleaner.
Future Work
=====
In a future PR, I would like to add an inheritance scheme. This seemed more controversial so I didn't want to include that here. Inheritance semantics are tricky, but it would also make these whitelists easier to read.
It also might be good to add a `getrpcwhitelist` command to facilitate permission discovery.
Tests
=====
Thanks to @ emilengler for adding tests for this feature. The tests cover all cases except for where `rpcwhitelistdefault=1` is used, given difficulties around testing with the current test framework.
ACKs for top commit:
laanwj:
ACK 2081442c421cc4376e5d7839f68fbe7630e89103
Tree-SHA512: 0dc1ac6a6f2f4b0be9c9054d495dd17752fe7b3589aeab2c6ac4e1f91cf4e7e355deedcb5d76d707cbb5a949c2f989c871b74d6bf129351f429569a701adbcbf
b6f9e3576a1ea18572e4803aeb3f39330f0cb759 test: re-enable CLI test support by using EncodeDecimal in json.dumps() (fanquake)
Pull request description:
As mentioned in https://github.com/bitcoin/bitcoin/pull/17675#issuecomment-563188648.
ACKs for top commit:
practicalswift:
ACK b6f9e3576a1ea18572e4803aeb3f39330f0cb759 assuming Travis is happy too -- diff looks correct :)
MarcoFalke:
> ACK b6f9e35 assuming Travis is happy too -- diff looks correct :)
Tree-SHA512: 79fa535cc1756c8ee610a3d6a316a1c4f036797d6990a5620e44985393a2e52f78450f8e0021d0a148c08705fd1ba765508464a365f9030ae0d2cacbd7a93e19
e97f5c18238835bc3a3aee2e9e65b287f1c8b938 depends: don't configure xcb_proto (fanquake)
Pull request description:
xcb_proto's configure doesn't understand `--disable-shared` or
`--with-pic`. All the package does it put a stack of XML files into
a directory to be used by libxcb.
Probably enough to close#16354.
ACKs for top commit:
dongcarl:
ACK e97f5c18238835bc3a3aee2e9e65b287f1c8b938
Tree-SHA512: 1a49fd7c8269405bbf312be33c1aeaac5f25ef8666829b01dc3c58f3a2a9281c23c42614a7f1cfc3ee260be4ea3e71285869b1cb9c2035dceda336296d9d9dea
3ab18246254019896132d1cdb8af2dcdb213ec3b build: Use dnl for all comments in configure.ac, rather than # (fanquake)
8ddcbb4e41fa91e7f80efe6d9c4d5e9bb1355036 build: Remove backticks from configure.ac (fanquake)
Pull request description:
Use `dnl` for all comments, rather than `#`.
Remove backticks - Their usage for the `bdb_prefix` and `qt5_prefix` commands may have improved backwards compatibility in some cases, however we now require recent versions of macOS. I'm not sure why they were being used in the `HAVE_STD__SYSTEM` and `HAVE_WSYSTEM` defines.
ACKs for top commit:
dongcarl:
ACK 3ab18246254019896132d1cdb8af2dcdb213ec3b
hebasto:
ACK 3ab18246254019896132d1cdb8af2dcdb213ec3b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2bcffb52c365acff87a0e6b9527ae31f36fdabb7ea095a8fd261f9a39b2c2848f5dfc148bc38d21e21e7bd761b1a2960e9a96f508c66be84d9569b8a401e812a
llmq/snapshot.cpp:31:22: error: loop variable 'h' binds to a temporary value produced by a range of type 'const std::vector<bool>' [-Werror,-Wrange-loop-bind-reference]
for (const auto& h : activeQuorumMembers) {
^
llmq/snapshot.cpp:31:10: note: use non-reference type 'std::__bit_const_reference<std::vector<bool>>'
for (const auto& h : activeQuorumMembers) {
^~~~~~~~~~~~~~~
1 error generated.
fa949b3c1325693ea7ecc5556b2de50d2a6c9ead test: Suppress epoll_ctl data race (MarcoFalke)
Pull request description:
Happens intermittently: https://cirrus-ci.com/task/5462892373868544?command=ci#L5385
ACKs for top commit:
hebasto:
ACK fa949b3c1325693ea7ecc5556b2de50d2a6c9ead, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: d5aa559fc105053da594531722f2a03d898eadeb4413c3a728fc5116cc4d1a2c16c49649a24c75ea810e4ec6bb9728b0bcd2ea991886bb9d206170218eddf6d2
d103484fe81a8a5bf1d692f3f7d1c0ef1be5f63c util: Do not use gArgs global in ArgsManager member functions (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
practicalswift:
ACK d103484fe81a8a5bf1d692f3f7d1c0ef1be5f63c: patch looks correct
Tree-SHA512: dda7a5062363170c6995f2fd8fda48c0a919e5ca67be9faa8f0fa66f9d3b535f134eb6f4860a0859bc5457c02230b34a8d1264045f22bed8d30668158ac2271f
f22d6a11423a4462196de24cd68e7f45513cc001 log: Remove static log message "Initializing chainstate Chainstate [ibd] @ height -1 (null)" (practicalswift)
Pull request description:
Remove static log message `Initializing chainstate Chainstate [ibd] @ height -1 (null)`.
AFAICT `chainstate->ToString()` will always equal `"Chainstate [ibd] @ height -1 (null)"` here which makes the log message neither relevant nor interesting :)
ACKs for top commit:
laanwj:
ACK f22d6a11423a4462196de24cd68e7f45513cc001
promag:
ACK f22d6a11423a4462196de24cd68e7f45513cc001, just get rid of it.
hebasto:
ACK f22d6a11423a4462196de24cd68e7f45513cc001, I agree that the removed log message in its current state is cryptic and useless.
Tree-SHA512: 1a65c0d14c9a433afcdaadef9bfcdd5d63276d5d2caee1bf3c48ac477e54fa28138f64020e6e26ca5e67872954a1e7d93fa24a12accc7c7211bc6e7a6039051d
0bbe26a1af2aab2287b18048f80b3f70e63e0044 wallet: filter for keys only before record deser in salvage (Andrew Chow)
544e12a4e81633d222574eec253a1ff292d3c4a5 walletdb: Add KeyFilterFn to ReadKeyValue (Andrew Chow)
Pull request description:
When salvaging a wallet, the only things that matter are the private keys. It is not necessary to attempt to deserialize any other records, especially if those records are corrupted too.
This PR adds a `KeyFilterFn` function callback to `ReadKeyValue` that salvage uses to filter for only the records that it wants. Of course doing it this way also lets us do other filters in the future from other places should we so desire.
ACKs for top commit:
ryanofsky:
Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044. Looks great! This should make the recovery code more robust. Normally it'd be good to have a test case for the problem this fixes, but Marco already wrote one in #19078, so I think we're covered
laanwj:
Code review ACK 0bbe26a1af2aab2287b18048f80b3f70e63e0044
Tree-SHA512: 8e3ee283a22a79273915711c4fb751f3c9b02ce94e6bf08dc468f1cfdf9fac35c693bbfd2435ce43c3a06c601b9b0a67e209621f6814bedfe3bc7a7ccc37bb01
9bdde3c802e1512cbcd8ae8f7db72b6fe13ea829 build: add /usr/local/ to LCOV_FILTER_PATTERN for macOS builds (eugene)
Pull request description:
With this commit, the files in /usr/local/ will not be included in
`make cov` or `make cov_fuzz` coverage reports. This behavior could
be observed when generating the reports on macOS with brew-installed
clang.
ACKs for top commit:
fanquake:
ACK 9bdde3c802e1512cbcd8ae8f7db72b6fe13ea829
Tree-SHA512: 15cbe8d514651448f3d7b8b0a00939fd6bd6f15e6812f1959fcaaab7364ca2ef4ee34f2ff8950b7fdc8ae64d043dc5f7185c0601dd94780b41331337e5e84c45
4148f55dd016f940df50a44cf03d117cdb1dd929 docs: Correct description for getblockstats's txs field (Nadav Ivgi)
Pull request description:
It does count the coinbase transaction.
Refs #19766
ACKs for top commit:
MarcoFalke:
ACK 4148f55dd016f940df50a44cf03d117cdb1dd929
theStack:
ACK 4148f55dd016f940df50a44cf03d117cdb1dd929
Tree-SHA512: ccd420f19242efbbbecfe822c825363bc89e26618834de0d805f5cdb07461c8bdc6e077c61ea8cd0d40564a96c67d8a71c68175c8543bb849909d7ae375b2a92
fa55c1d5fdd88c4bc4d361da231cd63b20255b50 build: Add Werror=range-loop-analysis (MarcoFalke)
Pull request description:
The warning is implicitly enabled for Bitcoin Core. Also explicitly since commit d92204c900d.
To avoid "fix range loop" follow-up refactors, we have two options:
* Disable the warning, so that issues never appear
* Enable it as an error, so that issues are either caught locally or by ci
ACKs for top commit:
fanquake:
ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
practicalswift:
ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50 -- pre-review fix-up is better than post-review fix-up
hebasto:
re-ACK fa55c1d5fdd88c4bc4d361da231cd63b20255b50
Tree-SHA512: 019aa133f254af8882c1d5d10c420d9882305db0fc2aa9dad7d285168e2556306c3eedcc03bd30e63f11eae4cc82b648d83fb6e9179d6a6364651fb602d70134
35cd2da623e32b975fbc485c3605934e4aa8bdc5 test: Fix 'make cov' with clang (Hennadii Stepanov)
Pull request description:
This is a follow up of #19688.
With this PR it is possible to do the following:
```
$ ./autogen.sh
$ ./configure --enable-lcov CC=clang CXX=clang++
$ make
$ make cov
```
Currently, on master (8a85377cd0b60cb00dae4f595d628d1afbd28bd5), `make cov` fails to `Processing src/test/test_bitcoin-util_tests.gcda`.
ACKs for top commit:
vasild:
ACK 35cd2da
Crypt-iQ:
ACK 35cd2da
Tree-SHA512: aaf56118e2644064e9738a8279889c617db5805c5c804c904469b24c496bd609f9c5fc2aebcf1a422f8a5ed2eb38bd6e76b484680310b55c36d922b73a4c33cf
a51d0ad2de89b9757d158df95ddeba2bfcb23935 rpc: Improve addnode remove command error message (Fabian Jahr)
Pull request description:
The `addnode` RPC with the `remove` command parameter is used to remove a node from the "added nodes". It did not have test coverage and in case of failure to remove the node it responded with the confusing message "Error: Node has not been added.".
This PR adds test coverage and introduces a new error code as well as changes the error message to something that makes sense.
ACKs for top commit:
laanwj:
Code review ACK a51d0ad2de89b9757d158df95ddeba2bfcb23935
theStack:
Tested ACK https://github.com/bitcoin/bitcoin/commit/a51d0ad2de
Tree-SHA512: 033ef5de0d4d49d58ef4df3759b838c9d19ee9dfb0aff9f814a3a63d124ca231a442c930efa7d343fe1f65727c4b59fc23dd5e26fe6ea69f9e84fda48b5c5cc2
99993489da9bc003b823bcab10e5f5297b369431 test: Set -logthreadnames in unit tests (MarcoFalke)
fa4ea997b4da1ae0afafba223fff9efbeefaf555 init: Setup scheduler in tests and init in exactly the same way (MarcoFalke)
Pull request description:
Generally the unit tests are single threaded, with the exception of the script check threads, the schedule, and optionally indexer threads.
Like the functional tests, the thread name can serve additional debug information, so set `-logthreadnames` in unit tests.
Can be tested with
```
./src/test/test_bitcoin -l test_suite -t validation_tests/test_combiner_all -- DEBUG_LOG_OUT
ACKs for top commit:
laanwj:
ACK 99993489da9bc003b823bcab10e5f5297b369431
Tree-SHA512: 3bdbfc211da146da64b50b0826246aff5c611a84b69ab896a55b3c9d1adc92c5975da36ab92aee577df82e229c4326b477f4105bfdd1a5df4c9a0b018cf61602
498cdbb42616bc1e6c44492c907678b4dce41870 Fix improper Doxygen inline comments (Ben Woosley)
Pull request description:
The proper syntax is `//!<`
http://www.doxygen.nl/manual/docblocks.html#memberdoc
Identified via `-Wdocumentation`:
```
In file included from ./util/system.h:26:
./util/settings.h:74:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
const SettingsValue* begin() const; //<! Pointer to first non-negated value.
^~~~
///<
./util/settings.h:75:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
const SettingsValue* end() const; //<! Pointer to end of values.
^~~~
///<
./util/settings.h:76:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
bool empty() const; //<! True if there are any non-negated values.
^~~~
///<
./util/settings.h:77:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
bool last_negated() const; //<! True if the last value is negated.
^~~~
///<
./util/settings.h:78:41: error: not a Doxygen trailing comment [-Werror,-Wdocumentation]
size_t negated() const; //<! Number of negated values.
^~~~
///<
```
ACKs for top commit:
fanquake:
ACK 498cdbb42616bc1e6c44492c907678b4dce41870
Tree-SHA512: 2851fc1cbbcf700d198d82ce4923b2ef4a700f8ce19dff431ecf24f4e6fecda9fed1b4b4d148f3c1adfb6b0c6bff5d5315ee01bbcd855eb3d83e1a69b0c98893
This was previously hard-coded in Sentinel, and can be useful to refactor
future versions of Sentinel (in progress), as well as to bring superblock logic
into DashCore in the future.
* feat(qt): Let users resend stuck txes one by one via context menu
The corresponding rpc was removed due to privacy concerns (resending many txes at once can hurt privacy). On the other hand by the time users try to resend txes by hand they would be locked via IS already (normally). We don't resend IS-locked txes which means we can be pretty sure that (in most cases) no IS lock means no such txes were ever broadcasted for some reason. With this in mind resending txes one by one is probably ok-ish since it would be similar to regular tx creation/sending.
* Only allow tx resend for single row selections
* format
