a4defcdd57918f3693902cc40a54d0e64d716d50 test, lint: add `crypted` to `ignore-words` (brunoerg)
Pull request description:
Fixes#26719
"Crypted" is used in some comments at `walletload_tests` because it refers to `DBKeys::CRYPTED_KEY`, it's not necessary
a mistake.
Obs: I can change the approach (changing `walletload_tests` comments to use `encrypted` word instead of adding it to the `ignore_words`) if reviewers think it makes more sense.
ACKs for top commit:
achow101:
ACK a4defcdd57918f3693902cc40a54d0e64d716d50
Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
fad1c55301b9f2d091d3b0d8a75ff522ce8dae5a lint: Skip COMMIT_RANGE if no CIRRUS_PR (MarcoFalke)
Pull request description:
It doesn't make sense to run this for non-PRs, because:
* There are known whitespace "violations" in previous commits, so the lint may fail
* Once the changes are merged, it is too late to fix them up (force pushes are illegal)
* It isn't possible to determine which commits to run on if there is no reference branch (target branch of the pull request)
Moreover, the test fails on non-master:
* https://github.com/bitcoin/bitcoin/runs/8664441400
Fix all issues by skipping it.
ACKs for top commit:
hebasto:
ACK fad1c55301b9f2d091d3b0d8a75ff522ce8dae5a, also tested in my personal Cirrus account.
Tree-SHA512: be15f00e2b2a9069583833545883e0e5968a33d2455dad59e6fb47c1102b4dd16ef932e9ba945e29e9d941e6c17bd531a02c66b0491097801be6bda476875537
bf9597606166323158bbf631137b82d41f39334f doc: add note about snapshot chainstate init (James O'Beirne)
e4d799528696c5ede38c257afaffd367917e0de8 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336d93a2dc88e4a61c49941887766bd72 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c08a00f1f407f1534853a5cddbbc0a23 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8f5971eb3c7b620aa7ad9b437cc515e test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215ed900145bd770525a341ba0ed9c027 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2c16a50d854f6f81428a68aa9180509 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247dad3982d54488bcdb3960470315290 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c2b271cb4683da2fb25fd295557f731 validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d159033106cc595cfa852695610bfe419c989c add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8bc5cdf4368ad55e827a873240535b28 init: add utxo snapshot detection (James O'Beirne)
f9f1735f139b6a1f1c7fea50717ff90dc4ba2bce validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100aaaa25c7558eeed8b5c536da99885f db: add StoragePath to CDBWrapper/CCoinsViewDB (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)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf9597606166323158bbf631137b82d41f39334f
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf9597606166323158bbf631137b82d41f39334f. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf9597606166323158bbf631137b82d41f39334f
aureleoules:
ACK bf9597606166323158bbf631137b82d41f39334f
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
and also fix spelling in test/lint/lint-locale-dependence.py not caught by the
spelling linter and fix up a paragraph we are touching here in test/README.md.
This changes the snapshot's leveldb chainstate dir name from
`chainstate_[blockhash]` to `chainstate_snapshot`. This simplifies
later logic that loads snapshot data, and enforces the limitation
of a single snapshot at any given time.
Since we still need to persis the blockhash of the base block, we
write that out to a file (`chainstate_snapshot/base_blockhash`) for
later use during initialization, so that we can reinitialize the
snapshot chainstate.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
ecff20db286e2f5d3afe32cfaae72de69d34d23c logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759fb15824a5dd3004e689d0eb5b884a32 logging: add LogPrintfCategory to log unconditionally with category (Jon Atack)
Pull request description:
These are the next two commits from #25203.
- Add `LogPrintfCategory` to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the `lint-logs.py` and `lint-format-strings.py` scripts.
- Replace the log messages that manually print a category, with `LogPrintfCategory`. In upcoming commits, it will likely be used in many other cases, such as to replace `LogPrintf` where it makes sense.
ACKs for top commit:
klementtan:
Code Review ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
laanwj:
Code review ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
brunoerg:
ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
d575413fb8f8569803ed0050c15be003ef50951c doc: add `desig` to ignore-words (brunoerg)
c06cc41ddbac3611d5c7826a2a3f2cbae9aeea12 doc: fix typo in kernel/context.h (brunoerg)
Pull request description:
This PR fixes a typo in `kernel/context.h` (libary => library) and add `desig` to ignore-words since it's a valid word, see:
b9416c3847/src/net.cpp (L1105-L1117)
ACKs for top commit:
fanquake:
ACK d575413fb8f8569803ed0050c15be003ef50951c
Tree-SHA512: 2d548c737b8184d0243445c7503f3f68256ecb0970bd834d52de099de3cd8c8b9c140e2b77d55e2542fbd45b1d21cbdee639f5b2ef8138c37b8b72e5211029c3
that was added in 2015 by commit b8c06ef40 in PR 7003, as that potential issue
would now be caught by the test/lint/lint-format-strings.py script run by the CI
f565b2836d5efeb6f7c16d0fac813b06fa4d41e4 Fixup option name in bench message (Ben Woosley)
bf209ac7a732394c3a54d6d1e3fb43f180ac1bb8 doc: Fix spelling errors identified by codespell in coments (Ben Woosley)
Pull request description:
From the output [here](https://cirrus-ci.com/task/5275612980969472?logs=lint#L849):
```
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
~~I left the 'nd' in miniscript_tests as-is, as it's valid miniscript,
and I'm wary of whitelisting it.~~
ACKs for top commit:
dunxen:
ACK f565b28
Tree-SHA512: 501a426c5f6f9761e2c8f980d5d955611428a827321888f53e0ae9526b0fecd43f9d1fa845fc70ae2489d77be6dc0b5b371dff55c5146f4b39ed874f4a1ea917
From the output here:
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
https://cirrus-ci.com/task/5275612980969472?logs=lint#L849
I added 'nd' to the spelling.ignored-words.txt, as it's valid miniscript.
664a14ba7ccb40aa82d35a59831acd35db1897a6 coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f1006875665ffe8ff5da8185effe25b860743b4e kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8e4856445b1cfc9b5e072ce8f690f36 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c06ffe74b9e9fbc07bfe6d282fef9cb scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f0498e52131f8ae0c76b4dfe25f45b076 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c965f79b8c376c8a922497e385445dd8 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2efd7341fe55f77b334f27ad8aad090 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b6b030f5d62502c73db84ba9a276640 Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5b84070279ff28399083cb3ab278593 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6a10b20a4e20116a68101a684929eda coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a296a2acea10ec7e5bf7b1827f73c45 coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b878e1236f8e043a8bb1ffb1afc1b673 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d708b7adc0150aba8e500a4aa19bc1c includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35d9eae4d68cbdbef68c337656f3c906 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993771d0a8a718ca1667241872de8241a kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)
Pull request description:
Part of: #24303
Depends on: #24322
The `GetUTXOStats` function has 2 codepaths:
- One which queries the `CoinStatsIndex` for the UTXO hash
- One which actually performs the hashing
For `libbitcoinkernel`, the only place where we call `GetUTXOStats` is in `PopulateAndValidateSnapshots`, which uses the `SHA256D` hash, and is therefore unable to use the `CoinStatsIndex` since that only provides `MuHash` hashes. Not that I think indices necessarily belong in `libbitcoinkernel` anyway.
This PR separates these 2 aforementioned codepaths of `GetUTXOStats`, uses the hashing codepath in `PopulateAndValidateSnapshots`, and removes the need to link in `index/coinstatsindex.cpp` and `node/coinstats.cpp`.
-----
Logistically, this PR:
- Extracts out the `index_requested` and `hash_type` members of `CoinStats`, which served as "in-params" to `GetUTXOStats` embedded within the `CoinStats` struct. This allows `CoinStats` to only consist of "out-param" members, and be returned by `GetUTXOStats` without needing to be an "in-out" param
- Introduce the purely virtual `UTXOHashers` class, with 3 implementations: `SHA256DHasher`, `MuHashHasher`, and `NullHasher`. These replace the existing template-based polymorphism.
- Split `GetUTXOStats` into:
- `CalculateUTXOStatsWithHasher(UTXOHasher&, ...)`, and
- `LookupUTXOStatsWithIndex(CoinStatsIndex&, ...)`
- Use `CalculateUTXOStatsWithHasher` directly where appropriate (`src/validation.cpp` and `src/fuzz`)
- Move `GetUTXOStats` to `rpc/blockchain`, which is the only place that depends on `GetUTXOStats`'s weird fallback behaviour
- Move `LookupUTXOStatsWithIndex` to `index/coinstatsindex`
Code organization:
- `src/`
- `kernel/` → only contains the hashing codepath
- `coinstats.cpp` → hashing codepath implementations
- `coinstats.h` → header for `kernel/coinstats.cpp`
- `index/` → only contains the index codepath
- `coinstatsindex.cpp` → index codepath implementations
- `coinstatsindex.h`
- `validation.cpp` → only uses the hashing codepath
- `rpc/blockchain.cpp` → uses both the hashing and index codepath, old `GetUTXOStats` fallback logic moved here as static
- `test/fuzz/coins_view.cpp` → only uses the hashing codepath
TODOs:
- [x] Commit messages could be fleshed out more
Would love any feedback!
ACKs for top commit:
laanwj:
Code review ACK 664a14ba7ccb40aa82d35a59831acd35db1897a6
Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73
This is the "fruit of our labor" for this patchset.
ChainstateManager::PopulateAndValidateSnapshot can now directly call
ComputeUTXOStats(...).
Our consensus engine is now fully decoupled from all indices.
See the src/Makefile.am for some satisfying removals.
908fb7e2ec37fe68675d38dbfee4df9f861bb2b5 test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80a7479a44b0ab09e87542c8cb7a8f72223 test: Don't use shell=True in `lint-files.py` (laanwj)
Pull request description:
Improvements to the `lint-files.py` script:
- Avoid use of `shell=True`.
- Check the permissions in git's metadata instead of in the filesystem. This stops the umask or filesystem from interfering. It's also more efficient as it only needs a single call to `git ls-files`.
(what triggered this change was `File "..." contains a shebang line, but has the file permission 775 instead of the expected executable permission 755.` errors running the script locally).
ACKs for top commit:
vincenzopalazzo:
re-tACK 908fb7e2ec
Tree-SHA512: 2eaf868c55a9c3508b12658a5b3ac429963fd0551e645332d0ac54be56fefccee95115e4667386df24b46b545593cb0d0bf8c6cecab73f9cb19d37ddf704c614
These are unreferenced in the CI and documentation, and have been since
2019 (see #17549).
I'm not sure the cppcheck is worthwhile. It takes a long time
to run (I think this is why it isn't in the normal lints), and right
now it only appears to find implicit constructors. The list of
exceptions is out of date. But if anyone wants to bring it back at any
time in the future they can do so from git history (and port it to Python).
bd6ceb4049602aa556a93a0ac8954802dc0bf456 test: port 'lint-shell.sh' to python (whiteh0rse)
Pull request description:
Converts `test/lint/lint-shell.sh` to Python and updates the docs accordingly. In order for the linter to run, it requires `git` and the `shellcheck` linter to be installed on the system. The script will fail gracefully with a help message if `shellcheck` is not installed.
Top commit has no ACKs.
Tree-SHA512: edc3f1af582b736a0b46f32bd7448e859201dc43f5dd086f16aab49037a1ab936f5376c29fc1006a932b9e98b4f2423d83d98e9666304781a06eb4d2a16f54e3
e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a8bc26e1612c771173325dbe49b3612 util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7b11b375042c3000d421fd93348c199 util: Refactor SysErrorString logic (laanwj)
e7f2f77756d33c6be9c8998a575b263ff2d39270 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbfbc39ebbc74ab1ed8c00edc12859373 util: Replace non-threadsafe strerror (laanwj)
Pull request description:
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in #4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives (with code from `NetworkErrorString`) and replace all uses of `strerror` with this.
Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.
Edit2: from the linux manpage:
```
ATTRIBUTES
For an explanation of the terms used in this section, see attributes(7).
┌───────────────────┬───────────────┬─────────────────────────┐
│Interface │ Attribute │ Value │
├───────────────────┼───────────────┼─────────────────────────┤
│strerror() │ Thread safety │ MT-Unsafe race:strerror │
├───────────────────┼───────────────┼─────────────────────────┤
…
├───────────────────┼───────────────┼─────────────────────────┤
│strerror_r(), │ Thread safety │ MT-Safe │
│strerror_l() │ │ │
└───────────────────┴───────────────┴─────────────────────────┘
```
As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.
ACKs for top commit:
jonatack:
ACK e3a06a3c6cbb288ac89a2725cf71ae8adaebf35c
Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25
Note that `SplitString` doesn't support token compression, but in this case
it does not matter as empty strings are already skipped anyways.
Also removes split.hpp and classification.hpp from expected includes
fa82a1ed833fd749849fa19267207b63e338d84d lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake)
Pull request description:
Follow up to commit b1c5991eebb916755be188f355ad36fe01a3f529. Also remove empty newline added in that commit.
ACKs for top commit:
fanquake:
ACK fa82a1ed833fd749849fa19267207b63e338d84d
Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478
29f44fed36f45cb827c5e593ce52682a942bc296 Converting `lint-all.sh` to `lint-all.py`. (hiago)
Pull request description:
This PR is converting `test/lint/lint-all.sh` to `test/lint/lint-assertions.py`. It's an item of https://github.com/bitcoin/bitcoin/issues/24783.
ACKs for top commit:
laanwj:
Tested ACK 29f44fed36f45cb827c5e593ce52682a942bc296
Tree-SHA512: 5427936aaa8e009613048448cbd79d9225675bdcadcf4fbb70fd091e0aab85e350ef1678da1b388f70d62ca16f40409409f90da3f6bbf057480522cd50fde811
Add `strerror` to the locale-dependence linter to catch its use. Add
exemptions for bdb interface code (false positive) and strerror.cpp
(the only allowed use).
Also fix a bug in the regexp so that `_r` and `_s` variants are detected
again.
172c2333f03aecb4c347c791537e13c296adbde2 Porting lint-assertions.sh to lint-assertions.py (hiago)
Pull request description:
This PR is converting `test/lint/lint-assertions.sh` to `test/lint/lint-assertions.py`. It's an item of #24783.
ACKs for top commit:
laanwj:
Tested ACK 172c2333f03aecb4c347c791537e13c296adbde2
Tree-SHA512: 94d5b03acfeaf2303fad95d489d6c3aa7bd655889ddaa807cc97e0613b8eb8f5ef094feee2a98d974606890deb554e76490a5c523d64eb5bc55afa6a43221aae
79635c79e0533aa7f2e2440515ad766f965343ba lint: Convert lint-circular-dependencies.sh to Python (Smlep)
Pull request description:
Here is a port of `/test/lint/lint-circular-dependencies.sh` to a Python-script as part of the request of https://github.com/bitcoin/bitcoin/issues/24783.
It aims to provide the same output as the bash version.
ACKs for top commit:
laanwj:
Tested ACK 79635c79e0533aa7f2e2440515ad766f965343ba
Tree-SHA512: f18077018f1229dd933cfe2bf0cfe7dc7d6538961c96a83c7a1f05e0cec4b068ca05502d68410d2aa4b6864523424db386e38233735190525904c2a8e9d2ba13
267684ee34d795e4f762c4949fa45d99fe74dde3 lint: convert format strings linter test to python (Eunoia)
Pull request description:
Refs #24783
Attempted to keep the style and flow of implementation as it is.
### Additional Notes(Optional):
1. There is scope of improvement on how the related files are fetched. In this `git grep` with `subprocess` is still used as I found it to be the simplest. Any pointers on this are appreciated.
2. Removed sort operation on the matching files as I couldn't think of any strong arguments to have it. Any pointers on this are appreciated.
3. Not important, but one small detail is that the previous implementation was storing matched files for all the `function_names` iterated so far. Fixed that in this PR.
ACKs for top commit:
laanwj:
Code review ACK 267684ee34d795e4f762c4949fa45d99fe74dde3
Tree-SHA512: 54ceae0c3501e561fdd9c5167b2dd8dd06da1b3697a077a042210970ce7004bda8c4e19abb1905ee64cbdce635f0a078508da645846ae7e81c016091f3f02458
035eef4be6071a060260907001363a0d12f1f2d2 lint: Convert lint-python-utf8-encoding.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-python-utf8-encoding.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
laanwj:
Code review ACK 035eef4be6071a060260907001363a0d12f1f2d2
Tree-SHA512: a8a2f505bf7953d318837182101346c44e73cfd1bf3b5342ff1400fb1c67c5292519fa99db1035da87cf27fb5f5ac5d28871bf55a1c085b5f8a3bb33ff0fa3fb
3043a1bc9d32b451020b511c8340c12e0d99f8cc lint: Make known violations more specific in lint-locale-dependence (Dimitri)
229917d3d4798bf60ee2935f31529668bbb53a23 lint: Convert lint-locale-dependence.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-locale-dependence.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
laanwj:
Tested and code review ACK 3043a1bc9d32b451020b511c8340c12e0d99f8cc
Tree-SHA512: 80555cf7aac156bab5488f85098731d1c12a42667fe7d0df0c35487ab8fc951654a70a15351a759282eabab8319f5aabd8bdb153412b9edc3a9033bef64fd609
