aa410c2b17 rpc: Validate maxfeerate with AmountFromValue (João Barbosa)
Pull request description:
With this change `maxfeerate` can also be set as a string, accordingly to the help test:
```
maxfeerate (numeric or string,
```
Beside, there are no tests for the removed errors.
ACKs for commit aa410c:
meshcollider:
utACK aa410c2b17
MarcoFalke:
utACK aa410c2b17 Good catch
Tree-SHA512: f3bfea91dc7daa943729e270585dbf333055aeda805fbd01eaab20a7e0e6147382647c11525334382d198df0d3d45da6102b541efda5a1361f96271c98d5d89d
7aab8d1024996c7c422bd34a8226df0117b813f7 [style] Code style fixups in GetWarnings() (John Newbery)
492c6dc1e742a62599dc6d5ba6c3896825b5144f util: change GetWarnings parameter to bool (John Newbery)
869b6314fd180856b6054fff28b5de994252c54c [qt] remove unused parameter from getWarnings() (John Newbery)
Pull request description:
`GetWarnings()` changes the format of the output warning string based on a passed-in string argument that can be set to "gui" or "statusbar".
Change the argument to a bool:
- there are only two types of behaviour, so a bool is a more natural argument type
- changing the name to `verbose` does not set any expectations for the how the calling code will use the returned string (currently, `statusbar` is used for RPC warnings, not a status bar)
- removes some error-handling code for when the passed-in string is not one of the two strings expected.
ACKs for top commit:
laanwj:
code review ACK 7aab8d1024996c7c422bd34a8226df0117b813f7
practicalswift:
ACK 7aab8d1024996c7c422bd34a8226df0117b813f7 -- diff looks correct :)
MarcoFalke:
ACK 7aab8d1024996c7c422bd34a8226df0117b813f7 otherwise.
promag:
Code review ACK 7aab8d1024996c7c422bd34a8226df0117b813f7.
Tree-SHA512: 75882c6e3e44aa9586411b803149b36ba487f4eb9cac3f5c8f07cd9f586870bba4488a51e674cf8147f05718534f482836e6a4e3f66e0d4ef6821900c7dfd04e
a5089f62bda9a39c1d6cbba285477670f1aa1f3f fix directory path for secp256k1 subtree in developer-notes (hackerrdave)
Pull request description:
Documentation update to fix the directory path of the `secp256k1` subtree in the developer notes
ACKs for top commit:
laanwj:
ACK a5089f62bda9a39c1d6cbba285477670f1aa1f3f
Tree-SHA512: d0986721d7091af26edaee769db78c9aabac25bbaddb2a1bfa96c7208187226e280e9c38897b5227ee6c9e40d5a1af86bb7c58e72c6a30a94a478c4bf54c086e
* refactor: create separate function for processing dsq messages in client.cpp
* refactor: avoid duplicate checks in CCoinJoinClientSession::ProcessMessage
* test: create tests for mixing and pending dsa requests in coinjoin_tests.cpp
* test: create tests for CTxDSIn in coinjoin_tests.cpp
* test: create tests for CoinJoinAccept in coinjoin_tests.cpp
* test: create tests for CoinJoinStatusUpdate in coinjoin_tests.cpp
* removal of check peer version
* Apply suggestions from code review
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
7c9e821c4e6cb186208ead9c8df616d1f393a49a scripts: add MACHO NOUNDEFS check to security-check.py (fanquake)
4ca92dc6d3f3e487d63286d8871d1829b3d279ff scripts: add MACHO PIE check to security-check.py (fanquake)
Pull request description:
This uses `otool -vh` to print the mach header and look for the `PIE` flag:
```bash
otool -vh src/bitcoind
Mach header
magic cputype cpusubtype caps filetype ncmds sizeofcmds flags
MH_MAGIC_64 X86_64 ALL LIB64 EXECUTE 24 2544 NOUNDEFS DYLDLINK TWOLEVEL WEAK_DEFINES BINDS_TO_WEAK PIE
```
From [`mach-o/loader.h`](https://opensource.apple.com/source/cctools/cctools-927.0.2/include/mach-o/loader.h.auto.html):
```c
#define MH_PIE 0x200000 /* When this bit is set, the OS will
load the main executable at a
random address. Only used in
MH_EXECUTE filetypes. */
```
ACKs for top commit:
laanwj:
code review ACK 7c9e821c4e6cb186208ead9c8df616d1f393a49a
Tree-SHA512: 5ba2f60440d0e31c70371a355c91ca4f723d80f7287d04e2098bf5b11892cc74216ff8f1454603c4db9675d4f7983614843b992b8dcfca0309aadf2aa7ab2e4b
90df92206cfce4e61eff9d584112643512f6b91c test: Change filemode of rpc_whitelist.py (Emil Engler)
Pull request description:
All python tests have the file mode `755`.
Probably due to a mistake `rpc_whitelist.py` is the only test with the permission `644`.
This PR makes it coherent with the other tests and updates it to `755` as well.
ACKs for top commit:
practicalswift:
ACK 90df92206cfce4e61eff9d584112643512f6b91c -- all tests should be executable
Tree-SHA512: b9e69cb5184a3bbee4c7b14ac35985145a9fd3403d0e449d79f15c18e9660cafec495d639f5f730e0c69dde5f4a3d7590b4e42d385e794cd02add1f4e3b785e7
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
37fe80e6267094f6051ccf9bec0c7f1a6b9e15da Only consider addrv2 peers for relay of non-addrv1 addresses (Pieter Wuille)
83f8821a6f41854edd5c0b11deabba658890cde1 refactor: add IsAddrCompatible() to CNode (Pieter Wuille)
Pull request description:
When selecting peers to relay an address to, only pick addrv2-capable ones if the address cannot be represented in addr(v1).
Without this I expect that propagation of torv3 addresses over the cleartext network will be very hard for a while.
ACKs for top commit:
jonatack:
ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da
vasild:
ACK 37fe80e6267094f6051ccf9bec0c7f1a6b9e15da
Tree-SHA512: 18a854ea43ad473cf89b9c5193b524109d7af75c26f7aa7e26cd72ad0db52f19c8001d566c607a7e6772bc314f770f09b6c3e07282d110c5daea193edc592cd2
1583498fb6781c01ca2f33c09319ed793964c574 Send and require SENDADDRV2 before VERACK (Pieter Wuille)
c5a89196602e43ebb1cdc9cd4f08d153419c13e1 Don't send 'sendaddrv2' to pre-70016 software (Pieter Wuille)
Pull request description:
BIP155 defines addrv2 and sendaddrv2 for all protocol versions, but some implementations reject messages they don't know. As a courtesy, don't send it to nodes with a version before 70016, as no software is known to support BIP155 that doesn't announce at least that protocol version number.
Also move the sending of sendaddrv2 earlier (before sending verack), as proposed in https://github.com/bitcoin/bips/pull/1043. This has the side effect that local address broadcast of torv3 will work (as it'll only trigger after we know whether or not the peer supports addrv2).
ACKs for top commit:
MarcoFalke:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jnewbery:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
jonatack:
ACK 1583498fb6781c01ca2f33c09319ed793964c574
vasild:
ACK 1583498
Tree-SHA512: 3bd5833fa8c8567b6dedd99e4a9b6bb71c127aa66d5284b217503c86d597dc59aa7382c41f3a4bf561bb658b89db81d1a7703a700eef4ffc17cb916660e23a82
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
