787df19b09 validation: don't try to invalidate genesis block (Sebastian Falbesoner)
Pull request description:
In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.
Fixes#20914.
ACKs for top commit:
sipa:
ACK 787df19b09. Tested invalidation of generic on regtest.
practicalswift:
Tested ACK 787df19b09
Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
8dd5946c0b add functional test (Larry Ruane)
b5a80fa7e4 util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)
Pull request description:
If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.
ACKs for top commit:
fjahr:
tACK 8dd5946c0b
luke-jr:
utACK 8dd5946c0b (no changes since previous utACK)
hebasto:
re-ACK 8dd5946c0b, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/18335#pullrequestreview-460621350) review.
darosior:
ACK 8dd5946c0b
Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
52dd40a9fe test: add missing netaddress include headers (Jon Atack)
6f09c0f6b5 util: add missing braces and apply clang format to SplitHostPort() (Jon Atack)
2875a764f7 util: add ParseUInt16(), use it in SplitHostPort() (Jon Atack)
6423c8175f p2p, refactor: pass and use uint16_t CService::port as uint16_t (Jon Atack)
Pull request description:
As noticed during review today in https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584873708 of the upcoming I2P network support, `CService::port` is `uint16_t` but is passed around the codebase and into the ctors as `int`, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
ACKs for top commit:
practicalswift:
cr ACK 52dd40a9fe: patch looks correct
MarcoFalke:
cr ACK 52dd40a9fe
vasild:
ACK 52dd40a9fe
Tree-SHA512: 203c1cab3189a206c55ecada77b9548b810281cdc533252b8e3330ae0606b467731c75f730ce9deb07cbaab66facf97e1ffd2051084ff9077cba6750366b0432
Now that we have a reliable way to detect inbound onion peers, this commit
updates our existing eviction protection of 1/4 localhost peers to instead
protect up to 1/4 onion peers (connected via our tor control service), sorted by
longest uptime. Any remaining slots of the 1/4 are then allocated to protect
localhost peers, or 2 localhost peers if no slots remain and 2 or more onion
peers are protected, sorted by longest uptime.
The goal is to avoid penalizing onion peers, due to their higher min ping times
relative to IPv4 and IPv6 peers, and improve our diversity of peer connections.
Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille
for valuable review feedback that shaped the direction.
and an `m_is_onion` struct member to NodeEvictionCandidate and tests.
We'll use these in the peer eviction logic to protect inbound onion peers
in addition to the existing protection of localhost peers.
An unordered set can tell if an element is present in ~O(1) time (constant on
average, worst case linear to the size of the container), which speeds up and
simplifies the lookup in IsEvicted().
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
out of net_tests, because the eviction tests:
- are a different domain of test coverage, with different dependencies
- run more slowly than the net tests
- will be growing in size, in this PR branch and in the future, as eviction
test coverage is improved
to allow deterministic unit testing of the ratio-based peer eviction protection
logic, which protects peers having longer connection times and those connected
via higher-latency networks.
Add documentation.
ea76f4ac7d Doc: Tell howto install clang-format on Debian/Ubuntu (wodry)
Pull request description:
Because only macOS wasy mentioned, I was unsure if this would be a macOS specific tool. I guess Linux is more used than Mac, so Linux guide should be there, too.
ACKs for top commit:
hebasto:
ACK ea76f4ac7d, every system upgrade via clean installation I do the same.
Tree-SHA512: 75c28540e8815cb41f4cf92784b6349978988b679e4deef9ae77ede951f93516ca13ec7b313ab72865b01273e115b49ed2b67cdcd68015af1b643a6186b190dd
Because only macOS wasy mentioned, I was unsure if this would be a macOS specific tool. I guess Linux is more used than Mac, so Linux guide should be there, too.
fa81773243 style-only: Remove whitespace (MarcoFalke)
fae77b9e6d net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)
Pull request description:
* Clarify that "ignoring" really means "disconnect" in the log
* Revive a refactor I took from #13670
ACKs for top commit:
jnewbery:
utACK fa81773243
sipa:
utACK fa81773243
Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
0fc0c00f7a test: Drop unused get_machine function (Hennadii Stepanov)
61a0f8f9cc test: Cleanup test files in test-{security,symbol}-check.py (Hennadii Stepanov)
Pull request description:
1) Test source and executable files are neither ignored by `.gitignore` nor removed by `make clean` and `make distclean`.
2) The `get_machine` function is no longer used since #21255.
ACKs for top commit:
fanquake:
ACK 0fc0c00f7a
Tree-SHA512: ef3fcf22d4a04b6e4f37f748bd4be57e09696d2a77982e26292843cb2a1297789c8325f5c4bdad37d8094fce7765c4cc9ab19809e07471487943361b2b1a252c
fa2a80bf12 refactor: Pass PeerManagerImpl members only once (MarcoFalke)
Pull request description:
Member variables are already passed to methods via `this`, so no need to pass them another time as function parameter.
ACKs for top commit:
jnewbery:
utACK fa2a80bf12
amitiuttarwar:
utACK fa2a80bf12
Tree-SHA512: 1743825c7560cc748235e3db03e4cea02ad1f670f1b898d7757da644f12693ba9bb2d3eb09b64b3d10dd2e68f52dea31e26d5e97bdc013759baa0515d3c7055c
680eb56d82 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f03 [net processing] Move RelayTransaction into PeerManager (John Newbery)
Pull request description:
This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
ACKs for top commit:
ajtowns:
ACK 680eb56d82
Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235
bb3f79fbdc doc: Update libnatpmp info in dependencies.md (Hennadii Stepanov)
1a01a5dc8d doc: Update zlib info in dependencies.md (Hennadii Stepanov)
Pull request description:
Update docs according to the recent changes in the code:
- #21209 (zlib)
- #21376 (libnatpmp)
ACKs for top commit:
fanquake:
ACK bb3f79fbdc - thanks for keeping this updated.
Tree-SHA512: 48350ad07700aa071ad6c34e4c161aaadc050488fc068cf478e9781d632828187962a4384c1b67c2344145a2c00c3e16cddd09259130af8e9e86cd76cd32900d
c180c911b8 doc: revamp macOS build doc (Jarol Rodriguez)
Pull request description:
This PR makes the macOS build-docs more informative and adds in the following information:
- Proper descriptions and delineation of required/optional dependencies
- walk-through of optional dependencies
- configuration walk-through
- various other tidbits of information
This is a part of the efforts done in https://github.com/bitcoin/bitcoin/pull/20601 and https://github.com/bitcoin/bitcoin/pull/20610 to update the docs and introduce some consistency between them.
This update does not add instructions for arm-based M1 Macbooks as I do not have one to test with. It would be nice to have someone follow up with an update containing instructions for arm-based Macs.
**Before/Master:** [render](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md)
**After/PR:** [render](c180c911b8/doc/build-osx.md)
ACKs for top commit:
fanquake:
ACK c180c911b8 - I still think these are getting too verbose and we're duplicating information all over the place; dependencies, configure options, combinations of options etc. However if people are happy to maintain them, I guess it's fine for now, and this revamping has already happened for some of the other build READMEs.
Tree-SHA512: 1440046c723fe80d4158e4a429e3aa8bd93570acb84ad202d5d24c749ab9a89a3aca8b61b49e75e042a4bf4317acd632d3906e1b5808a9052e74209256528b45
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
57ff5a42ab doc: specify minimum HWI version (Sjors Provoost)
03308b2bfa rpc: don't require wallet for enumeratesigners (Sjors Provoost)
Pull request description:
HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0
As of #16546 we already rely on features that are in 2.0 and not in the previous 1.* releases:
* `--chain` param
This shouldn't be a problem, because HWI 2.0 has been released before we release v22.
Misc improvements:
* document that HWI 2.0 is required
* drop wallet requirement for `enumeratesigners`
ACKs for top commit:
achow101:
Code Review ACK 57ff5a42ab
Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.
Does not change behavior.
d09120b7d1 test: give fundraw more time for test_transaction_too_large (Jon Atack)
Pull request description:
to hopefully fix timeouts from a new test added in 48a0319bab of #20536 merged March 8, 2021
seen locally when running via the test runner
```
File "/home/jon/projects/bitcoin/bitcoin/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large
raise JSONRPCException({
test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 30.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
and in the CI like https://bitcoinbuilds.org/index.php?ansilog=28537952-2c92-46f2-9871-8918e5ba2738.log#l2398
```
File "/home/ubuntu/src/test/functional/rpc_fundrawtransaction.py", line 927, in test_transaction_too_large
test_framework.authproxy.JSONRPCException: 'generatetoaddress' RPC took longer than 240.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
```
Top commit has no ACKs.
Tree-SHA512: f11c923439014fe12420f986c640fd301a26282eb41516957d73b9c751087cbae3d0e316f9ccb49bcb424f488540266f70d3f97948633e77c62bd7935df90452
