ee822d85d6 util: use stronger-guarantee rename method (Vasil Dimov)
Pull request description:
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
This is a rebase of #20435 by vasild.
ACKs for top commit:
MarcoFalke:
review ACK ee822d85d6
hebasto:
Approach ACK ee822d85d6.
vasild:
ACK ee822d85d6
Tree-SHA512: 8f65f154d235c2704f18008d9d40ced3c5d84e4d55653aa70bde345066b6083c84667b5a2f4d69eeaad0bec6c607645e21ddd2bf85617bdec4a2e33752e2059a
In `PeerManagerImpl::RelayAddress()` we used just the hash of the
address that is being relayed to decide where to relay it to. Include
the port in that hash, so that e.g. `1.1.1.1:5555` and `1.1.1.1:6666`
get relayed to different peers. Those are two different, distinct
services after all.
This new constructor will be useful if we just want to hash a `CService`
object without the two `GetRand()` calls (in `RelayAddress()` in a
subsequent commit).
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
0c49e52b22 build: remove unneeded getentropy detection (HAVE_GETENTROPY) (Sebastian Falbesoner)
5cd15ffdce random: use arc4random on OpenBSD (Sebastian Falbesoner)
Pull request description:
Inspired by a discussion on obtaining randomness on various OSes in a secp256k1 PR (https://github.com/bitcoin-core/secp256k1/pull/748#discussion_r524605472, see also https://bitcoincore.reviews/libsecp256k1-748), I think it makes sense to follow best practices and use `arc4random_buf` rather than `getentropy` on OpenBSD in our random module.
The [getentropy(2) man page](https://man.openbsd.org/getentropy.2) states:
```
getentropy() is not intended for regular code; please use the
arc4random(3) family of functions instead.
```
The [arc4random(3) man page](https://man.openbsd.org/arc4random.3) states:
```
Use of these functions is encouraged for almost all random number
consumption because the other interfaces are deficient in either quality,
portability, standardization, or availability.
```
On the linked PR discussion worries about using RC4 internally has been expressed (see https://security.stackexchange.com/questions/85601/is-arc4random-secure-enough/172905#172905), but this would only affect users of OpenBSD <5.5, using a version that was released more than 8 years ago.
ACKs for top commit:
laanwj:
Tested ACK 0c49e52b22
Tree-SHA512: b5ed3d0718962c5a3839db9a28f93d08a0ac93094cc664f83bc4cf1cfad25049e6240b7b81fe06b71e6a3a0ca24a2c337eab088abec5470ad014e10c04fdb216
Use std::filesystem::rename() instead of std::rename(). We rely on the
destination to be overwritten if it exists, but std::rename()'s behavior
is implementation-defined in this case.
fac9fe5d05 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)
Pull request description:
This fixes two issues for strings that start with a colon and only have one colon:
* `fMultiColon` is incorrectly set to `true`
* There is an unsigned integer overflow `colon - 1` (`0 - 1`)
Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.
ACKs for top commit:
laanwj:
Code review ACK fac9fe5d05
shaavan:
Code Review ACK fac9fe5d05
Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
fa2f7d0059 fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)
Pull request description:
`FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.
Though, keep it for now, but avoid the unsigned integer overflow with this patch.
ACKs for top commit:
laanwj:
Code review ACK fa2f7d0059
Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280
fa1b227a72 Remove broken and unused CDataStream methods (MarcoFalke)
faee5f8dc2 test: Create fresh CDataStream each time (MarcoFalke)
fa71114926 test: Inline expected_xor (MarcoFalke)
Pull request description:
The `insert` and `erase` methods have many issues:
* They are unused
* They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed
* They are broken (See https://github.com/bitcoin/bitcoin/pull/24231)
* Fixing them leads to mingw compile errors (See https://github.com/bitcoin/bitcoin/pull/24231#issuecomment-1029286985)
Fix all issues by removing them
ACKs for top commit:
laanwj:
Code review ACK fa1b227a72
Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a
fac62056b5 Fix integer sanitizer suppressions in validation.cpp (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
Fix it with a refactor and remove the suppression.
ACKs for top commit:
hebasto:
ACK fac62056b5, I have reviewed the code and it looks OK, I agree it can be merged.
prayank23:
Code Review ACK fac62056b5
Tree-SHA512: efc5b9887cb2e207033b264ebf425bae5ff013e909701c049aea5d79a21f10495826e962d171b3d412717cbf0a4723e5124133b5401b35a73915212e85e91020
aeb18b665c Bugfix: GUI: Check validity when QValidatedLineEdit::setText is called (Luke Dashjr)
b1a544be10 Bugfix: GUI: Re-check validity after QValidatedLineEdit::setCheckValidator (Luke Dashjr)
2385b508d5 Bugfix: GUI: Only apply invalid style to QValidatedLineEdit, not its tooltip (Luke Dashjr)
Pull request description:
1. Use a CSS selector to avoid changing the background colour of the tooltip.
2. Re-check validity of input when we first set the validator (probably a no-op in practice).
3. Check validity of input when it is set programmatically via `setText` (eg, via the address book). Probably no-op in practice UNTIL merging https://github.com/bitcoin/bitcoin/pull/15987 or any other PR that adds a warning for valid addresses.
Moved from https://github.com/bitcoin/bitcoin/pull/18133 (just concept ACKs)
ACKs for top commit:
Sjors:
tACK aeb18b665c
hebasto:
ACK aeb18b665c, tested on Linux Mint 20.3 (Qt 5.12.8).
Tree-SHA512: b6fa8ee4dec76e1c759095721240e6fa5071a02993cb28406e96a0fa2e819f5dddc03d2e7c9073354d7865c2b09eb263afaf853ecba42e9fc4f50ef4ae20bf0f
99de8068cd validation: use stronger EXCLUSIVE_LOCKS_REQUIRED() (Vasil Dimov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/24103 added annotations to
denote that the callers of `CChainState::ActivateBestChain()` and
`CChainState::InvalidateBlock()` must not own `m_chainstate_mutex` at
the time of the call.
Replace the added `LOCKS_EXCLUDED()` with a stronger
`EXCLUSIVE_LOCKS_REQUIRED()`, see
https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative for the
difference between both.
ACKs for top commit:
hebasto:
ACK 99de8068cd.
jonatack:
ACK 99de8068cd. Tested with Debian clang version 13.0.1. Reproduced hebasto's results. Verified that `LoadExternalBlockFile()` needs the annotation added here.
Tree-SHA512: 59640d9ad472cdb5066ecde89cc0aff8632a351fc030f39bb43800d2c856fb1aed3576e4134212d32be161b18780f06dc5066ac71df7f7cd69e3f21f886e1542
b9c113af75 util: Avoid buggy std::filesystem:::create_directories() call (Hennadii Stepanov)
Pull request description:
Compiled with some libstdc++ versions (e.g., on Ubuntu 20.04) [`std::filesystem:::create_directories()`](https://en.cppreference.com/w/cpp/filesystem/create_directory) call [fails](https://github.com/bitcoin/bitcoin/issues/24257#issue-1123753243) to handle symbol links properly.
No behavior change in comparison to the [pre-20744](c194293883) master branch.
Fixes bitcoin/bitcoin#24257.
ACKs for top commit:
ryanofsky:
Code review ACK b9c113af75. Nice simplification and fix
MarcoFalke:
review ACK b9c113af75🐬
Tree-SHA512: 79d940cfc1f68d9b0548fb2ab005e90850b54ac0fb3bb2940afd632d56288d92687579a3176bac3fd0ea3d2dae71e26444f8f7bdb87862414c12866ae5e857c4
fadcd03139 Fix unsigned integer overflow in LoadMempool (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
This removes one of the two violations.
This should be a refactor.
ACKs for top commit:
prayank23:
Code Review ACK fadcd03139
Tree-SHA512: 9fb2f3d49008a59cd45b7c17be0c88c04e61183197c11c8176865af5532c8d0c940db49a351dd0fc75e1d7fd8678c3b816d34cfca170dc6b9cf8f37fdf1c8cae
fad81548fa test: Avoid testing negative block heights (MarcoFalke)
Pull request description:
A negative chain height is only used to denote an empty chain, not the height of any block.
So stop testing that and remove a suppression.
ACKs for top commit:
brunoerg:
crACK fad81548fa
Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766
fadc54b79b Fix unsigned integer overflow in tapscript validation weight calculation (MarcoFalke)
Pull request description:
Change the tapscript validation weight constants from uint64_t to int64_t, since the type of m_validation_weight_left is also int64_t. Otherwise this will cause sanitizer warnings.
This should be safe because signed integer overflow isn't expected to happen.
ACKs for top commit:
PastaPastaPasta:
utACK fadc54b79b
theStack:
Code-review ACK fadc54b79b
Tree-SHA512: 7a62d3a84733ab7827e3fa50d83f5493f2481b725c587e986eb2c128a769f023756f3ad964401526e386a847aa630a9f6c43a57d25ce5fd4af0b6bb5e0615528
d216bc8d76 Re-enable walletinit_verify_walletdir_no_trailing2 test disabled in #20744 (Ryan Ofsky)
80cd64e842 Re-enable util_datadir check disabled in #20744 (Ryan Ofsky)
Pull request description:
Reenable some broken tests as discussed https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798651736 and https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798678137
Fix windows test cases broken in #20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests.
It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine.
ACKs for top commit:
hebasto:
ACK d216bc8d76, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2099ddfa1a3ad70f7ac2ff413929414a1851d257b280da25c0f5cefb46fd1372b580a1f1ee5280681a1c16e6031f119185cadd4f7a6121298562cf001f711068
824e1ffa9f bench: Represents paths with fs::path instead of std::string (Ryan Ofsky)
Pull request description:
Suggested https://github.com/bitcoin/bitcoin/pull/20744#issuecomment-1022486215
ACKs for top commit:
fanquake:
untested ACK 824e1ffa9f
hebasto:
ACK 824e1ffa9f, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: 348fc189f30b5ad9a8e49e95e535d2c044462a9d534c3f34d887fbde0c05c41e88e02b4fc340709e6395a1188496a8333eb9e734310aa4c41755ec080e53c06e
This should also fix an init error if a -walletdir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This change passes canonical
paths to fs calls validating the -walletdir path to fix this.
This should also fix an assert error if a -datadir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This fix adds a
path::lexically_normal() call to the failing assert so it passes.
985d85e9a8 Follow Transifex docs to prepare XLIFF source (Hennadii Stepanov)
Pull request description:
This PR is a #21694 follow up.
From the Transifex [docs](https://docs.transifex.com/formats/xliff#how-to-distinguish-between-a-source-file-and-a-translation-file):
> A source file is different than a translation file. The translation file contains \<Target> references, whereas a source file does not.
This PR makes the `qt/locale/bitcoin_en.xlf` source file according to the docs.
ACKs for top commit:
laanwj:
ACK 985d85e9a8
Tree-SHA512: 537ef78f39a12f094b17fc902c5b59c7ae4d27e5fa35fbf6b33386843e4521236dac3275ba26ff2d1c435e53e8942284644d5de67b0b3543cec96cbcd4351607
f7a19ef774 qt,refactor: Use std::chrono in TrafficGraphWidget class (Shashwat)
Pull request description:
The PR is a follow-up to #517
- It addresses the change suggested in [this](https://github.com/bitcoin-core/gui/pull/517#pullrequestreview-850260826) comment.
- This PR changes the type of `msecsPerSample` from **int** to **std::chrono::minutes** and makes other relevant subsequent changes that were limited to the **trafficgraphwidget** file.
ACKs for top commit:
RandyMcMillan:
tACK f7a19ef774
hebasto:
ACK f7a19ef774
promag:
Code review ACK f7a19ef774.
Tree-SHA512: 5094ba894f3051fc99148cb8f408fc6f9d6571188673dcb7bf24366cdfb3eaf6d4e41083685d578ad2a9fbe31cc491a5f3fa9b7c9ab6eb90e4dc1356f89ae18a
04255073bb qt: Update source translations (laanwj)
cf79c56e65 init: Remove confusing '(possible integer overflow?)' from error message (laanwj)
d570a63894 qt: Update transifex resource blob to 23.0 (laanwj)
Pull request description:
- Update translations for 0.23 string freeze
- Update transifex resource blob to 23.0
This is necessary before a 23.0 resource can be created on Transifex.
ACKs for top commit:
hebasto:
ACK 04255073bb
Tree-SHA512: ff886e92721f070e3c135cfec229c41848a67c02355b88f2a5a507241b545f4209167d83b561420c2a82f49a5994170b01dcfb95bfc3fe6b9c832abcc6547b14
