d117f4541d4717e83c9396273e92960723622030 Add test for setban (nicolas.dorier)
dc7529abf0197dccb876dc4a93cbdd2ad9f03e5c [Fix] Allow connection of a noban banned peer (nicolas.dorier)
Pull request description:
Reported by @MarcoFalke on https://github.com/bitcoin/bitcoin/pull/16248#discussion_r314026195
The bug would mean that if the peer connecting to you is banned, but whitelisted without specific permissions, it would not be able to connect to the node.
The solution is just to move the same line below.
ACKs for top commit:
Sjors:
Agree inline is more clear. utACK d117f45
MarcoFalke:
ACK d117f4541d4717e83c9396273e92960723622030
Tree-SHA512: 0fed39acb1e8db67bb0bf4c4de3ad034ae776f38d55bd661f1ae0e1a4c6becaf1824ab46ed8279f2f31df3f4b29ff56461d8b167d3e9cece62cfe58b5a912811
c5b404e8f1973afe071a07c63ba1038eefe13f0f Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa391844f658bd7035659b5b16695733dd56 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4c3644a30092100ffc399e30e193275 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa6842f7dd7c4537ede000f965ea0189 Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1973afe071a07c63ba1038eefe13f0f
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
7257353b93 Select orphan transaction uniformly for eviction (Pieter Wuille)
Pull request description:
The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.
Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>
eea02be70e Add locking annotation for vNodes. vNodes is guarded by cs_vNodes. (practicalswift)
Pull request description:
Add locking annotation for `vNodes`. `vNodes` is guarded by `cs_vNodes`.
Tree-SHA512: b1e18be22ba5b9dd153536380321b09b30a75a20575f975af9af94164f51982b32267ba0994e77c801513b59da05d923a974a9d2dfebdac48024c4bda98b53af
0297be61a Allow connections from misbehavior banned peers. (Gregory Maxwell)
Pull request description:
This allows incoming connections from peers which are only banned
due to an automatic misbehavior ban if doing so won't fill inbound.
These peers are preferred for eviction when inbound fills, but may
still be kept if they fall into the protected classes. This
eviction preference lasts the entire life of the connection even
if the ban expires.
If they misbehave again they'll still get disconnected.
The main purpose of banning on misbehavior is to prevent our
connections from being wasted on unhelpful peers such as ones
running incompatible consensus rules. For inbound peers this
can be better accomplished with eviction preferences.
A secondary purpose was to reduce resource waste from repeated
abuse but virtually any attacker can get a nearly unlimited
supply of addresses, so disconnection is about the best we can
do.
This can reduce the potential from negative impact due to incorrect misbehaviour bans.
Tree-SHA512: 03bc8ec8bae365cc437daf70000c8f2edc512e37db821bc4e0fafa6cf56cc185e9ab40453aa02445f48d6a2e3e7268767ca2017655aca5383108416f1e2cf20f
b6a253337f6371e4aa27c488ad70741d2b750d01 Remove redundant BIP174 test from rpc_psbt.json (araspitzu)
Pull request description:
There was a duplicate test for SIGNER role inside 'test/functional/data/rpc_psbt.json', namely test number 2 was equal to test number 3 in the array of data for 'signer'. This pull request removes the 3rd (redundant) test.
Tree-SHA512: e2128c93183f2e0acf5247274397c77a962accf95dee3bb6f785494cf3080a3f28ea47d8209e36b3064490c821690d1742c22e0d76370cb1688dcb2ab91d8f57
65a449f8e3 Explain when reindex-chainstate can be used instead of reindex (Sjors Provoost)
Pull request description:
Save users from having to Google this: https://bitcoin.stackexchange.com/a/60711
Tree-SHA512: 3128565d037c77265a2ecf3bce137b8d27740f513802a4e683be06f21a75b82ee6cc22eb903181c4f438a2990cb682ce1d076f4d3af33d5aaa79b783a9f664b1
ed2332aeffb071a3404be9cff8f9fb8a81a9fbfb test: Add test for config file parsing errors (MarcoFalke)
a66c0f78a941968340f030911765a84219908c4d util: Report parse errors in configuration file (Wladimir J. van der Laan)
Pull request description:
Report errors while parsing the configuration file, instead of silently ignoring them.
$ src/bitcoind -regtest
Error reading configuration file: parse error on line 22: nodebuglogfile, if you intended to specify a negated option, use nodebuglogfile=1 instead
$ src/bitcoind -regtest
Error reading configuration file: parse error on line 22: sdafsdfafs
$ src/bitcoind -regtest
Error reading configuration file: parse error on line 24: -nodebuglogfile=1, options in the configuration file must be specified without leading -
(inspired by https://github.com/bitcoin/bitcoin/pull/14100#issuecomment-417264823)
Tree-SHA512: d516342b65db2969edf200390994bbbda23654c648f85dcc99f9f2d217d3d59a72e0f58227be7b4746529dcfa54ba26d8188ba9f14a57c9ab00015d7283fade2
f34c8c466a0e514edac2e8683127b4176ad5d321 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)
Pull request description:
Make objects in range declarations immutable by default.
Rationale:
* Immutable objects are easier to reason about.
* Prevents accidental or hard-to-notice change of value.
Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
8ecaee13f70a2062e88a977c950a65d3a2de560f Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4 (12 is assuming the changes in #14086 are also implemented). (practicalswift)
Pull request description:
Remove unreferenced local variables:
Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4. 12 is the number of MSVC warnings under our current appveyor setup assuming the changes in #14086 are also implemented.
This makes it easier to spot errors or more important warnings in the verbose appveyor output. MSVC warnings are good, so having access to them in a noise free way (read: without trivial warnings) via appveyor without having to use Windows is really valuable.
See https://github.com/bitcoin/bitcoin/pull/14086#issuecomment-416610313 plus discussion for context.
Before:
```
c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
c:\projects\bitcoin\src\rest.cpp(467): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(511): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(524): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(722): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(783): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\dbwrapper_tests.cpp(265): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\net_tests.cpp(118): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\net_tests.cpp(151): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
```
After:
```
c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
```
Tree-SHA512: 5051134126c570b8421d57c710f1f1b977600398d2b5e69f8a8bd766b3696f992bf4e3459643b99a6b7e08dee1adc92985ee4d0d52b20755954415cb6f23f2fb
8dfc2f30dea6bde0f74d23691377f248966011ab Test rpc_help.py failed: Check whether ZMQ is enabled or not. (Kvaciral)
Pull request description:
/test/functional/rpc_help.py checks for the zmq-category even while zmq may be disabled (in /test/config.ini) , I have added a check function to test_framework.py that can be used whether to determine to include zmq in a test or not.
Tree-SHA512: 6819050277e2dc875f8d9bf49a02291555cb7b301379dfb9d898e6d8e14bfb8eeb6bef8af46d07b5db45b2fe281b35ea7f98af9ffba703768658a69addbc81b1
19efc01aec6b0d8750413fa1b721e04aaecf8f73 Add PSBT documentation (Pieter Wuille)
Pull request description:
This is just some initial text to get going; other contributions welcome.
I'd like to include other workflows, such as hardware wallets and (manual) coinjoins. However, the former will in practice require PSBT interfaces for existing hardware devices, and the second can really use some extra RPCs first.
Tree-SHA512: 951e475e31bb2ea9ab5d84d139b8bc436153ad035185f00ad1d56afc0c6f7c4de8176a785a6d0c38bb3fd9cbf318e513e1a032e83e1da99ded5d43a36f9cbc60
1e94a2bcbc5ff8ae61eed9f31317ea534649116d depends: Add --sysroot option to mac os native compile flags (Russell Yanofsky)
Pull request description:
Catalina SDK clang stopped automatically searching the SDK include paths when invoked without `--sysroot`:
- https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-594600985
- https://github.com/Homebrew/homebrew-core/issues/45061
This hasn't been a problem for current native depends packages because are passing their own `--sysroot` values, and hasn't been a problem for current host packages because they use `darwin_` commands instead of `build_darwin_` commands. But the current `build_darwin_CC` and `build_darwin_CXX` commands are still unnecessarily fragile, and incompatible with new native depends packages added in https://github.com/bitcoin/bitcoin/pull/18677.
Cory Fields (theuni) suggested in https://github.com/bitcoin/bitcoin/pull/16367#issuecomment-595393546 switching compiler from SDK clang to native clang (from $PATH) to avoid this problem. This is easy and makes a certain amount of sense for building native packages, as opposed to host packages. But Michael (fanquake) pointed out in https://github.com/bitcoin/bitcoin/pull/18677#discussion_r409934309 that it would be inconsistent to switch to non-SDK compilers while still using other SDK tools like `ranlib` and `install_name_tool`. So simplest, minimal fix seems to be just adding the missing `--sysroot` option.
ACKs for top commit:
ryanofsky:
> ACK [1e94a2b](1e94a2bcbc) - I think this change is ok, and I prefer it to the previous patch.
fanquake:
ACK 1e94a2bcbc5ff8ae61eed9f31317ea534649116d - I think this change is ok, and I prefer it to the previous patch. Thanks for the summary in the PR description. I played around with Xcode and the CLT; I think previously I didn't fully grok the slight differences between the two.
Tree-SHA512: 4d4bbb7f49acb76d934a872a15b4e14f36290b508cb9e728815f959767ec174bcfb6d2ca7dcd995cc550d86980d64d4247ea5ecfca2301f0953006e50744fdb4
d10f2cd7d8 travis: set codespell version to avoid breakage (MarcoFalke)
Pull request description:
codespell changes the behaviour across version, so just hardcode the version to avoid warnings when they bump the version.
Tree-SHA512: f1a4d078d28c088cf0036f5160bfd79d5e4ccda061e535c066a74cd3d54d41b43ffa4df87a00d1941957ce0b5b60cb69135f6eee85e3e38bd496808fdf36743f
18185b57c32d0a43afeca4c125b9352c692923e9 scripted-diff: batch-recase BanMan variables (Carl Dong)
c2e04d37f3841d109c1fe60693f9622e2836cc29 banman: Add, use CBanEntry ctor that takes ban reason (Carl Dong)
1ffa4ce27d4ea6c1067d8984455df97994c7713e banman: reformulate nBanUtil calculation (Carl Dong)
daae598feb034f2f56e0b00ecfb4854d693d3641 banman: add thread annotations and mark members const where possible (Cory Fields)
84fc3fbd0304a7d6e660bf783c84bed2dd415141 scripted-diff: batch-rename BanMan members (Cory Fields)
af3503d903b1a608cd212e2d74b274103199078c net: move BanMan to its own files (Cory Fields)
d0469b2e9386a7a4b268cb9725347e7517acace6 banman: pass in default ban time as a parameter (Cory Fields)
2e56702ecedd83c4b7cb8de9de5c437c8c08e645 banman: pass the banfile path in (Cory Fields)
4c0d961eb0d7825a1e6f8389d7f5545114ee18c6 banman: create and split out banman (Cory Fields)
83c1ea2e5e66b8a83072e3d5ad6a4ced406eb1ba net: split up addresses/ban dumps in preparation for moving them (Cory Fields)
136bd7926c72659dd277a7b795ea17f72e523338 tests: remove member connman/peerLogic in TestingSetup (Cory Fields)
7cc2b9f6786f9bc33853220551eed33ca6b7b7b2 net: Break disconnecting out of Ban() (Cory Fields)
Pull request description:
**Old English à la Beowulf**
```
Banman wæs bréme --blaéd wíde sprang--
Connmanes eafera Coreum in.
aéglaéca léodum forstandan
Swá bealdode bearn Connmanes
guma gúðum cúð gódum daédum·
dréah æfter dóme· nealles druncne slóg
```
**Modern English Translation**
```
Banman was famed --his renown spread wide--
Conman's hier, in Core-land.
against the evil creature defend the people
Thus he was bold, the son of Connman
man famed in war, for good deeds;
he led his life for glory, never, having drunk, slew
```
--
With @theuni's blessing, here is Banman, rebased. Original PR: https://github.com/bitcoin/bitcoin/pull/11457
--
Followup PRs:
1. Give `CNode` a `Disconnect` method ([source](https://github.com/bitcoin/bitcoin/pull/14605#discussion_r248065847))
2. Add a comment to `std::atomic_bool fDisconnect` in `net.h` that setting this to true will cause the node to be disconnected the next time `DisconnectNodes()` runs ([source](https://github.com/bitcoin/bitcoin/pull/14605#discussion_r248384309))
Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35
d6b076c17bc7d513243711563b262524ef0ba74c Drop IsLimited in favor of IsReachable (Ben Woosley)
Pull request description:
These two methods have had the same meaning, but inverted, since
110b62f069. Having one name for a single
concept simplifies the code.
This is a follow-up to #15051.
/cc #7553
Tree-SHA512: 347ceb9e2a55ea06f4c01226411c7bbcade09dd82130e4c59d0824ecefd960875938022edbe5d4bfdf12b0552c9b4cb78b09a688284d707119571daf4eb371b4
63c74d2d3abcb685c773c3ad1414da6ad062a55d build: Remove illegal spacing in darwin.mk (Jon Layton)
Pull request description:
MacOS 10.13.6, `make -v` = `GNU Make 4.2.1 Built for x86_64-apple-darwin17.7.0`
```
cd depends
make HOST=x86_64-apple-darwin17.7.0
```
Results in error:
```
builders/darwin.mk:1: *** empty variable name. Stop.
```
This seems to fix it.
Tree-SHA512: 3481b9418571186c123942dc95b12bcaf47acbe2099ddd4524f5bb9dfe203175d7252bb7a1aa8ca5fec82578beeacfa06299472cb9d0d627c61f09486d74756c
