fa6286891f Remove unused includes from wallet.cpp (MarcoFalke)
fa8fdbe229 Remove unused includes from blockfilter.h (MarcoFalke)
fad8c36aa9 move-only: Create src/kernel/mempool_removal_reason.h (MarcoFalke)
fa57608800 Remove unused includes from txmempool.h (MarcoFalke)
Pull request description:
This makes compilation of wallet.cpp use a few % less memory and time, locally.
Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem.
ACKs for top commit:
hebasto:
ACK fa6286891f, I have reviewed the code and it looks OK.
Tree-SHA512: 06f1120af2a8ef3368dbd9ae747acda88ace2507bd261bcc10341d476a0b3d71c8485377ea6c108b47df3e4c13b7f75a15f486bafa6a8466303168dde16ebbc8
This fixes a bug in the linter:
"""
Python's open(...) seems to be used to open text files without explicitly specifying encoding='utf8':
test/functional/test_framework/test_node.py: with open(self.debug_log_path, **kwargs) as dl:
"""
6eb33bd0c2 kernel: Add fatalError method to notifications (TheCharlatan)
7320db96f8 kernel: Add flushError method to notifications (TheCharlatan)
3fa9094b92 scripted-diff: Rename FatalError to FatalErrorf (TheCharlatan)
edb55e2777 kernel: Pass interrupt reference to chainman (TheCharlatan)
e2d680a32d util: Add SignalInterrupt class and use in shutdown.cpp (TheCharlatan)
Pull request description:
Get rid of all `ShutdownRequested` calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.
Replace all `AbortNode` calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.
---
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a `StartShutdown` call which will be removed in followup PR https://github.com/bitcoin/bitcoin/pull/27711. This PR also drops the last reference to the `uiInterface` global in kernel code. The process of moving the `uiInterface` out of the kernel was started in https://github.com/bitcoin/bitcoin/pull/27636.
This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky [here](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869).
ACKs for top commit:
achow101:
light ACK 6eb33bd0c2
hebasto:
ACK 6eb33bd0c2, I have reviewed the code and it looks OK.
ryanofsky:
Code review ACK 6eb33bd0c2. No changes since last review other than rebase.
Tree-SHA512: 7d2d05fa4805428a09466d43c11ae32946cbb25aa5e741b1eec9cd142e4de4bb311e13ebf1bb125ae490c9d08274f2d56c93314e10f3d69e7fec7445e504987c
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
This is done in preparation for the next commit where a new FatalError
function is introduced. FatalErrorf follows common convention to append
'f' for functions accepting format arguments.
-BEGIN VERIFY SCRIPT-
sed -i 's/FatalError/FatalErrorf/g' $( git grep -l 'FatalError')
-END VERIFY SCRIPT-
eefe56967b bugfix: Fix incorrect debug.log config file path (Ryan Ofsky)
3746f00be1 init: Error if ignored bitcoin.conf file is found (Ryan Ofsky)
398c3719b0 lint: Fix lint-format-strings false positives when format specifiers have argument positions (Ryan Ofsky)
Pull request description:
Show an error on startup if a bitcoin datadir that is being used contains a `bitcoin.conf` file that is ignored. There are two cases where this could happen:
- One case reported in [#27246 (comment)](https://github.com/bitcoin/bitcoin/issues/27246#issuecomment-1470006043) happens when a `bitcoin.conf` file in the default datadir (e.g. `$HOME/.bitcoin/bitcoin.conf`) has a `datadir=/path` line that sets different datadir containing a second `bitcoin.conf` file. Currently the second `bitcoin.conf` file is ignored with no warning.
- Another way this could happen is if a `-conf=` command line argument points to a configuration file with a `datadir=/path` line and that path contains a `bitcoin.conf` file, which is currently ignored.
This change only adds an error message and doesn't change anything about way settings are applied. It also doesn't trigger errors if there are redundant `-datadir` or `-conf` settings pointing at the same configuration file, only if they are pointing at different files and one file is being ignored.
ACKs for top commit:
pinheadmz:
re-ACK eefe56967b
willcl-ark:
re-ACK eefe56967b
TheCharlatan:
ACK eefe56967b
Tree-SHA512: 939a98a4b271b5263d64a2df3054c56fcde94784edf6f010d78693a371c38aa03138ae9cebb026b6164bbd898d8fd0845a61a454fd996e328fd7bcf51c580c2b
Do not error on valid format specifications like strprintf("arg2=%2$s arg1=%1$s arg2=%2$s", arg1, arg2);
Needed to avoid lint error in upcoming commit: https://cirrus-ci.com/task/4755032734695424?logs=lint#L221
Additionally tested with python -m doctest test/lint/run-lint-format-strings.py
be55f545d5 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is https://github.com/bitcoin/bitcoin/pull/27254.
The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.
The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale.
ACKs for top commit:
MarcoFalke:
re-ACK be55f545d5🚲
ryanofsky:
Code review ACK be55f545d5. Just small cleanups since the last review.
hebasto:
ACK be55f545d5, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
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.
61bb4e783b lint: enable E722 do not use bare except (Leonardo Lazzaro)
Pull request description:
Improve test code and enable E722 lint check.
If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
Reference: https://peps.python.org/pep-0008/#programming-recommendations
ACKs for top commit:
MarcoFalke:
lgtm ACK 61bb4e783b
Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
that was resolved in PR27036 "test: Remove last uses of snprintf and simplify"
and while here, fix up 2 words in docs to make the spelling linter green again.
One test case uses snprintf to convert an
int to a string. Change it to use ToString
(which uses a locale-independent version of
std::to_string). Also remove unnecessary
parts of StringContentsSerializer.
b68e5a7fef lint: specify the right commit range when running locally (James O'Beirne)
dff7ed5732 test: add an easy way to run linters locally (James O'Beirne)
Pull request description:
Adds a Dockerfile configuration ~~(originally written mostly by fanquake)~~ that allows straightforward running of linters with compatible versions locally. This removes a ton of annoyance when trying to appease CI, because many of the linter versions are quite old and difficult to maintain locally.
I realize that people may not be thrilled to add more ancillary tooling to the repo, but I think this makes a lot of sense given the linter versions listed in this container configuration are dictated by this repo (within the CI configuration), so having these things live in two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid any chance of inconsistencies between local dev experience and CI.
ACKs for top commit:
aureleoules:
ACK b68e5a7fef
stickies-v:
ACK b68e5a7fe
john-moffett:
ACK b68e5a7fef
Tree-SHA512: 7ef7a5dae023d81fdb6296d5d92dfa074ee321c7993e607c9f014d0f21c91558611aa00fc3ce1edc7b5e68371aea0d27fa1931291a79bb867a6c783bb536775f
Adds a Dockerfile configuration
that allows straightforward running of linters with compatible versions
locally. This removes a ton of annoyance when trying to appease CI,
because many of the linter versions are quite old and difficult to
maintain locally.
I realize that people may not be thrilled to more ancillary tooling to
the repo, but I think this makes a lot of sense given the linter
versions listed in this container configuration are dictated by this
repo (within the CI configuration), so having these things live in
two separate places is a recipe for version mismatches.
Eventually we can likely just use this container on CI directly to avoid
any chance of inconsistencies between local dev experience and CI.
e6864fa157 contrib: remove builder keys (fanquake)
Pull request description:
This has been superseded by adding a builder-keys/ directory in
guix.sigs, where the presence of keys, and validity of signatures
is checked. Preventing issues like missing keys or invalid signatures.
New (or exisiting) Guix builders can add their key in the next PR
they open adding attestations.
Related to issues like #26566, #26563.
Also follows up with the comment here: https://github.com/bitcoin/bitcoin/pull/26565#issuecomment-1326053939.
ACKs for top commit:
hebasto:
ACK e6864fa157, modulo s/update/remove/ in the PR tittle.
Tree-SHA512: 095b4cf12ed0baeaf0ee7b8edcb3e2647e9c0f812e8fd63915ddb454f81dacc9c2d2b409de2773b7adb5ff643893d614d8aad1bc44c26da648e1bbbe19e11e05
a4defcdd57 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 a4defcdd57
Tree-SHA512: 49f38eed30ffb0fda4e792566591c3455629379619eb9a5c4240c5b00e14cd27ba1faa36337192233752e642f0998373b86fcb8ca586508bbf15900d68b17950
This has been superseded by adding a builder-keys/ directory in
guix.sigs, where the presence of keys, and validity of signatures
is checked. Preventing issues like missing keys or invalid signatures.
New (or exisiting) Guix builders can add their key in the next PR
they open adding attestations.
fad1c55301 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 fad1c55301, also tested in my personal Cirrus account.
Tree-SHA512: be15f00e2b2a9069583833545883e0e5968a33d2455dad59e6fb47c1102b4dd16ef932e9ba945e29e9d941e6c17bd531a02c66b0491097801be6bda476875537
bf95976061 doc: add note about snapshot chainstate init (James O'Beirne)
e4d7995286 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c0 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8 test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d1590331 add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8b init: add utxo snapshot detection (James O'Beirne)
f9f1735f13 validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100 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 bf95976061
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf95976061. 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 bf95976061
aureleoules:
ACK bf95976061
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.
ecff20db28 logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759f 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 ecff20db28
laanwj:
Code review ACK ecff20db28
brunoerg:
ACK ecff20db28
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
d575413fb8 doc: add `desig` to ignore-words (brunoerg)
c06cc41ddb 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 d575413fb8
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
f565b2836d Fixup option name in bench message (Ben Woosley)
bf209ac7a7 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.
664a14ba7c coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687566 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f04 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c9 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6 coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b8 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993 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 664a14ba7c
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.
908fb7e2ec test: Use permissions from git in `lint-files.py` (laanwj)
48d2e80a74 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).
bd6ceb4049 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
e3a06a3c6c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7 util: Refactor SysErrorString logic (laanwj)
e7f2f77756 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbf 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 e3a06a3c6c
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
fa82a1ed83 lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake)
Pull request description:
Follow up to commit b1c5991eeb. Also remove empty newline added in that commit.
ACKs for top commit:
fanquake:
ACK fa82a1ed83
Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478
29f44fed36 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 29f44fed36
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.
172c2333f0 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 172c2333f0
Tree-SHA512: 94d5b03acfeaf2303fad95d489d6c3aa7bd655889ddaa807cc97e0613b8eb8f5ef094feee2a98d974606890deb554e76490a5c523d64eb5bc55afa6a43221aae
79635c79e0 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 79635c79e0
Tree-SHA512: f18077018f1229dd933cfe2bf0cfe7dc7d6538961c96a83c7a1f05e0cec4b068ca05502d68410d2aa4b6864523424db386e38233735190525904c2a8e9d2ba13
267684ee34 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 267684ee34
Tree-SHA512: 54ceae0c3501e561fdd9c5167b2dd8dd06da1b3697a077a042210970ce7004bda8c4e19abb1905ee64cbdce635f0a078508da645846ae7e81c016091f3f02458
035eef4be6 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 035eef4be6
Tree-SHA512: a8a2f505bf7953d318837182101346c44e73cfd1bf3b5342ff1400fb1c67c5292519fa99db1035da87cf27fb5f5ac5d28871bf55a1c085b5f8a3bb33ff0fa3fb
3043a1bc9d lint: Make known violations more specific in lint-locale-dependence (Dimitri)
229917d3d4 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 3043a1bc9d
Tree-SHA512: 80555cf7aac156bab5488f85098731d1c12a42667fe7d0df0c35487ab8fc951654a70a15351a759282eabab8319f5aabd8bdb153412b9edc3a9033bef64fd609
67b41678c8 lint: Convert lint-includes.sh to Python (Dimitri)
Pull request description:
A port of `test/lint/lint-includes.sh` to a Python-script as part of the request of #24783. Checked for output-consistency.
ACKs for top commit:
KevinMusgrave:
Tested ACK 67b41678c8
Tree-SHA512: 05b4b114dc101e571004aee8aea1480e4dda1dc645426100649e9cb81e56e8667f88d6d5646a9860ea1c7abc36754eda2a77ec10156c54b62db00e2c00b8ceae
