f64aa9c411 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
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 altogether, 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.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
In previous commits in this patchset, we've made sure that every
Unload/UnloadBlockIndex member function resets its own members, and does
not reach out to globals.
This means that their corresponding classes' default destructors can now
replace them, and do an even more thorough job without the need to be
updated for every new member variable.
Therefore, we can remove them, and also remove UnloadBlockIndex since
that's not used anymore.
Unfortunately, chainstatemanager_loadblockindex relies on
CChainState::UnloadBlockIndex, so that needs to stay for now.
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
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>
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
c848a45101 test: fix connman UB by calling derived constructor (chinggg)
Pull request description:
Hopefully closes #24373 by calling `ConnmanTestMsg` test-constructor to avoid undefined behavior in process_message.cpp after casting `g_setup->m_node.connman`.
Top commit has no ACKs.
Tree-SHA512: c3dce9dcce33614c7b739edf28e416b600ab3d38d16cdb0430490e8ffc9b64aff9292006ae6fe7c636ab0627893bb21f69435893bdfb129a9a865be92baa6f17
691d45fdc8 Add coinstatsindex_unclean_shutdown test (Ryan Ofsky)
eb6cc05da3 index: Commit DB_MUHASH and DB_BEST_BLOCK to disk together (Martin Zumsande)
Pull request description:
Fixes #24076
Coinstatsindex currently writes the MuHash (`DB_MUHASH`) to disk in `CoinStatsIndex::WriteBlock()` and `CoinStatsIndex::ReverseBlock()`, but the best synced block is written in `BaseIndex::Commit()`. These are called at different points in time, both during the ThreadSync phase, and also after the initial sync is finished and validation callbacks (`BlockConnected()` vs `ChainStateFlushed()`) perform the syncing.
As a result, the index DB is temporarily in an inconsistent state, and if bitcoind is terminated uncleanly (so that there is no time to call `Commit()` by receiving an interrupt or by flushing the chainstate) this leads to problems:
On the next startup, `Init()` will read the best block and a MuHash that corresponds to a different (higher) block. Indexing will be picked up at the the best block processing some blocks again, but since MuHash is a rolling hash, it will process some utxos twice and the muhashes for all future blocks will be wrong, as was observed in #24076.
Fix this by always committing `DB_MUHASH` together with `DB_BEST_BLOCK`.
Note that the block data for the index is still written at different times, but this does not corrupt the index - at worst, these entries will be processed another time and overwritten after an unclean shutdown and restart.
ACKs for top commit:
ryanofsky:
Code review ACK 691d45fdc8. Only change since last review is adding test
fjahr:
ACK 691d45fdc8
Tree-SHA512: e1c3b5f06fa4baacd1b070abb0f8111fe2ea4a001ca8b8bf892e96597cf8b5d5ea10fa8fb837cfbf46648f052c742d912add4ce26d4406294fc5fc20809a0e1b
Make it possible to override from the command line (without recompiling)
the addrman check ratio in the common `TestingSetup::m_node::addrman`
(used by all unit tests) instead of hardcoding it to 0:
```
test_bitcoin --run_test="transaction_tests/tx_valid" -- -checkaddrman=1
```
Retrieve the command line arguments from boost and pass them to
`BasicTestingSetup` so that we gain extra flexibility of passing any
config options on the test command line, e.g.:
```
test_bitcoin -- -printtoconsole=1 -checkaddrman=5
```
e3544c864e init: Use clang-tidy named args syntax (Carl Dong)
3401630417 style-only: Rename *Chainstate return values (Carl Dong)
1dd582782d docs: Make LoadChainstate comment more accurate (Carl Dong)
6b83576388 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong)
Pull request description:
There are 2 proposed fixups in discussions in #23280 which I have not implemented:
1. An overhaul to return types and an option type for the two `*Chainstate` functions: https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-984149564
- The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR.
2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: https://github.com/bitcoin/bitcoin/pull/23280#discussion_r765051533
- I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct.
If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one!
ACKs for top commit:
ryanofsky:
Code review ACK e3544c864e
MarcoFalke:
ACK e3544c864e🐸
Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904
6bf6e9fd9d net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85a net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9d
jamesob:
ACK 6bf6e9fd9d ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9d per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
826e12b010 test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne)
Pull request description:
for additional coverage and similarity to actual init process.
Followup to #23280.
ACKs for top commit:
dongcarl:
Code Review ACK 826e12b010
ryanofsky:
Code review ACK 826e12b010
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
7f15eff2dd style-only: Remove redundant scope in *Chainstate (Carl Dong)
89bec827fd Collapse the 2 cs_main locks in LoadChainstate (Carl Dong)
3b1584b794 Remove all #include // for * comments (Carl Dong)
9a5a5a3d08 test/setup: Use LoadChainstate (Carl Dong)
c541da0d62 node/chainstate: Add options for in-memory DBs (Carl Dong)
ceb9790341 node/caches: Remove intermediate variables (Carl Dong)
ac4bf138b8 node/caches: Extract cache calculation logic (Carl Dong)
15f2e33bb3 validation: VerifyDB only needs Consensus::Params (Carl Dong)
4da9c076d1 node/chainstate: Decouple from ShutdownRequested (Carl Dong)
05441c2dc5 node/chainstate: Decouple from GetTime (Carl Dong)
2414ebc18b init: Delay RPC block notif until warmup finished (Carl Dong)
8d466a8504 Move -checkblocks LogPrintf to AppInitMain (Carl Dong)
aad8d59789 node/chainstate: Reduce coupling of LogPrintf (Carl Dong)
b345979a2b node/chainstate: Decouple from concept of uiInterface (Carl Dong)
ca7c0b934d Split off VerifyLoadedChainstate (Carl Dong)
adf4912d77 node/chainstate: Remove do/while loop (Carl Dong)
975235ca0a Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Carl Dong)
8715658983 Move mempool nullptr Assert out of LoadChainstate (Carl Dong)
9162a4f93e node/chainstate: Decouple from concept of NodeContext (Carl Dong)
c7a5c46e6f node/chainstate: Decouple from ArgsManager (Carl Dong)
ae9121f958 node/chainstate: Decouple from stringy errors (Carl Dong)
cbac28b72f node/chainstate: Decouple from GetTimeMillis (Carl Dong)
cb64af9635 node: Extract chainstate loading sequence (Carl Dong)
Pull request description:
This PR:
1. Coalesce the Chainstate loading sequence between `AppInitMain` and `*TestingSetup` (which makes it more tested)
2. Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.
Code-wise, this PR:
1. Extracts `AppInitMain`'s Chainstate loading sequence into a `::LoadChainstateSequence` function
2. Makes this `::LoadChainstateSequence` function reusable by
1. Decoupling it from various concepts (`ArgsManager`, `uiInterface`, etc)
2. Making it report errors using an `enum` rather than by setting a `bilingual_str`
3. Makes `*TestingSetup` use this new `::LoadChainstateSequence`
Reviewers: Aside from commentary, I've also included `git diff` flags of interest in the commit messages which I hope will aid review!
ACKs for top commit:
ryanofsky:
Code review ACK 7f15eff2dd. Thanks for updates!
MarcoFalke:
review ACK 7f15eff2dd💳
Tree-SHA512: fb9a6cbd1c511a52b477c62a5e68e53a8be5dec2fff0e44a279966afb91efbab44bf1fe7c6b1519f8464ecc25f42dd4bae8e1efbf55ee91fc90fa0b92e3a83e2
fadc0c80ae p2p: Make timeout mockable and type safe, speed up test (MarcoFalke)
fa6d5a238d scripted-diff: Rename m_last_send and m_last_recv (MarcoFalke)
Pull request description:
Use type-safe time for better code readability/maintainability and mockable time for better testability. This speeds up the p2p_timeout test.
This is also a bugfix for intermittent test issues like: https://cirrus-ci.com/task/4769904156999680?command=ci#L2836
Fixes #20654
ACKs for top commit:
laanwj:
Code review ACK fadc0c80ae
naumenkogs:
ACK fadc0c80ae
Tree-SHA512: 28c6544c97f188c8a0fbc80411c74ab74ffd055885322c325aa3d1c404b29c3fd70a737e86083eecae58ef394db1cb56bc122d06cff63742aa89a8e868730c64
ffd09281fe rpc: various fixups for dumptxoutset (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
A few fixes to make this RPC actually useful when generating snapshots.
- Generate an assumeutxo hash and display it (sort of a bugfix)
- Add nchaintx to output (necessary for use in chainparams entry)
- Add path of serialized UTXO file to output
ACKs for top commit:
laanwj:
Code review ACK ffd09281fe
Tree-SHA512: b0b5fd5138dea0e21258b1b18ab75bf3fd1628522cc1dbafa81af9cb9fa96562a1c39124fdb31057f256bfc560f462f907e9fe5e209b577b3f57afae2b7be826
fa00447442 scripted-diff: Use clang-tidy syntax for C++ named arguments (MarcoFalke)
fae13c3989 doc: Use clang-tidy comments in crypto_tests (MarcoFalke)
Pull request description:
Incorrect named args are source of bugs, like #22979.
To allow them being checked by `clang-tidy`, use a format it can understand.
ACKs for top commit:
shaavan:
ACK fa00447442
rajarshimaitra:
ACK fa00447442
jonatack:
ACK fa00447442
fanquake:
ACK fa00447442
Tree-SHA512: 4d23a8363da81dfea21a4cd8516ab5e0dc70119e4d503f3f240f38573218b2c2e84083b97e956c62942d78b2f17490f8b3b2e8077d257644fda1d901e2b80507
- Actually generate an assumeutxo hash and display it
- Add nchaintx to output (necessary for use in chainparams entry)
- Add path of serialized UTXO file to output
fa4e09924b refactor: Replace validation.h include with forward-decl in miner.h (MarcoFalke)
fa0739a7d3 style: Sort file list after rename (MarcoFalke)
fa53e3a58c scripted-diff: Move miner to src/node (MarcoFalke)
Pull request description:
It is impossible to run the miner without a node (validation, chainstate, mempool, rpc, ...). Also, the module is in the node library. Thus, it should be moved to `src/node`.
Also, replace the `validation.h` include in the header with a forward-declaration.
ACKs for top commit:
theStack:
Code-review ACK fa4e09924b
Tree-SHA512: 791e6caa5839d8dc83b0f58f3f49bc0a7e3c1710822e8a44dede254c87b6f7531a0586fb95e8a067c181457a3895ad6041718aa2a2fac64cfc136bf04bb851d5
2d2edc1248 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
99516285b7 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb64a tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c6bd tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319fea4 tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf0243872 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9b90 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1320 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)
Pull request description:
Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.
Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.
The tests which test specific legacy wallet behavior remain unchanged.
ACKs for top commit:
laanwj:
Code review ACK 2d2edc1248
brunoerg:
tACK 2d2edc1248
Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1
fadf1186c8 p2p: Use mocktime for ping timeout (MarcoFalke)
Pull request description:
It is slightly confusing to use mocktime for some times, but not others.
Start fixing that by making the ping timeout use mocktime.
The only downside would be that tests that use mocktime disconnect peers after this patch. However, I don't think this is an issue, as the inactivity check is already disabled for all functional tests after commit 6d76b57ca0. Only one unit test needed the inactivity check disabled as part of this patch.
A nice side effect of this patch is that the `p2p_ping` functional test now runs 4 seconds faster.
ACKs for top commit:
laanwj:
Code review ACK fadf1186c8
Tree-SHA512: e9e7b21040a89d9d574b3038f85a67e6336de6cd6e41aa286769cd03cada6e75a94ec01700e052e56d822ef85d7813cc06bf7e67b81543eff8917a16cdccf942
