Add more fs::path operator/ and operator+ overloads to prevent unsafe
string->path conversions on Windows that would cause strings to be
decoded according to the current Windows locale & code page instead of
the correct string encoding.
Update application code to deal with loss of implicit string->path
conversions by calling fs::u8path or fs::PathFromString explicitly, or
by just changing variable types from std::string to fs::path to avoid
conversions altoghther, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings
only contained ASCII characters and would be decoded the same regardless
of what encoding was used, or (2) because of the 1:1 mapping between
paths and strings using the PathToString and PathFromString functions.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fad6d4f952 Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb22 Remove duplicate static_asserts (MarcoFalke)
Pull request description:
No need to go from `arith_uint256`->`uint256` when a `uint256` can be constructed right away.
ACKs for top commit:
laanwj:
Code review ACK fad6d4f952
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
3ae7791bca refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bca
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
Add missing includes.
Swap C headers for their C++ counterparts.
Remove pointless / unmaintainable include comments. This is even more the case
when we are actually using IWYU, as if anyone wants to see the comments they can
just get IWYU to generate them.
We inline `UpdatePreferredDownload` because it is only used in one
location during the version handshake. We simplify it by removing the
initial subtraction of `state->fPreferredDownload` from
`nPreferredDownload`. This is ok since the version handshake is only
called once per peer and `state->fPreferredDownload` will always be
false before the newly inlined code is called, making the subtraction a
noop.
f0a2fb3c5d scripted-diff: Rename pindexBestHeader, fHavePruned (Carl Dong)
a401402125 Clear fHavePruned in BlockManager::Unload() (Carl Dong)
3308ecd3fc move-mostly: Make fHavePruned a BlockMan member (Carl Dong)
c96524113c Clear pindexBestHeader in ChainstateManager::Unload() (Carl Dong)
73eedaaacc style-only: Miscellaneous whitespace changes (Carl Dong)
0d567daf23 move-mostly: Make pindexBestHeader a ChainMan member (Carl Dong)
5d670173a3 validation: Load pindexBestHeader in ChainMan (Carl Dong)
Pull request description:
Split off from #22564 per Marco's suggestion: https://github.com/bitcoin/bitcoin/pull/22564#issuecomment-1100011503
This is basically the move-mostly parts of #22564. The overall intent is to move mutable globals manually reset by `::UnloadBlockIndex` into appropriate structs such that they are cleared at the appropriate times. Please read #22564's description for more rationale.
In summary , this PR moves:
1. `pindexBestHeader` -> `ChainstateManager::m_best_header`
2. `fHavePruned` -> `BlockManager::m_have_pruned`
ACKs for top commit:
ajtowns:
ACK f0a2fb3c5d -- code review only
MarcoFalke:
kirby ACK f0a2fb3c5d😋
Tree-SHA512: 8d161701af81af1ff42da1b22a6bef2f8626e8642146bc9c3b27f3a7cd24f4d691910a2392b188ae058fec0611a17304dd73f60da695f53832d327f73d2fc963
-----
Code Reviewer Notes
Call graph of relevant functions:
UnloadBlockIndex() <-- Moved from
calls ChainstateManager::Unload()
which calls BlockManager::Unload() <-- Moved to
So calling UnloadBlockIndex() would still run this moved code. The code
will also now run when ~BlockManager gets called, which makes sense.
-----
Code Reviewer Notes
Call graph of relevant functions:
UnloadBlockIndex() <-- Moved from
calls ChainstateManager::Unload() <-- Moved to
Safe because ChainstateManager::Unload() is called only by
UnloadBlockIndex() and no other callers.
3ec6504a2e qt: Do not use `QKeyEvent` copy constructor (Hennadii Stepanov)
Pull request description:
This PR is preparation for [Qt 6](https://github.com/bitcoin/bitcoin/pull/24798), and it fixes an experimental build with Qt 6.2.4 as copying of `QEvent` has been [disabled](19f9b0d5f5) in Qt 6.0.0.
ACKs for top commit:
w0xlt:
tACK 3ec6504a2e on Ubuntu 21.10, Qt 5.15.2
shaavan:
reACK 3ec6504a2e
Tree-SHA512: 583a9dad0c621d9f02f77ccaa9f55ee79e12e3c47f418911ef2dfe0de357d772d1928ae3ec19b6f0c0674da858bab9d4542a26cc14b06ed921370dfeabd1c194
7417594187 miniscript: the 'd:' wrapper must not be 'u' (Antoine Poinsot)
Pull request description:
The type system was incorrectly relying on a standardness rule to be sound.
This bug was found and reported by Andrew Poelstra [based on a question from Aman Kumar Kashyap](https://github.com/rust-bitcoin/rust-miniscript/discussions/341).
ACKs for top commit:
sipa:
ACK 7417594187
apoelstra:
utACK 7417594187
achow101:
ACK 7417594187
Tree-SHA512: af68c1df1c40e40dd105ef54544c226f560524dd8e35248fa0305dbef966e96ec1fa6ff2fe50fb8f2792ac310761a29c55ea81dd7b6d122a0de0a68b135e5aaa
a2c4a7acd1 net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() (Vasil Dimov)
d65b6c3fb9 net: use Sock::SetSockOpt() instead of setsockopt() (Vasil Dimov)
184e56d668 net: add new method Sock::SetSockOpt() that wraps setsockopt() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add a `virtual` (thus mockable) method `Sock::SetSockOpt()` that wraps the system `setsockopt()`.
Convert the standalone `SetSocketNoDelay()` function to a `virtual` (thus mockable) method `Sock::SetNoDelay()`.
This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.
ACKs for top commit:
laanwj:
Code review ACK a2c4a7acd1
jonatack:
ACK a2c4a7acd1 change since last review is folding `Sock::SetNoDelay()` into the callers
Tree-SHA512: 3e2b016c1e4128317a28c17dc9b30472949e1ac3b071b2697c6d30cbcc830df1ee4392a4e23b2ea1ab4e3fb0f59ef450e2a4f3c1df3d8c803dd081652b6c7387
07ddecb84e refactor: Use [[maybe_unused]] attribute (Hennadii Stepanov)
55e0fc8df9 refactor: Drop unneeded workarounds aimed to silence unused warning (Hennadii Stepanov)
Pull request description:
This change is required for bitcoin/bitcoin#24773 as it prevents MSVC yelling about "warning C4551: function call missing argument list".
But it is useful by itself as it makes code more concise and readable.
ACKs for top commit:
Empact:
Code review ACK 07ddecb84e
laanwj:
Code review ACK 07ddecb84e
vincenzopalazzo:
ACK 07ddecb84e
w0xlt:
ACK 07ddecb
Tree-SHA512: 01791855a9ba742202d5718203303af989fcb501b7cf2a24ac8d78e87487acca38f77bef264b8e27e41ad1ccf96e426725cf65bfd96ce2ac71c46b3792bed857
0000a63689 Simplify GetTime (MarcoFalke)
Pull request description:
The implementation of `GetTime` is confusing:
* The value returned by `GetTime` is assumed to be equal to `GetTime<std::chrono::seconds>()`. Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption.
* On some systems, `time_t` might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas `GetTime<std::chrono::seconds>` does not. Also, `time_t` might be `-1` "on error", where "error" is unspecified.
* `GetTime<std::chrono::seconds>` calls `GetTimeMicros`, which calls `GetSystemTime`, which calls `std::chrono::system_clock::now`, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now
* `GetTimeMicros` and the internal-only `GetSystemTime` will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate `std::chrono::time_point<Clock>` getters.
Fix all issues by:
* making `GetTime()` an alias for `GetTime<std::chrono::seconds>().count()`.
* inlining the needed parts of `GetSystemTime` directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future.
ACKs for top commit:
martinus:
Code review, untested ACK 0000a63689. By the way strictly speaking `std::chrono::system_clock` is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock
theStack:
Code-review ACK 0000a63689
Tree-SHA512: f751ba740e0da65537be800e9414dd02282d9f04c0b0fb986a36546f257d0b888d8688653cdda5d355ec832c0e09d866922d9161b1ccd33485c1c92c5d1e802f
This is constructed before addrman and connman, and destructed afterwards.
netgroupman does not currently do anything, but will have functionality added in future commits.
6f29409ad1 test: Add a test that creates a wallet with invalid parameters (w0xlt)
0359d9b6a3 Change wallet validation order (w0xlt)
Pull request description:
In the current code, the database is created before the last validation, which checks that passphrase is set and private keys are disabled.
Therefore, if this validation fails, it will result in an empty database and the user will not be able to recreate a wallet with the same name and with the correct parameters.
Behavior on the master branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_01"
error code: -4
error message:
Wallet file verification failed. Failed to create database path '/home/w/.bitcoin/regtest/wallets/invalid_wallet'. Database already exists.
```
Behavior on the PR branch:
```
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02" disable_private_keys=true passphrase="passphrase"
error code: -4
error message:
Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled.
$ ./src/bitcoin-cli -regtest -named createwallet wallet_name="invalid_wallet_02"
{
"name": "invalid_wallet_01",
"warning": ""
}
```
ACKs for top commit:
achow101:
ACK 6f29409ad1
Tree-SHA512: d192955fc2285bf27ae5dd4c1b7cfd3d85441a7f3554b189b974aefb319c6b997543991dbb0ca2c8cb980f7058913a77cf0164c02e9b51ceb9c2cb601317c428
The value it leaves on the stack depends on the last element on the
stack. However, we can't make sure this element is OP_1 (which would
give us the 'u' property) without the MINIMALIF rule.
MINIMALIF is only policy for P2WSH, therefore giving 'd:' the 'u'
property breaks consensus soundness: it makes it possible (by consensus
but not policy) for instance to satisfy a thresh() without satisfying
at least k of its subs.
This bug was found and reported by Andrew Poelstra.
3429d67014 init: Prevent -noproxy and -proxy=0 settings from interacting with other settings (Ryan Ofsky)
Pull request description:
Prevent `-noproxy` and `-proxy=0` settings from interacting with `-listen`, `-upnp`, and `-natpmp` settings.
These settings started being handled inconsistently in the `AppInitMain` and `InitParameterInteraction` functions starting in commit baf05075fa from #6272:
baf05075fa/src/init.cpp (L990-L991)baf05075fa/src/init.cpp (L687)
This commit changes both functions to handle proxy arguments the same way so
there are not side effects from specifying a proxy=0 setting.
This change was originally part of #24830 but really is independent and makes more sense as a separate PR
ACKs for top commit:
hebasto:
ACK 3429d67014, tested on Ubuntu 22.04.
Tree-SHA512: c4c6b4aeb3c07321700e974c16fd47a1bd3d469f273a6b308a69638db81c88c4e67208fddc96fcda9c8bd85f3ae22c98ca131c9622895edaa34eb65c194f35db
The `GUIUtil::ObjectInvoke()` template function was a replacement of
the `QMetaObject::invokeMethod()` functor overload which is available
in Qt 5.10+.
No behavior change.
