| Age | Commit message (Collapse) | Author |
|---|
|
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.
Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.
Contains release notes and several interface improvements by
John Newbery.
Github-Pull: #19219
Rebased-From: b691f2df5f7d443c0c9ee056ab94aa0fc19566d5
|
|
Github-Pull: #18665
Rebased-From: b91e4ae0d8ab2ae6b77585c97c52d825f56ed539
|
|
7fcdec0f326f7fb547ec9c651871842cf518ea38 Remove PID file at the very end (Hennadii Stepanov)
Pull request description:
While reproducing the bug from #18517, I've noticed that the `bitcoind.pid` file has already been removed when the `bitcoind` hangs.
This PR makes `Shutdown()` keep the `bitcoind.pid` file available until the end.
ACKs for top commit:
MarcoFalke:
ACK 7fcdec0f326f7fb547ec9c651871842cf518ea38
emilengler:
utACK 7fcdec0f326f7fb547ec9c651871842cf518ea38
promag:
Code review ACK 7fcdec0f326f7fb547ec9c651871842cf518ea38.
theStack:
Code review ACK 7fcdec0f326f7fb547ec9c651871842cf518ea38
Tree-SHA512: 9732ef34e137dbee70a06d922b316b8ea7b9a1c959cf8861b6940cd789336dc19ee468a4c3a28d95d1458076a48270c676b0ff27fec30cf57eced6ddab0a2a9b
|
|
fa369651c5523f393e0bfcfa328b8e27006711aa net: Add missing cs_vNodes lock (MarcoFalke)
Pull request description:
Fixes #18457
ACKs for top commit:
promag:
Code review ACK fa369651c5523f393e0bfcfa328b8e27006711aa.
laanwj:
ACK fa369651c5523f393e0bfcfa328b8e27006711aa
Tree-SHA512: 60d7000f2f3d480bb0953ce27a0020763e7102da16a0006b619e0a236cfc33cbd4f83d870e9f0546639711cd877c1f9808d419184bbc153bb328885417e0066c
|
|
|
|
|
|
|
|
621e86ee8d0102e2bf41f7656a368083b89b2f83 Update -blocksonly documentation (glowang)
Pull request description:
When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0.
This behavior is not captured by the current documentation, which
claims that -blocksonly does not impact any wallet transactions at
all.
Fixes #17294
ACKs for top commit:
MarcoFalke:
ACK 621e86ee8d0102e2bf41f7656a368083b89b2f83
Tree-SHA512: f47bfb40a196c23e62505e1d4f79094011ac7c21fc9b920fad60cdadb5c4f48e993be1f015e26e568ce329967c24848fd7b665a6cffd3881f4cfcd2fd0081ed8
|
|
When -blocksonly is set to 1, it interacts with the -walletbroadcast
parameter and sets it to 0 if it has not been set already.This behavior
is not captured by the current documentation, which claims that -blocksonly
does not impact any wallet transactions.
Update the max number of outgoing peers from 8 to 10, due to the
addition of two -blocksonly peers.
|
|
signals
e57980b4738c10344baf136de3e050a3cb958ca5 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f36124972d2364f941de9c3417c65f05b6 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f527631ede1a31c7855151e5c5d91f8f [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b4000fed088b8cf7b99674c328d15e1 [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443cc16edf974f099b8485e04b3db1b1d7 [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d759b13af68acec6d5bfa04aaa24561f8 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b4738c10344baf136de3e050a3cb958ca5, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
|
|
fa36f3a29538012a6eb5c3402b3b3c18fd32b230 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cff9a9a340eac1b5a853e2467d5e0ef7 scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4e304646b4610228f3975b3a9762643 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)
Pull request description:
Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`
ACKs for top commit:
vasild:
ACK fa36f3a (code review, not tested)
ajtowns:
ACK fa36f3a29538012a6eb5c3402b3b3c18fd32b230
jonatack:
ACK fa36f3a
Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
|
|
This refactor does two things:
* Pass mempool in to PeerLogicValidation
* Pass m_mempool around where needed
|
|
NotifyEntryAdded never had any subscribers so can be removed.
Since ConnectTrace no longer subscribes to NotifyEntryRemoved, there are
now no subscribers.
The CValidationInterface TransactionAddedToMempool and
TransactionRemovedFromMempool methods can now provide this
functionality. There's no need for a special notifications framework for
the mempool.
|
|
|
|
|
|
Calling interrupt_all() will immediately stop the scheduler, so it's
safe to invoke stop() beforehand, and this removes the reliance on boost
to interrupt serviceQueue().
|
|
and update feature_asmap.py and test_runner.py
This commit moves the asmap init.cpp code from the end of "Step 12: start node"
to "Step 6: network initialization" to provide feedback on passing an -asmap
config arg much more quickly. This change speeds up the feature_asmap.py
functional test file from 60 to 5 seconds by accelerating the 2 tests that use
`assert_start_raises_init_error`.
Credit to Wladimir J. van der Laan for the suggestion.
|
|
- move asmap #includes to sorted positions in addrman and init (move-only)
- remove redundant quotes in asmap InitError, update test
- remove full stops from asmap logging to be consistent with debug logging,
update tests
|
|
and update the tests.
|
|
- allow passing an absolute file path to the -asmap config arg
- update the -asmap config help
- add a functional test in feature_asmap.py
|
|
and move to sorted position
|
|
|
|
facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See https://github.com/bitcoin/bitcoin/blob/4a072330763b3ff2d1b5c5b8d30a9517873ac6de/src/net_processing.cpp#L3862-L3866
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b3469915d8c9ba4cd3b918f16782edf0de, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
|
|
8bca30ea17cd4c1dacee28eaa27e5fa3493b021d [rpc] expose ability to mock scheduler via the rpc (Amiti Uttarwar)
7c8b6e5b5206a98f86675d0107ad99ea1d080466 [lib] add scheduler to node context (Amiti Uttarwar)
930d8375421451c8c4127608c360b0f6a0a62127 [test] add chainparams property to indicate chain allows time mocking (Amiti Uttarwar)
1cd43e83c6e8d81e950aaaede7a8a51505d0a2bc [test] unit test for new MockForward scheduler method (Amiti Uttarwar)
a6f63598adb880a75e1571aac58338c17fa7ad53 [util] allow scheduler to be mocked (Amiti Uttarwar)
Pull request description:
This PR is to support functional tests by allowing the scheduler to be mocked via the RPC.
It adds a `MockForward` method to the scheduler class that iterates through the task queue and reschedules them to be `delta_seconds` sooner.
This is currently used to support functional testing of the "unbroadcast" set tracking in #18038. If this patch is accepted, it would also be useful to simplify the code in #16698.
ACKs for top commit:
MarcoFalke:
ACK 8bca30ea17cd4c1dacee28eaa27e5fa3493b021d, only change is some style fixups š
Tree-SHA512: 2a97fe8ade2b7fd1fb5cdfa1dcafb3227a377d7a847e3845a228bc119eb77824b4aefa43d922a06d583939b22725e223f308cf092961048079d36f6b1d9a639b
|
|
- also update test setup & access point in denial of service test
|
|
|
|
c86bc144081f960347232546f7d22deb65d27deb Make asmap Interpret tolerant of malicious map data (Pieter Wuille)
38c2395d7a905c87dc4630031849fd8e403e61bf Use ASNs for mapped IPv4 addresses correctly (Pieter Wuille)
6f8c93731203c111f86c39eaf2102f9a825d1706 Mark asmap const in statistics code (Pieter Wuille)
d58bcdc4b569a667b6974c3547b7ff6f665afce9 Avoid asmap copies in initialization (Pieter Wuille)
Pull request description:
Here are a few things to improve in the asmap implementation. The first two commits are just code improvements. The last one is a bugfix (the exsting code wouldn't correctly apply ASN lookups to mapped/embedded IPv4 addresses).
ACKs for top commit:
practicalswift:
ACK c86bc144081f960347232546f7d22deb65d27deb -- patch looks correct
naumenkogs:
utACK c86bc14
laanwj:
ACK c86bc144081f960347232546f7d22deb65d27deb
jonatack:
ACK c86bc144081f960347232546f7d22deb65d27deb code looks correct, built/ran tests, bitcoind with -asmap pointed to asmap/demo.map
Tree-SHA512: 1036f43152754d621bfbecfd3b7c7276e4670598fcaed42a3d275e51fa2cf3653e2c9e9cfa714f6c7719362541510e92171e076ac4169b55a0cc8908b2d514c0
|
|
|
|
|
|
3c1bc40205a3fcab606e70b0e3c13d68b2860e34 Add extra logging of asmap use and bucketing (Gleb Naumenko)
e4658aa8eaf1629dd5af8cf7b9717a8e72028251 Return mapped AS in RPC call getpeerinfo (Gleb Naumenko)
ec45646de9e62b3d42c85716bfeb06d8f2b507dc Integrate ASN bucketing in Addrman and add tests (Gleb Naumenko)
8feb4e4b667361bf23344149c01594abebd56fdb Add asmap utility which queries a mapping (Gleb Naumenko)
Pull request description:
This PR attempts to solve the problem explained in #16599.
A particular attack which encouraged us to work on this issue is explained here [[Erebus Attack against Bitcoin Peer-to-Peer Network](https://erebus-attack.comp.nus.edu.sg/)] (by @muoitranduc)
Instead of relying on /16 prefix to diversify the connections every node creates, we would instead rely on the (ip -> ASN) mapping, if this mapping is provided.
A .map file can be created by every user independently based on a router dump, or provided along with the Bitcoin release. Currently we use the python scripts written by @sipa to create a .map file, which is no larger than 2MB (awesome!).
Here I suggest adding a field to peers.dat which would represent a hash of asmap file used while serializing addrman (or 0 for /16 prefix legacy approach).
In this case, every time the file is updated (or grouping method changed), all buckets will be re-computed.
I believe that alternative selective re-bucketing for only updated ranges would require substantial changes.
TODO:
- ~~more unit tests~~
- ~~find a way to test the code without including >1 MB mapping file in the repo.~~
- find a way to check that mapping file is not corrupted (checksum?)
- comments and separate tests for asmap.cpp
- make python code for .map generation public
- figure out asmap distribution (?)
~Interesting corner case: Iām using std::hash to compute a fingerprint of asmap, and std::hash returns size_t. I guess if a user updates the OS to 64-bit, then the hash of asap will change? Does it even matter?~
ACKs for top commit:
laanwj:
re-ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34
jamesob:
ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34 ([`jamesob/ackr/16702.3.naumenkogs.p2p_supplying_and_using`](https://github.com/jamesob/bitcoin/tree/ackr/16702.3.naumenkogs.p2p_supplying_and_using))
jonatack:
ACK 3c1bc40205a3fcab606e70b0e3c13d68b2860e34
Tree-SHA512: e2dc6171188d5cdc2ab2c022fa49ed73a14a0acb8ae4c5ffa970172a0365942a249ad3d57e5fb134bc156a3492662c983f74bd21e78d316629dcadf71576800c
|
|
|
|
characters. Add tests.
7a046cdc1423963bdcbcf9bb98560af61fa90b37 tests: Avoid using C-style NUL-terminated strings as arguments (practicalswift)
fefb9165f23fe9d10ad092ec31715f906e0d2ee7 tests: Add tests to make sure lookup methods fail on std::string parameters with embedded NUL characters (practicalswift)
9574de86ad703ad942cdd0eca79f48c0d42b102b net: Avoid using C-style NUL-terminated strings as arguments in the netbase interface (practicalswift)
Pull request description:
Don't allow resolving of `std::string`:s with embedded `NUL` characters.
Avoid using C-style `NUL`-terminated strings as arguments in the `netbase` interface
Add tests.
The only place in where C-style `NUL`-terminated strings are actually needed is here:
```diff
+ if (!ValidAsCString(name)) {
+ return false;
+ }
...
- int nErr = getaddrinfo(pszName, nullptr, &aiHint, &aiRes);
+ int nErr = getaddrinfo(name.c_str(), nullptr, &aiHint, &aiRes);
if (nErr)
return false;
```
Interface changes:
```diff
-bool LookupHost(const char *pszName, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup);
+bool LookupHost(const std::string& name, std::vector<CNetAddr>& vIP, unsigned int nMaxSolutions, bool fAllowLookup);
-bool LookupHost(const char *pszName, CNetAddr& addr, bool fAllowLookup);
+bool LookupHost(const std::string& name, CNetAddr& addr, bool fAllowLookup);
-bool Lookup(const char *pszName, CService& addr, int portDefault, bool fAllowLookup);
+bool Lookup(const std::string& name, CService& addr, int portDefault, bool fAllowLookup);
-bool Lookup(const char *pszName, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions);
+bool Lookup(const std::string& name, std::vector<CService>& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions);
-bool LookupSubNet(const char *pszName, CSubNet& subnet);
+bool LookupSubNet(const std::string& strSubnet, CSubNet& subnet);
-CService LookupNumeric(const char *pszName, int portDefault = 0);
+CService LookupNumeric(const std::string& name, int portDefault = 0);
-bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed);
+bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocketRet, int nTimeout, bool& outProxyConnectionFailed);
```
It should be noted that the `ConnectThroughProxy` change (from `bool *outProxyConnectionFailed` to `bool& outProxyConnectionFailed`) has nothing to do with `NUL` handling but I thought it was worth doing when touching this file :)
ACKs for top commit:
EthanHeilman:
ACK 7a046cdc1423963bdcbcf9bb98560af61fa90b37
laanwj:
ACK 7a046cdc1423963bdcbcf9bb98560af61fa90b37
Tree-SHA512: 66556e290db996917b54091acd591df221f72230f6b9f6b167b9195ee870ebef6e26f4cda2f6f54d00e1c362e1743bf56785d0de7cae854e6bf7d26f6caccaba
|
|
9dd58ca611f6f2b59c25d727a4e955333525d345 init: Stop indexes on shutdown after ChainStateFlushed callback. (Jim Posen)
Pull request description:
Replaces https://github.com/bitcoin/bitcoin/pull/17852.
Currently, the latest index state may not be committed to disk on shutdown. The state is committed on `ChainStateFlushed` callbacks and the current init order unregisters the indexes as validation interfaces before the final `ChainStateFlushed` callback is called on them.
Issue identified by paulyc.
For review: an alternative or supplemental solution would be to call `Commit` at the end of `BaseIndex::Stop`. I don't see any harm in doing so and it makes the less prone to user error. However, the destructor would have to be modified to not call `Stop` because `Commit` calls a virtual method, so I figured it wasn't worth it. But I'm curious how others feel.
ACKs for top commit:
fjahr:
tested ACK 9dd58ca611f6f2b59c25d727a4e955333525d345
paulyc:
> Code review ACK [9dd58ca](https://github.com/bitcoin/bitcoin/commit/9dd58ca611f6f2b59c25d727a4e955333525d345), but failed to test because I can't reproduce the original problem.
kallewoof:
Tested ACK 9dd58ca611f6f2b59c25d727a4e955333525d345
promag:
Code review ACK 9dd58ca611f6f2b59c25d727a4e955333525d345, but failed to test because I can't reproduce the original problem.
Tree-SHA512: 2918380b699833cb7eab07456d1667dbf8ebbe2d2b5988300a3cf5b6a6cfc818b6d9086e1936ffe7881f67e409306c4b91d61a08a169cfd0a301383479d4f3cb
|
|
3d5d7aad269c7afe7e36677d3e76c6579e1b8aba windows: remove call to SetProcessDEPPolicy (fanquake)
f2645c26017591f819344d24dc0a88dc32dde6fd windows: Enable heap terminate-on-corruption (fanquake)
Pull request description:
This PR is currently two separate changes:
#### Enable heap terminate-on-corruption
This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc.
> Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process.
After a process enables this feature, it cannot be disabled.
More info [here](https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation).
#### Remove call to SetProcessDEPPolicy()
DEP is always enabled on 64-bit Windows processes, and `SetProcessDEPPolicy()` only works when called from a 32-bit process. I've tested that our current usage always fails ([as expected](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy#remarks)) with [ERROR_NOT_SUPPORTED](https://github.com/mirror/mingw-w64/blob/16151c441e89081fd398270bb888511ebef6fb35/mingw-w64-headers/include/error.h#L42).
Please don't add a "Needs gitian build" tag here yet.
ACKs for top commit:
sipsorcery:
ACK 3d5d7aad269c7afe7e36677d3e76c6579e1b8aba.
laanwj:
ACK 3d5d7aad269c7afe7e36677d3e76c6579e1b8aba
Tree-SHA512: 0948bcf165685b6b573f2cd950680c34356b856690de655ced2b93d497e02e7b22aa195c99f6ce33202f182622c67302ff31c98ab51b7d050574af3debdee5ce
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-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-
|
|
SetProcessDEPPolicy() is only supported by 32-bit Windows processes. On 64-bit
Windows it fails with ERROR_NOT_SUPPORTED (DEP is always enabled on 64-bit).
More info:
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy
|
|
This is default behavior from Windows 8 onwards, however we still support
Windows 7, so it should make sense to explicitly enable this.
More info:
https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation
|
|
Currently, the latest index state may not be committed to disk on shutdown.
|
|
interface
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
|
|
Instead of using /16 netgroups to bucket nodes in Addrman for connection
diversification, ASN, which better represents an actor in terms
of network-layer infrastructure, is used.
For testing, asmap.raw is used. It represents a minimal
asmap needed for testing purposes.
|
|
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
|
|
d94d34f05f4ae3efa07de409489d68bbcc216346 doc: update developer notes wrt unix epoch time (Jon Atack)
e2f32cb5c5c7f2b1d1fc7003587b6573fb59526a qa: unify unix epoch time descriptions (Jon Atack)
Pull request description:
Closes #17613.
Updated call sites: mocktime, getblockheader, getblock, pruneblockchain,
getchaintxstats, getblocktemplate, setmocktime, getpeerinfo, setban,
getnodeaddresses, getrawtransaction, importmulti, listtransactions,
listsinceblock, gettransaction, getwalletinfo, getaddressinfo
Commands for testing manually:
```
bitcoind -help-debug | grep -A1 mocktime
bitcoin-cli help getblockheader
bitcoin-cli help getblock
bitcoin-cli help pruneblockchain
bitcoin-cli help getchaintxstats
bitcoin-cli help getblocktemplate
bitcoin-cli help setmocktime
bitcoin-cli help getpeerinfo
bitcoin-cli help setban
bitcoin-cli help getnodeaddresses
bitcoin-cli help getrawtransaction
bitcoin-cli help importmulti
bitcoin-cli help listtransactions
bitcoin-cli help listsinceblock
bitcoin-cli help gettransaction
bitcoin-cli help getwalletinfo
bitcoin-cli help getaddressinfo
```
ACKs for top commit:
laanwj:
re-ACK d94d34f05f4ae3efa07de409489d68bbcc216346
Tree-SHA512: 060713ea4e20ab72c580f06c5c7e3ef344ad9c2c9cb034987d980a54e3ed2ac0268eb3929806daa5caa7797c45f5305254fd499767db7f22862212cf77acf236
|
|
to "UNIX epoch time".
Call sites updated:
```
mocktime
getblockheader
getblock
pruneblockchain
getchaintxstats
getblocktemplate
setmocktime
getpeerinfo
setban
getnodeaddresses
getrawtransaction
importmulti
listtransactions
listsinceblock
gettransaction
getwalletinfo
getaddressinfo
```
|
|
|
|
|
|
This commit fires TransactionRemovedFromMempool callbacks from the
mempool and cleans up a bunch of code.
|
|
fa538813b1c382cf135cbf2a0cc3fa01f36964d8 scripted-diff: Replace ::mempool with m_node.mempool in tests (MarcoFalke)
8888ad02e204b0fa7a2ea2cfed2fc3f298cf1623 test: Replace recursive lock with locking annotations (MarcoFalke)
fac07f2038a3ccd5edadc6e6122c02fa30e697bd node: Add reference to mempool in NodeContext (MarcoFalke)
Pull request description:
This is the first step toward making the mempool a global that is not initialized before main.
#### Motivation
Currently the mempool is a global that is initialized before the `main` function. This is confusing and easy to get wrong. E.g. the mempool constructor queries state that has not been initialized, like randomness (fixed), or command line arguments (not an issue last time I checked). Also without having the chainstate (chain tip) initialized first, it doesn't make conceptually sense to have a mempool, since the mempool builds txs on top of the utxo set (chain tip).
Finally, in the future someone might want to run a consensus-only full node (`-nowallet -noblockfilter -no... -nomempool` command line options) that only verifies blocks and updates the utxo set.
This is conceptually the same change that has already been done for the connection manager `CConnman`.
ACKs for top commit:
jnewbery:
utACK fa538813b1c382cf135cbf2a0cc3fa01f36964d8
ariard:
Tested ACK fa53881.
Tree-SHA512: 2c446a8a51476354aad7126c2b833500d36b24490caa94f847b2bdc622054de0dae28980f23e3d91b1b492dc32931656d98dbd019af9e4e58f2f8c5375aac694
|
|
|
