Age | Commit message (Collapse) | Author |
|
3153e7d779ac284f86e433af033d63f13f361b6f [fuzz] Add HeadersSyncState target (dergoegge)
53552affca381cdb5103ecdbcc7f3fb562e66ac4 [headerssync] Make m_commit_offset protected (dergoegge)
Pull request description:
This adds a fuzz target for the `HeadersSyncState` class.
I am unsure how well this is able to cover the logic since it is just processing unserialized CBlockHeaders straight from the fuzz input (headers are sometimes made continuous). However, it does manage to get to the redownload phase so i thought it is better then not having fuzzing at all.
It would also be nice to fuzz the p2p logic that is using `HeadersSyncState` (e.g. `TryLowWorkHeadersSync`, `IsContinuationOfLowWorkHeadersSync`) but that likely requires some more work (refactoring👻).
ACKs for top commit:
mzumsande:
ACK 3153e7d779ac284f86e433af033d63f13f361b6f
Tree-SHA512: 8a4630ceeeb30e4eeabaa8eb5491d98f0bf900efe7cda07384eaac9f2afaccfbcaa979cc1cc7f0b6ca297a8f5c17a7759f94809dd87eb87d35348d847c83e8ab
|
|
|
|
We've already used it unguarded in `httpserver.cpp` for years, with no
build issues.
|
|
00e9b97f37e0bdf4c647236838c10b68b7ad5be3 refactor: Move fs.* to util/fs.* (TheCharlatan)
106b46d9d25b5228ef009fbbe6f9a7ae35090d15 Add missing fs.h includes (TheCharlatan)
b202b3dd6393b415fa68e18dc49c9431dc6b58b2 Add missing cstddef include in assumptions.h (TheCharlatan)
18fb36367a28819bd5ab402344802796a1248979 refactor: Extract util/fs_helpers from util/system (Ben Woosley)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by empact and is taken from its parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (https://github.com/bitcoin/bitcoin/pull/25290, https://github.com/bitcoin/bitcoin/pull/25487, https://github.com/bitcoin/bitcoin/pull/25527, https://github.com/bitcoin/bitcoin/pull/25862, https://github.com/bitcoin/bitcoin/pull/26177, and https://github.com/bitcoin/bitcoin/pull/27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in https://github.com/bitcoin/bitcoin/pull/27238.
#### Changes
Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files.
There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory.
Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed.
ACKs for top commit:
hebasto:
ACK 00e9b97f37e0bdf4c647236838c10b68b7ad5be3
Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666
|
|
faf8dc496e761a15956f8226d727f4bbab8dff82 fuzz: Remove legacy int parse fuzz tests (MarcoFalke)
Pull request description:
The fuzz tests checked that the result of the new function was equal to the legacy function. (Side note: The checks were incomplete, as evident by the follow-up fix in commit b5c9bb5cb9f4a8db57b33ef7399310c7d6de5822).
Given that they haven't found any issues in years (beside missing the above issue, that they couldn't catch), it seems time to remove them.
They may come in handy in the rare case that someone would want to modify `LocaleIndependentAtoi()` or `Parse*Int*()`, however that seems unlikely. Also, appropriate checks can be added then.
ACKs for top commit:
fanquake:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
dergoegge:
ACK faf8dc496e761a15956f8226d727f4bbab8dff82
Tree-SHA512: 4ec88b9fa8ba49a923b0604016f0f471b3c9b9e0ba6c5c3dc4e20503c6994789921e7221d9ec467a2a37a73f21a70ba51ba3370ed5ad311dee989e218290b29a
|
|
cd0c8eeb0940790b6ba83786d1c9e362d4dc4829 [net] Pass nRecvFloodSize to CNode (dergoegge)
860402ef2ed728ef096dda4e65e77d566782209f [net] Remove trivial GetConnectionType() getter (dergoegge)
b5a85b365a4abd98176b0935015dbb502cc3e6f6 [net] Delete CNetMessage copy constructor/assignment op (dergoegge)
Pull request description:
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
ACKs for top commit:
jnewbery:
utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
theStack:
ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41
|
|
Before wiping the ChainStateManager, the validationinterface
queue must be drained to avoid accessing deleted memory.
|
|
|
|
|
|
related fixes
03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 clang-tidy: Exclude `performance-*` checks rather including them (Hennadii Stepanov)
24004372302adfc0e7cb36f8db6830694bf050e9 clang-tidy: Add `performance-type-promotion-in-math-fn` check (Hennadii Stepanov)
7e975e6cf86617346c1d8e2568f74a0252c03857 clang-tidy: Add `performance-inefficient-vector-operation` check (Hennadii Stepanov)
516b75f66ec3ba7495fc028c750937bd66cc9bba clang-tidy: Add `performance-faster-string-find` check (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
martinus:
ACK 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808
TheCharlatan:
re-ACK [03ec5b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/03ec5b6f9ca3af28c9ce25cf2393e28ae852d808)
Tree-SHA512: 2dfa52f9131da88826f32583bfd534a56a998477db9804b7333c0e7ac0b6b36141009755c7163b9f95d0ecbf5c2cb63f8a69ce4b114bb83423faed21b50cec67
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html
|
|
CConnman and ConnmanTestMsg
3566aa7d495bb783bbd135726238d9f2a9e9f80e [net] Remove CNode friends (dergoegge)
3eac5e7cd1eda6ababb9af9cd72dae08d395993d [net] Add CNode helper for send byte accounting (dergoegge)
60441a3432df10f5d7c15c09c9569f27a793625b scripted-diff: [net] Rename CNode process queue members (dergoegge)
6693c499f7d48aaa1f527a1a860b943fc8442241 [net] Make cs_vProcessMsg a non-recursive mutex (dergoegge)
23d93526545c271b0eed2bd468681864c4213cce [net] Make CNode msg process queue members private (dergoegge)
897e342d6ec320c286753daef01d6eb9839e2c4d [net] Encapsulate CNode message polling (dergoegge)
cc5cdf877666c232a94f03faaf430cbeb6968372 [net] Deduplicate marking received message for processing (dergoegge)
ad44aa5c64d4ee5f31c867fda26350ab560575b7 [net] Add connection type getter to CNode (dergoegge)
Pull request description:
We should define clear interfaces between CNode, CConnman and PeerManager. This PR makes a small step in that direction by ending the friendship of CNode, CConnman and ConnmanTestMsg. CNode's message processing queue is made private in the process and its mutex is turned into a non-recursive mutex.
ACKs for top commit:
jnewbery:
utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
vasild:
ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
theStack:
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
brunoerg:
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
Tree-SHA512: 26b87da5054e32401b693b2904e9c5f40e35a53937c0b6cf44b8597034ad07bacf27d87cdffc54d3e7ccfebde4231ef30a38d326f88cc18133bbb34688ead567
|
|
The fs.* files are already part of the libbitcoin_util library. With the
introduction of the fs_helpers.* it makes sense to move fs.* into the
util/ directory as well.
|
|
This is an extraction of filesystem related functions from util/system
into their own utility file.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager defined in system.h.
Moving these functions out of system.h allows including them from a
separate source file without including the ArgsManager definitions from
system.h.
|
|
|
|
swap functions
95ad70ab652ddde7de65f633c36c1378b26a313a test: Default initialize `should_freeze` to `true` (Hennadii Stepanov)
cea50521fe810111a8a3c84ad14f944eafb5b658 refactor: Drop no longer used `swap` member functions (Hennadii Stepanov)
a87fb6bee5a7fb0879b3adea9a29997f1331acb0 clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` (Hennadii Stepanov)
b4bed5c1f98c0eed18f52fdcea11a420c10ed98d refactor: Drop no longer used `CScriptCheck()` default constructor (Hennadii Stepanov)
d8427cc28e3a9ac3319fb452b16661957c812b8f refactor: Use move semantics in `CCheckQueue::Loop` (Hennadii Stepanov)
9a0b5241396efe3b3ceb3931717c30bb94f99bfb clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` (Hennadii Stepanov)
04831fee6dca3eb86cd1d6b9ef879b296263fe35 refactor: Make move semantics explicit for callers (Hennadii Stepanov)
6c2d5972f3544c4f3e987828a99e88f27b62cf87 refactor: Use move semantics in `CCheckQueue::Add` (Hennadii Stepanov)
06820032142a75cc3c5b832045058bc6f6f74786 test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` (Hennadii Stepanov)
15209d97c6aad7d5c199fe007ad39b91c8ee6562 consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` (Hennadii Stepanov)
Pull request description:
This PR makes code more succinct and readable by using move semantics.
ACKs for top commit:
martinus:
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a
achow101:
ACK 95ad70ab652ddde7de65f633c36c1378b26a313a
TheCharlatan:
re-ACK https://github.com/bitcoin/bitcoin/commit/95ad70ab652ddde7de65f633c36c1378b26a313a
MarcoFalke:
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥
Tree-SHA512: adda760891b12d252dc9b823fe7c41eed660364b6fb1a69f17607d7a31eb0bbb82a80d154a7acfaa241b5de37d42a293c2b6e059f26a8e92d88d3a87c99768fb
|
|
It is safe now, when move semantics is used instead of a custom swap
function.
|
|
|
|
Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
|
|
|
|
|
|
Co-authored-by: Martin Leitner-Ankerl <martin.ankerl@gmail.com>
|
|
|
|
|
|
|
|
|
|
fixes #22638
If we find a duplicate key and error, clear `values` before returning so that
WriteSettings will write an empty file, therefore clearing it.
This aligns with GUI behaviour added in 1ee6d0b.
|
|
1a0d8e178c7b9a3e94d14f94b77305802ebdc93b build: Re-enable external signer on Windows (Hennadii Stepanov)
989451d0689543b25ee6bf1d5b82c863d583597b configure: Detect compatibility of Boost.Process rather than hardcode non-Windows (Luke Dashjr)
Pull request description:
As https://github.com/boostorg/process/issues/207 has been resolved, it is possible now to re-enable external signer on Windows when cross-compiling.
Guix build hashes:
```
78f69ea7e0dbc8338981a92c0352220ccd7c2272d8cbff6a3b082a1412a935c5 guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/SHA256SUMS.part
ee17456ec818ddf5a175182508966e622573ccb518807cca43a40fa1dceda092 guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/bitcoin-1a0d8e178c7b-aarch64-linux-gnu-debug.tar.gz
5080551bde379c746cc67b10429aef33b9f9e49d2d4e21ee1c3bfd9c1c845d46 guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/bitcoin-1a0d8e178c7b-aarch64-linux-gnu.tar.gz
dfab220ce76a40bf7dcf07aab352a616a91b516503639455fe7e1b137bad3e85 guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/SHA256SUMS.part
516ceb822571a8bd88fe107dca434ef596b1e4328ccbda1d51e1d482d3050396 guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/bitcoin-1a0d8e178c7b-arm-linux-gnueabihf-debug.tar.gz
21325380638f817107c203b9a1aedb808d1a4a2b4041493753ca4cbf19aa4f2c guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/bitcoin-1a0d8e178c7b-arm-linux-gnueabihf.tar.gz
cf48ed78fcfceaeb3610ccf22326d735a129dcbf9d50b557b3de359169aefdfd guix-build-1a0d8e178c7b/output/arm64-apple-darwin/SHA256SUMS.part
d4d51e136148bac6a20bb3adb402c499967647736acb420bfdeb71603aba57da guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin-unsigned.dmg
95bb62d24f860e08a392ddb74d5860ccf27e8baa183e6749af877d26a3bd6b0b guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin-unsigned.tar.gz
68da4c92f37bb802df37141af194f47c16da1d84f77a0fbb1016013ae0338502 guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin.tar.gz
6704e38c2d3f11321403797598d05f062648fec6f2d76900ba250dab481e29da guix-build-1a0d8e178c7b/output/dist-archive/bitcoin-1a0d8e178c7b.tar.gz
64b936bc90d1e01fe8f276511edc9bb945dcebe70332aa37d3a786348443b8e7 guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/SHA256SUMS.part
3d03532e54b6e42498ea240c86b8567e94fd462f56087b869c3d6f09e2dde878 guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64-linux-gnu-debug.tar.gz
c5843d79a58b0a864fe723458dab4eee54ad11f4b1f7960975b086eeedc0d541 guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64-linux-gnu.tar.gz
f861ff519bd5e3d6d5ce1646ee0a06bcef1288ddb804a4a600e4dbfe5d5be521 guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/SHA256SUMS.part
5f477da21980dbcf9696081903dc1ba8a3f79ce3579641d208e69a6f598c8eb9 guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64le-linux-gnu-debug.tar.gz
b3757b11c614136934158acea5139e8abd0c5c9cdfda72ae44db436f21716b33 guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64le-linux-gnu.tar.gz
1c21bdb17fe3436e685e88c62423e630fe2b3c41dd00025a99fd80d97817ac2f guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/SHA256SUMS.part
f36ae98473f086ae8f0dc66223b5ec407d57dc4d8d45ae284401520ff5c0b273 guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/bitcoin-1a0d8e178c7b-riscv64-linux-gnu-debug.tar.gz
1603e4d0e869eb47a1dc2d26b67772d0016d90f7ba5e50d2009365cc02cb8169 guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/bitcoin-1a0d8e178c7b-riscv64-linux-gnu.tar.gz
f86ef652102f022827b70477bffa0a44008c6300cf62ca7b3595146cf2ed91ba guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/SHA256SUMS.part
f84d435d8e4709bf29bc7ac7ed8dc6b8af4077cef05e520b468b2896ce10876a guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin-unsigned.dmg
af2aab969b7ed7aeea0e02adbcc9e3b438086bf76b6bfc36146c53e05a27bd57 guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin-unsigned.tar.gz
32a5109ba28ab74ff66238e6a8f8a04e455ebce382a3be287df92a227818fe72 guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin.tar.gz
377462e9a96f4aba72c915dd5df5159a4301a1fa8ed0ee48faa6c71573de80c3 guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/SHA256SUMS.part
a3bf62e828d2350a483b2d16205014f66e8884597b0b72e178042a958c548336 guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/bitcoin-1a0d8e178c7b-x86_64-linux-gnu-debug.tar.gz
66cda980188ea1941a7d66c8b03c447580af33db55abe3bbe3581823ae0534a3 guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/bitcoin-1a0d8e178c7b-x86_64-linux-gnu.tar.gz
2117f0dd9baeb4d585f841592e94c088f4487bf2008b8f281d0c3ceee92ff6cc guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/SHA256SUMS.part
d40d5dec3287f467c42232c05d82f7fb538cda34bd2e63ff7e1876f471c3a790 guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-debug.zip
92dcc92765fbc07b1cc8258bfa69280541e1b4553cc41fed18672c2c6931d5c0 guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-setup-unsigned.exe
a6dd9b4d29f21d3a18cf64556cb03446ef17bf801eb6ac257b65d27cbd95080f guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-unsigned.tar.gz
a4022e595d955198f73530473ef8e90a708746089ee2dd27de794176873330c1 guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64.zip
```
ACKs for top commit:
Sjors:
tACK 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b
achow101:
ACK 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b
Tree-SHA512: db7319259b1e1571cfab4bb3b99ae10a2f744e62757cae5059fd6f4dd6d5586eb09feb63a0c4bb07f7128b283f1dc281ed435224bc8e40da577fd4f04cde489a
|
|
2b373fe49d64f04ceab2309d3f40da7bac6b37d6 docs: update assumeutxo.md (James O'Beirne)
87a1108c81fe0cb15c3860e3a67dc1f43ffec705 test: add snapshot completion unittests (James O'Beirne)
d70919a88fc90a2662f9a844deb085d03ee7b5d8 refactor: make MempoolMutex() public (James O'Beirne)
7300ced9de22e6d1bff816e6538d3370cebe7501 log: add LoadBlockIndex() message for assumedvalid blocks (James O'Beirne)
d96c59cc5cd2f73f1f55c133c52208671fe75ef3 validation: add ChainMan logic for completing UTXO snapshot validation (James O'Beirne)
f2a4f3376f1476b38a79a549bd81ba3006225df6 move-only-ish: init: factor out chainstate initialization (James O'Beirne)
637a90b973f60555ea4fef4b845ffa7533dcb866 add Chainstate::HasCoinsViews() (James O'Beirne)
c29f26b47b8ef978d8689dc0222aa663361ee6cb validation: add CChainState::m_disabled and ChainMan::isUsable (James O'Beirne)
5ee22cdafd2562bcb8bf0ae6025e4b53c826382d add ChainstateManager.GetSnapshot{BaseHeight,BaseBlock}() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
Part two of replacing https://github.com/bitcoin/bitcoin/pull/24232.
---
When a user activates a snapshot, the serialized UTXO set data is used to create an "assumed-valid" chainstate, which becomes active in an attempt to get the node to network tip as quickly as possible. Simultaneously in the background, the already-existing chainstate continues "conventional" IBD to both accumulate full block data and serve as a belt-and-suspenders to validate the assumed-valid chainstate.
Once the background chainstate's tip reaches the base block of the snapshot used, we set `m_stop_use` on that chainstate and immediately take the hash of its UTXO set; we verify that this matches the assumeutxo value in the source code. Note that while we ultimately want to remove this background chainstate, we don't do so until the following initialization process, when we again check the UTXO set hash of the background chainstate, and if it continues to match, we remove the (now unnecessary) background chainstate, and move the (previously) assumed-valid chainstate into its place. We then reinitialize the chainstate in the normal way.
As noted in previous comments, we could do the filesystem operations "inline" immediately when the background validation completes, but that's basically just an optimization that saves disk space until the next restart. It didn't strike me as worth the risk of moving chainstate data around on disk during runtime of the node, though maybe my concerns are overblown.
The final result of this completion process is a fully-validated chain, where the only evidence that the user synced using assumeutxo is the existence of a `base_blockhash` file in the `chainstate` directory.
ACKs for top commit:
achow101:
ACK 2b373fe49d64f04ceab2309d3f40da7bac6b37d6
Tree-SHA512: a204e1d6e6932dd83c799af3606b01a9faf893f04e9ee1a36d63f2f1ccfa9118bdc1c107d86976aa0312814267e6a42074bf3e2bf1dead4b2513efc6d955e13d
|
|
Also adjusts the previous snapshot chainstate init tests
to account for the fact that the init process is now attempting to
validate and complete background chainstates whose tip is at the
snapshot base block. We use a DisconnectTip() hack to preserve the
nature of the test.
|
|
802cc1ef536e11944608fe9ab782d3e962037703 Deduplicate bitcoind and bitcoin-qt init code (Ryan Ofsky)
d172b5c6718b69200c8ad211fe709860081bd692 Add InitError(error, details) overload (Ryan Ofsky)
3db2874bd71d2391747b7385cabcbfef67218c4c Extend bilingual_str support for tinyformat (Ryan Ofsky)
c361df90b9fd34e50bbf1db43b866930e5498357 scripted-diff: Remove double newlines after some init errors (Ryan Ofsky)
Pull request description:
Add common InitConfig function to deduplicate bitcoind and bitcoin-qt code reading config files and creating the datadir.
Noticed the duplicate code while reviewing #27073 and want to remove it because difference in bitcoin-qt and bitcoind behavior make it hard to evaluate changes like #27073
There are a few minor changes in behavior:
- In bitcoin-qt, when there is a problem reading the configuration file, the GUI error text has changed from "Error: Cannot parse configuration file:" to "Error reading configuration file:" to be consistent with bitcoind.
- In bitcoind, when there is a problem reading the settings.json file, the error text has changed from "Failed loading settings file" to "Settings file could not be read" to be consistent with bitcoin-qt.
- In bitcoind, when there is a problem writing the settings.json file, the error text has changed from "Failed saving settings file" to "Settings file could not be written" to be consistent with bitcoin-qt.
- In bitcoin-qt, if there datadir is not accessible (e.g. no permission to read), there is an normal error dialog showing "Error: filesystem error: status: Permission denied [.../settings.json]", instead of an uncaught exception.
ACKs for top commit:
Sjors:
Light review ACK 802cc1ef536e11944608fe9ab782d3e962037703
TheCharlatan:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
achow101:
ACK 802cc1ef536e11944608fe9ab782d3e962037703
Tree-SHA512: 9c78d277e9ed595fa8ce286b97d2806e1ec06ddbbe7bd3434bd9dd7b456faf8d989f71231e97311f36edb9caaec645a50c730bd7514b8e0fe6e6f7741b13d981
|
|
|
|
string_view
545ff924ab6303ffabd91fdfc4f0a4962daf133c refactor: use string_view for RPC named argument values (stickies-v)
7727603e44f8f674e0fc8389e78047e2b56e6052 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v)
1d02e599012721549d4c20b1b37fcc5ee7b961b6 test: add cases to JSON parsing (stickies-v)
Pull request description:
Inspired by MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/26506#discussion_r1036149426). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change.
## Questions
- ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~
- Cleared up by https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059
- If there are no objections to 7727603e44f8f674e0fc8389e78047e2b56e6052, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`.
ACKs for top commit:
ryanofsky:
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
MarcoFalke:
review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c 📻
Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c
|
|
too large scripts
56e37e71a2538a240cc360678aeb752d17bd8f45 Make miniscript fuzzers avoid script size limit (Pieter Wuille)
bcec5ab4ff1039c0c309dbbb9953adbd0a4f3e88 Make miniscript fuzzers avoid ops limit (Pieter Wuille)
213fffa5138229eac2d4a9eda0f643fe90870378 Enforce type consistency in miniscript_stable fuzz test (Pieter Wuille)
e1f30414c6b9434048e089ccc3ec4f475f980c60 Simplify miniscript fuzzer NodeInfo struct (Pieter Wuille)
5abb0f5ac37e8a17072d5989a025227035fdc7e6 Do base type propagation in miniscript_stable fuzzer (Pieter Wuille)
Pull request description:
This adds a number of improvements to the miniscript fuzzers that all amount to rejecting invalid or overly big miniscripts early on:
* Base type propagation in the miniscript_stable fuzzers prevents constructing a large portion of miniscripts that would be illegal, with just a little bit of type logic in the fuzzer. The fuzzer input format is unchanged.
* Ops and script size tracking in GenNode means that too-large scripts (either due to script size limit or ops limit) will be detected on the fly during fuzz input processing, before actually constructing the scripts.
Closes #27147.
ACKs for top commit:
darosior:
re-ACK 56e37e71a2
dergoegge:
tACK 56e37e71a2538a240cc360678aeb752d17bd8f45
Tree-SHA512: 245584adf9a6644a35fe103bc81b619e5b4f5d467571a761b5809d08b1dec48f7ceaf4d8791ccd8208b45c6b309d2ccca23b3d1ec5399df76cd5bf88f2263280
|
|
Previous bilingual_str tinyformat::format accepted bilingual format strings,
but not bilingual arguments. Extend it to accept both. This is useful when
embedding one translated string inside another translated string, for example:
`strprintf(_("Error: %s"), message)` which would fail previously if `message`
was a bilingual_str.
|
|
`CheckSequenceLocksAtTip()`
75db62ba4cae048e742ca02dc6a52b3b3d6727de refactor: Move calculation logic out from `CheckSequenceLocksAtTip()` (Hennadii Stepanov)
3bc434f4590758db673e1bd4ebf1906ea632f593 refactor: Add `CalculateLockPointsAtTip()` function (Hennadii Stepanov)
Pull request description:
This PR is follow up for bitcoin/bitcoin#22677 and bitcoin/bitcoin#23683.
On master (013daed9acca1b723f599d63ab36b9c2a5c60e5f) it is not obvious that `CheckSequenceLocksAtTip()` function can modify its `LockPoints* lp` parameter which leads to https://github.com/bitcoin/bitcoin/pull/22677#discussion_r762040101.
This PR:
- separates the lockpoint calculate logic from `CheckSequenceLocksAtTip()` function into a new `CalculateLockPointsAtTip()` one
- cleans up the `CheckSequenceLocksAtTip()` function interface
- makes code easier to reason about (hopefully)
ACKs for top commit:
achow101:
ACK 75db62ba4cae048e742ca02dc6a52b3b3d6727de
stickies-v:
re-ACK 75db62b
Tree-SHA512: 072c3fd9cd1e1b0e0bfc8960a67b01c80a9f16d6778f374b6944ade03a020415ce8b8ab2593b0f5e787059c8cf90af798290b4c826785d41955092f6e12e7486
|
|
files on startup
3141eab9c669488a2e7fef5f60d356ac92294922 test: add functional test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
e252909e561e47d75cb3a892657662a139f6532c test: add unit test for ScanAndUnlinkAlreadyPrunedFiles (Andrew Toth)
77557dda4a123515d0fa2a545ee21d7c43a66988 prune: scan and unlink already pruned block files on startup (Andrew Toth)
Pull request description:
There are a few cases where we can mark a block and undo file as pruned in our block index, but not actually remove the files from disk.
1. If we call `FindFilesToPrune` or `FindFilesToPruneManual` and crash before `UnlinkPrunedFiles`.
2. If on Windows there is an open file handle to the file somewhere else when calling `fs::remove` in `UnlinkPrunedFiles` (https://en.cppreference.com/w/cpp/filesystem/remove, https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-deletefilew#remarks). This could be from another process, or if we are calling `ReadBlockFromDisk`/`ReadRawBlockFromDisk` without having a lock on `cs_main` (which has been allowed since https://github.com/bitcoin/bitcoin/commit/ccd8ef65f93ed82a87cee634660bed3ac17d9eb5).
This PR mitigates this by scanning all pruned block files on startup after `LoadBlockIndexDB` and unlinking them again.
ACKs for top commit:
achow101:
ACK 3141eab9c669488a2e7fef5f60d356ac92294922
pablomartin4btc:
re-ACK with added functional test 3141eab9c669488a2e7fef5f60d356ac92294922.
furszy:
Code review ACK 3141eab9
theStack:
Code-review ACK 3141eab9c669488a2e7fef5f60d356ac92294922
Tree-SHA512: 6c73bc57838ad1b7e5d441af3c4d6bf4c61c4382e2b86485e57fbb74a61240710c0ceeceb8b4834e610ecfa3175c6955c81ea4b2285fee11ca6383f472979d8d
|
|
Use the same technique as is using in the FromString miniscript parser to
predict the final script size of the miniscript being generated in the
miniscript_stable and miniscript_smart fuzzers (by counting every unexplored
sub node as 1 script byte, which is possible because every leaf node always
adds at least 1 byte). This allows bailing out early if the script being
generated would exceed the maximum allowed size (before actually constructing
the miniscript, as that may happen only significantly later potentially).
Also add a self-check to make sure this predicted script size matches that
of generated scripts.
|
|
Keep track of the total number of ops the constructed script will have
during miniscript_stable and miniscript_smart fuzzers' GenNode, so it
can abort early if the 201 ops limit would be exceeded.
Also add a self-check that the final constructed node has the predicted
ops size limit, so we know the fuzzer's logic for keeping track of this
is correct.
|
|
Add a self-check to the fuzzer that the constructed types match the expected
types in the miniscript_stable fuzzer too.
|
|
Since we now keep track of all expected child node types (even if rudimentary)
in both miniscript_stable and miniscript_smart fuzzers, there is no need anymore
for the former shortcut NodeInfo constructors without sub types.
|
|
Keep track of which base type (B, K, V, or W) is desired in the miniscript_stable
ConsumeStableNode function. This allows aborting early if the constructed node
won't have the right type.
Note that this does not change the fuzzer format; the meaning of inputs in
ConsumeStableNode is unmodified. The only change is that often the fuzzer will
abort early.
The direct motivation is preventing recursing v: wrappers, which are the only
fragment type that does not otherwise increase the overall minimum possible script
size. In a later commit this will be exploited to prevent overly-large scripts from
being constructed.
|
|
faab273e060d27e166b5fb7fe7692614ec9e5c76 util: Return empty vector on invalid hex encoding (MarcoFalke)
fa3549a77bf6a15d8309d36056237f3126baf721 test: Add hex parse unit tests (MarcoFalke)
Pull request description:
Seems a bit confusing to happily accept random bytes and pretend they are hex encoded strings.
ACKs for top commit:
stickies-v:
re-ACK faab273e060d27e166b5fb7fe7692614ec9e5c76
Tree-SHA512: a808135f744f50aece03d4bf5a71481c7bdca1fcdd0d5b113abdb0c8b382bf81cafee6d17c239041fb49b59f4e19970f24a475378e7f711c3a47d6438de2bdab
|
|
|
|
|
|
VerifyDB dosn't finish successfully
0af16e7134459e0820ab95d751093876c1ec4c6d doc: add release note for #25574 (Martin Zumsande)
57ef2a4812f443b2d734f43cebf3ef5038da83f2 validation: report if pruning prevents completion of verification (Martin Zumsande)
0c7785bb2540b69564104767d38342704230cbc2 init, validation: Improve handling if VerifyDB() fails due to insufficient dbcache (Martin Zumsande)
d6f781f1cfcbc2c2ad5ee289a0642ed00386d013 validation: return VerifyDBResult::INTERRUPTED if verification was interrupted (Martin Zumsande)
6360b5302d2675788de5c4a28ea77d823f6d809e validation: Change return value of VerifyDB to enum type (Martin Zumsande)
Pull request description:
`VerifyDB()` can fail to complete due to insufficient dbcache at the level 3 checks. This PR improves the error handling in this case in the following ways:
- The rpc `-verifychain` now returns false if the check can't be completed due to insufficient cache
- During init, we only log a warning if the default values for `-checkblocks`Â and `-checklevel` are taken and the check doesn't complete. However, if the user actively specifies one of these args, we return with an InitError if we can't complete the check.
This PR also changes `-verifychain` RPC to return `false` if the verification didn't finish due to missing block data (pruning) or due to being interrupted by the node being shutdown.
Previously, this PR also included a fix for a possible assert during verification - this was done in #27009 (now merged).
ACKs for top commit:
achow101:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
ryanofsky:
Code review ACK 0af16e7134459e0820ab95d751093876c1ec4c6d. Only small suggested changes since the last review, like renaming some of the enum values. I did leave more suggestions, but they are not very important and could be followups
john-moffett:
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
MarcoFalke:
lgtm re-ACK 0af16e7134459e0820ab95d751093876c1ec4c6d 🎚
Tree-SHA512: 84b4f767cf9bfbafef362312757c9bf765b41ae3977f4ece840e40c52a2266b1457832df0cdf70440be0aac2168d9b58fc817238630b0b6812f3836ca950bc0e
|
|
creating Miniscript nodes
c1b7bd047f47dcd3eb6897adfaf9a55594deff5d fuzz: avoid redundant dup key checks when creating Miniscript nodes (Antoine Poinsot)
Pull request description:
I thought i had done that already in #24149, but it must have slipped through the rebase. It's a 2x speed improvement against the existing corpora and will probably be much more as we extend them with larger nodes.
ACKs for top commit:
sipa:
ACK c1b7bd047f47dcd3eb6897adfaf9a55594deff5d
Tree-SHA512: 9e6ceb6254183964b6c5538e21ba6321df95a68acb343a15a6ecfef5c51a1980d2627df5aeef9aef1db41656e18cc4f3bc96e6f24314d12fa60368b04a350001
|
|
helper, dedupe add_coin
4275195606e6f42466d9a8ef766b3035833df4d5 De-duplicate add_coin methods to a test util helper (Jon Atack)
9d92c3d7f42c18939a9a6aa1ee185f1c958360a0 Create InsecureRandMoneyAmount() test util helper (Jon Atack)
81f5ade2a324167c03c5ce765a26bd42ed652723 Move random test util code from setup_common to random (Jon Atack)
Pull request description:
- Move random test utilities from `setup_common` to a new `random` file, as many tests don't use this code.
- Create a helper to generate semi-random CAmounts up to `MONEY_RANGE` rather than only uint32, and use the helper in the unit tests.
- De-duplicate a shared `add_coin` method by extracting it to a `coins` test utility.
ACKs for top commit:
pinheadmz:
ACK 4275195606e6f42466d9a8ef766b3035833df4d5
achow101:
ACK 4275195606e6f42466d9a8ef766b3035833df4d5
john-moffett:
ACK 4275195606e6f42466d9a8ef766b3035833df4d5
Tree-SHA512: 3ed974251149c7417f935ef2f8865aa0dcc33b281b47522b0f96f1979dff94bb8527957f098fe4d210f40d715c00f29512f2ffe189097102229023b7284a3a27
|
|
dbwrapper and txdb
aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f refactor, validation: Add ChainstateManagerOpts db options (Ryan Ofsky)
0352258148c51572426666d337c7b28d0033376c refactor, txdb: Use DBParams struct in CBlockTreeDB (Ryan Ofsky)
c00fa1a7341d3f47f992e0beb043da655cbca777 refactor, txdb: Add CoinsViewOptions struct (Ryan Ofsky)
2eaeded37f3a07c35eef38d9a80c1e5fbd4d41ee refactor, dbwrapper: Add DBParams and DBOptions structs (Ryan Ofsky)
Pull request description:
Code in the libbitcoin_kernel library should not be calling `ArgsManager` methods or trying to read options from the command line. Instead it should just get options values from simple structs and function arguments that are passed in externally. This PR removes `gArgs` accesses from `dbwrapper` and `txdb` modules by defining appropriate options structs, and is a followup to PR's #25290 #25487 #25527 which remove other `ArgsManager` calls from kernel modules.
This PR does not change behavior in any way. It is a simpler alternative to #25623 because the only thing it does is remove `gArgs` references from kernel code. It avoids other unnecessary changes like adding options to the kernel API (they can be added separately later).
ACKs for top commit:
TheCharlatan:
Code review ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f
achow101:
ACK aadd7c5b9b43a38beaa954b4cb8c2fff55f2200f
furszy:
diff ACK aadd7c5b
Tree-SHA512: 46dfd5d99ab3110492e7bba97a87122c831b8344caaf7dd2ebdb6e0ad6aa9174d4d1832d6f3a7465eda9294fe50defaa3c000afbbddc4e72838687df09a63ffd
|
|
CService and use better naming
c9d548c91fb12fba516dee896f1f97692cfa2104 net: remove CService::ToStringPort() (Vasil Dimov)
fd4f0f41e915d99c9b0eac1afd21c5628222e368 gui: simplify OptionsDialog::updateDefaultProxyNets() (Vasil Dimov)
96c791dd20fea54c17d224000dee677bc158f66a net: remove CService::ToString() use ToStringAddrPort() instead (Vasil Dimov)
944a9de08a00f8273e73cd28b40e46cc0eb0bad1 net: remove CNetAddr::ToString() and use ToStringAddr() instead (Vasil Dimov)
043b9de59aec88ae5e29daac7dc2a8b51a9414ce scripted-diff: rename ToStringIP[Port]() to ToStringAddr[Port]() (Vasil Dimov)
Pull request description:
Before this PR we had the somewhat confusing combination of methods:
`CNetAddr::ToStringIP()`
`CNetAddr::ToString()` (duplicate of the above)
`CService::ToStringIPPort()`
`CService::ToString()` (duplicate of the above, overrides a non-virtual method from `CNetAddr`)
`CService::ToStringPort()`
Avoid [overriding non-virtual methods](https://github.com/bitcoin/bitcoin/pull/25349/#issuecomment-1185226396).
"IP" stands for "Internet Protocol" and while sometimes "IP addresses" are called just "IPs", it is incorrect to call Tor or I2P addresses "IPs". Thus use "Addr" instead of "IP".
Change the above to:
`CNetAddr::ToStringAddr()`
`CService::ToStringAddrPort()`
The changes touch a lot of files, but are mostly mechanical.
ACKs for top commit:
sipa:
utACK c9d548c91fb12fba516dee896f1f97692cfa2104
achow101:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
jonatack:
re-ACK c9d548c91fb12fba516dee896f1f97692cfa2104 only change since my previous reviews is rebase, but as a sanity check rebased to current master and at each commit quickly re-reviewed and re-verified clean build and green unit tests
LarryRuane:
ACK c9d548c91fb12fba516dee896f1f97692cfa2104
Tree-SHA512: 633fb044bdecf9f551b5e3314c385bf10e2b78e8027dc51ec324b66b018da35e5b01f3fbe6295bbc455ea1bcd1a3629de1918d28de510693afaf6a52693f2157
|