e60fc7d5d3 logging: Replace uses of LogPrintfCategory (Anthony Towns)
f7ce5ac08c logging: add LogError, LogWarning, LogInfo, LogDebug, LogTrace (Anthony Towns)
fbd7642c8e logging: add -loglevelalways=1 option (Anthony Towns)
782bb6a056 logging: treat BCLog::ALL like BCLog::NONE (Anthony Towns)
667ce3e329 logging: Drop BCLog::Level::None (Anthony Towns)
ab34dc6012 logging: Log Info messages unconditionally (Anthony Towns)
dfe98b6874 logging: make [cat:debug] and [info] implicit (Anthony Towns)
c5c76dc615 logging: refactor: pull prefix code out (Anthony Towns)
Pull request description:
Replace `LogPrint*` functions with severity based logging functions:
* `LogInfo(...)`, `LogWarning(...)`, `LogError(...)` for unconditional (uncategorised) logging (replaces `LogPrintf`)
* `LogDebug(CATEGORY, ...)` and `LogTrace(CATEGORY, ...)` for conditional logging (replaces `LogPrint`)
* `LogPrintLevel(CATEGORY, LEVEL, ...)` for when the level isn't known in advance, or a category needs to be added for an info/warning/error log message (mostly unchanged, but rarely needed)
Logs look roughly as they do now with `LogInfo` not having an `[info]` prefix, and `LogDebug` having a `[cat]` prefix, rather than a `[cat:debug]` prefix. This removes `BCLog::Level::None` entirely -- for `LogFlags::NONE` just use `Level::Info`, for any actual category, use `Level::Debug`.
Adds docs to developer-notes about when to use which level.
Adds `-loglevelalways=1` option so that you get `[net:debug]`, `[all:info]`, `[all:warning]` etc, which might be helpful for automated parsing, or just if you like everything to be consistent. Defaults to off to reduce noise in the default config, and to avoid unnecessary changes on upgrades.
Changes the behaviour of `LogPrintLevel(CATEGORY, BCLog::Level::Info, ...)` to be logged unconditionally, rather than only being an additional optional logging level in addition to trace and debug. Does not change the behaviour of `LogPrintLevel(NONE, Debug, ...)` and `LogPrintLevel(NONE, Trace, ...)` being no-ops.
ACKs for top commit:
maflcko:
re-ACK e60fc7d5d3π
achow101:
ACK e60fc7d5d3
stickies-v:
ACK e60fc7d5d3
jamesob:
ACK e60fc7d5d3 ([`jamesob/ackr/28318.1.ajtowns.logging_simplify_api_for`](https://github.com/jamesob/bitcoin/tree/ackr/28318.1.ajtowns.logging_simplify_api_for))
Tree-SHA512: e7a4588779b148242495b7b6f64198a00c314cd57100affab11c43e9d39c9bbf85118ee2002792087fdcffdea08c84576e20844b3079f27083e26ddd7ca15d7f
These provide simple and clear ways to write the most common logging
operations:
LogInfo("msg");
LogDebug(BCLog::LogFlags::NET, "msg");
LogError("msg");
LogWarning("msg");
LogTrace(BCLog::LogFlags::NET, "msg");
For cases where the level cannot be hardcoded, LogPrintLevel(category,
level, ...) remains available.
fa01f884d3 ci: Add missing COPY for ./test/lint/test_runner (MarcoFalke)
faff3e3b46 lint: Report all lint errors instead of early exit (MarcoFalke)
Pull request description:
`all-lint.py` currently collects all failures. However, the `06_script.sh` does not, since July this year (https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268115806).
Fix this by printing all failures before exiting.
Can be tested by modifying (for example) two subtrees in the same commit and then running the linters.
ACKs for top commit:
kevkevinpal:
ACK [fa01f88](fa01f884d3)
TheCharlatan:
lgtm ACK fa01f884d3
Tree-SHA512: c0f3110f2907d87e29c755e3b77a67dfae1f8a25833fe6ef8f2f2c58cfecf1aa46f1a20881576b62252b04930140a9e416c78b4edba0780d3c4fa7aaebabba81
a478c817b2 test: replace `Callable`/`Iterable` with their `collections.abc` alternative (PEP 585) (stickies-v)
4b9afb18e6 scripted-diff: use PEP 585 built-in collection types for verify-binary script (Sebastian Falbesoner)
d516cf83ed test: use built-in collection types for type hints (Python 3.9 / PEP 585) (Sebastian Falbesoner)
Pull request description:
With Python 3.9 / [PEP 585](https://peps.python.org/pep-0585/), [type hinting has become a little less awkward](https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections), as for collection types one doesn't need to import the corresponding capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can use the built-in types directly (see https://peps.python.org/pep-0585/#implementation for the full list).
This PR applies the replacement for all Python scripts (i.e. in the contrib and test folders) for the basic types, i.e.:
- typing.Dict -> dict
- typing.List -> list
- typing.Set -> set
- typing.Tuple -> tuple
For an additional check, I ran mypy 1.6.1 on both master and the PR branch via
```
$ mypy --ignore-missing-imports --explicit-package-bases $(git ls-files "*.py")
```
and verified that the output is identical -- (from the 22 identified problems, most look like false-positives, it's probably worth it to go deeper here and address them in a follow-up though).
ACKs for top commit:
stickies-v:
ACK a478c817b2
fanquake:
ACK a478c817b2
Tree-SHA512: 6948c905f6abd644d84f09fcb3661d7edb2742e8f2b28560008697d251d77a61a1146ab4b070e65b0d27acede7a5256703da7bf6eb1c7c3a897755478c76c6e8
43de4d3630 doc: fix typos (Sjors Provoost)
Pull request description:
This PR fixes typos found by lint-spelling.py using codespell 2.2.6.
Our CI linter job uses codespell 2.2.5 and found fewer typos that I did locally. In any case it's happy now.
ACKs for top commit:
pablomartin4btc:
re ACK 43de4d3630
Tree-SHA512: c032fe86cb49c924a468385653b31f309a9db68c478d70335bba3e65a1ff3826abe80284fe00a090ab5a509e1edbf17e476f6922fb15d055e50f1103dad2ccb0
bbbbdb0cd5 ci: Add filesystem lint check (MarcoFalke)
fada2f9110 refactor: Replace <filesystem> with <util/fs.h> (MarcoFalke)
Pull request description:
Using `std::filesystem` is problematic:
* There is a `fs` namespace wrapper for it. So having two ways to achieve the same is confusing.
* Not using the `fs` wrapper is dangerous and buggy, because it disables known bugs by deleting problematic functions.
Fix all issues by removing use of it and adding a linter to avoid using it again in the future.
ACKs for top commit:
TheCharlatan:
ACK bbbbdb0cd5
fanquake:
ACK bbbbdb0cd5π¦
Tree-SHA512: 0e2d49742b08eb2635e6fce41485277cb9c40fe20b81017c391d3472a43787db1278a236825714ca1e41c9d2f59913865cfb0c649e3c8ab1fb598c849f80c660
The `-zapwallettxes` functionality has been removed in v0.21.0
(see commit 3340dbadd3 / PR #19671),
with the parameter being kept as hidden option, to inform users via
an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to
a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x),
it is highly unlikely that the error caused by the option is still
relevant for any user, hence it seems fine to remove it now.
d9cc99d04e [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a3788c [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24a3b [MiniMiner] track inclusion order and add Linearize() function (glozow)
004075963f [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c0ba [MiniMiner] make target_feerate optional (glozow)
5a83f55c96 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e630b2 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b79b2 [lint] update expected boost includes (glozow)
Pull request description:
This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Allow using `MiniMiner` on transactions that aren't in the mempool.
- Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
- Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
- Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
- Tests
Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.
ACKs for top commit:
TheCharlatan:
ACK d9cc99d04e
kevkevinpal:
reACK [d9cc99d](d9cc99d04e)
achow101:
re-ACK d9cc99d04e
Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
856325fac1 lint: Add `lint-qt-translation.py` (Hennadii Stepanov)
294a018bf5 qt: Avoid error prone leading spaces in translatable strings (Hennadii Stepanov)
d8298e7f06 qt, refactor: Drop superfluous type conversions (Hennadii Stepanov)
Pull request description:
While working on the GUI translation via Transifex web interface, I found it error-prone to have leading whitespace in translatable strings. This is because it is very easy to unintentionally drop them in translations unnoticed.
Fixed all current cases. Added a linter to prevent similar cases in the future.
ACKs for top commit:
furszy:
utACK 856325f
Tree-SHA512: b1ca5effb2db6649e1e99382de79acf3a9f81cc9dad434db5623338489e597897e8addd60c1ab3dcc7506ae62753a7a4ad5a41d7a865f8fcdf94348b54baa7e7
Since Python 3.9, type hinting has become a little less awkward, as for
collection types one doesn't need to import the corresponding
capitalized types (`Dict`, `List`, `Set`, `Tuple`, ...) anymore, but can
use the built-in types directly. [1] [2]
This commit applies the replacement for all Python scripts (i.e. in the
contrib and test folders) for the basic types:
- typing.Dict -> dict
- typing.List -> list
- typing.Set -> set
- typing.Tuple -> tuple
[1] https://docs.python.org/3.9/whatsnew/3.9.html#type-hinting-generics-in-standard-collections
[2] https://peps.python.org/pep-0585/#implementation for a list of type
fa25e8b0a1 doc: Recommend lint image build on every call (MarcoFalke)
faf70c1f33 Bump python minimum version to 3.9 (MarcoFalke)
fa8996b930 ci: Bump i686_multiprocess.sh to latest Ubuntu LTS (MarcoFalke)
Pull request description:
All supported operating systems ship with python 3.9 (or later), so bumping the minimum should not cause any issues. A bump will allow new code to use new python 3.9 features.
For reference:
* https://packages.debian.org/bullseye/python3
* https://packages.ubuntu.com/focal/python3.9
* FreeBSD 12/13 also ships with 3.9
* CentOS-like 8/9 also ships with 3.9 (and 3.11)
* OpenSuse Leap also ships with 3.9 (and 3.11) https://software.opensuse.org/package/python311-base
This is for Bitcoin Core 27.0 in 2024 (next year), not the soon upcoming 26.0 next month.
ACKs for top commit:
Sjors:
ACK fa25e8b0a1
jamesob:
ACK fa25e8b0a1 ([`jamesob/ackr/28211.1.MarcoFalke.bump_python_minimum_supp`](https://github.com/jamesob/bitcoin/tree/ackr/28211.1.MarcoFalke.bump_python_minimum_supp))
Tree-SHA512: 86c9f6ac4b5ba94a62ee6a6062dd48a8295d8611a39cdb5829f4f0dbc77aaa1a51edccc7a99275bf699143ad3a6fe826de426d413e5a465e3b0e82b86d10c32e
fa6e6a3f03 doc: Remove confusing assert linter (MarcoFalke)
Pull request description:
The `assert()` documentation and linter are redundant and confusing:
* The source code already refuses to compile with `assert()` disabled.
* They violate the assumptions about `Assert()`, which *requires* side effects.
* The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects.
Fix all issues by removing the docs and the linter. See also https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102
Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit.
ACKs for top commit:
hebasto:
ACK fa6e6a3f03, I have reviewed the code and it looks OK.
theStack:
ACK fa6e6a3f03
Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236
edbed31066 chainparams: add signet assumeutxo param at height 160_000 (Sjors Provoost)
b8cafe3871 chainparams: add testnet assumeutxo param at height 2_500_000 (Sjors Provoost)
99839bbfa7 doc: add note about confusing HaveTxsDownloaded name (James O'Beirne)
7ee46a755f contrib: add script to demo/test assumeutxo (James O'Beirne)
42cae39356 test: add feature_assumeutxo functional test (James O'Beirne)
0f64bac603 rpc: add getchainstates (James O'Beirne)
bb05857794 refuse to activate a UTXO snapshot if mempool not empty (James O'Beirne)
ce585a9a15 rpc: add loadtxoutset (James O'Beirne)
62ac519e71 validation: do not activate snapshot if behind active chain (James O'Beirne)
9511fb3616 validation: assumeutxo: swap m_mempool on snapshot activation (James O'Beirne)
7fcd21544a blockstorage: segment normal/assumedvalid blockfiles (James O'Beirne)
4c3b8ca35c validation: populate nChainTx value for assumedvalid chainstates (James O'Beirne)
49ef778158 test: adjust chainstate tests to use recognized snapshot base (James O'Beirne)
1019c39982 validation: pruning for multiple chainstates (James O'Beirne)
373cf91531 validation: indexing changes for assumeutxo (James O'Beirne)
1fffdd76a1 net_processing: validationinterface: ignore some events for bg chain (James O'Beirne)
fbe0a7d7ca wallet: validationinterface: only handle active chain notifications (James O'Beirne)
f073917a9e validationinterface: only send zmq notifications for active (James O'Beirne)
4d8f4dcb45 validation: pass ChainstateRole for validationinterface calls (James O'Beirne)
1e59acdf17 validation: only call UpdatedBlockTip for active chainstate (James O'Beirne)
c6af23c517 validation: add ChainstateRole (James O'Beirne)
9f2318c76c validation: MaybeRebalanceCaches when chain leaves IBD (James O'Beirne)
434495a8c1 chainparams: add blockhash to AssumeutxoData (James O'Beirne)
c711ca186f assumeutxo: remove snapshot during -reindex{-chainstate} (James O'Beirne)
c93ef43e4f bugfix: correct is_snapshot_cs in VerifyDB (James O'Beirne)
b73d3bbd23 net_processing: Request assumeutxo background chain blocks (Suhas Daftuar)
Pull request description:
- Background and FAQ: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
- Prior progress/project: https://github.com/bitcoin/bitcoin/projects/11
- Replaces https://github.com/bitcoin/bitcoin/pull/15606, which was closed due to Github slowness. Original description and commentary can be found there.
---
This changeset finishes the first phase of the assumeutxo project. It makes UTXO snapshots loadable via RPC (`loadtxoutset`) and adds `assumeutxo` parameters to chainparams. It contains all the remaining changes necessary to both use an assumedvalid snapshot chainstate and do a full validation sync in the background.
This may look like a lot to review, but note that
- ~200 lines are a (non-essential) demo shell script
- Many lines are functional test, documentation, and relatively dilute RPC code.
So it shouldn't be as burdensome to review as the linecount might suggest.
- **P2P**: minor changes are made to `init.cpp` and `net_processing.cpp` to make simultaneous IBD across multiple chainstates work.
- **Pruning**: implement correct pruning behavior when using a background chainstate
- **Blockfile separation**: to prevent "fragmentation" in blockfile storage, have background chainstates use separate blockfiles from active snapshot chainstates to avoid interleaving heights and impairing pruning.
- **Indexing**: some `CValidationInterface` events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.
- Have `-reindex` properly wipe snapshot chainstates.
- **RPC**: introduce RPC commands `loadtxoutset` and (hidden) `getchainstates`.
- **Release docs & first assumeutxo commitment**: add notes and a particular assumeutxo hash value for first AU-enabled release.
- This will complete the project and allow use of UTXO snapshots for faster node bootstrap.
The next phase, if it were to be pursued, would be coming up with a way to distribute the UTXO snapshots over the P2P network.
---
### UTXO snapshots
Create your own with `./contrib/devtools/utxo_snapshot.sh`, e.g.
```shell
./contrib/devtools/utxo_snapshot.sh 788000 utxo.dat ./src/bitcoin-cli -datadir=$(pwd)/testdata`)
```
or use the pre-generated ones listed below.
- Testnet: **2'500'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:511e09f4bf853aefab00de5c070b1e031f0ecbe9&dn=utxo-testnet-2500000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `79db4b025448cc0ac388d8589a28eab02de53055d181e34eb47391717aa16388`
- Signet: **160'000** (Sjors):
- torrent: `magnet:?xt=urn:btih:9da986cb27b3980ea7fd06b21e199b148d486880&dn=utxo-signet-160000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
- sha256: `eeeca845385ba91e84ef58c09d38f98f246a24feadaad57fe1e5874f3f92ef8c`
- Mainnet: **800'000** (Sjors):
- Note: this needs the following commit cherry-picked in: 24deb2022b
- torrent: `magnet:?xt=urn:btih:50ee955bef37f5ec3e5b0df4cf0288af3d715a2e&dn=utxo-800000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969`
### Testing
#### For fun (~5min)
If you want to do a quick test, you can run `./contrib/devtools/test_utxo_snapshots.sh` and follow the instructions. This is mostly obviated by the functional tests, though.
#### For real (longer)
If you'd like to experience a real usage of assumeutxo, you can do that too.
I've cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with `./contrib/devtools/utxo_snapshot.sh` if you want). Download that, and then create a datadir for testing:
```sh
$ cd ~/src/bitcoin # or whatever
# get the snapshot
$ curl http://img.jameso.be/utxo-788000.dat > utxo-788000.dat
# you'll want to do this if you like copy/pasting
$ export AU_DATADIR=/home/${USER}/au-test # or wherever
$ mkdir ${AU_DATADIR}
$ vim ${AU_DATADIR}/bitcoin.conf
dbcache=8000 # or, you know, something high
blockfilterindex=1
coinstatsindex=1
prune=3000
logthreadnames=1
```
Obtain this branch, build it, and then start bitcoind:
```sh
$ git remote add jamesob https://github.com/jamesob/bitcoin
$ git fetch jamesob assumeutxo
$ git checkout jamesob/assumeutxo
$ ./configure $conf_args && make # (whatever you like to do here)
# start 'er up and watch the logs
$ ./src/bitcoind -datadir=${AU_DATADIR}
```
Then, in some other window, load the snapshot
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
```
You'll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you'll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you'll see the occasional `[background validation]` log message go by.
In yet another window, you can check out chainstate status with
```sh
$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
```
as well as usual favorites like `getblockchaininfo`.
ACKs for top commit:
achow101:
ACK edbed31066
Tree-SHA512: 6086fb9a38dc7df85fedc76b30084dd8154617a2a91e89a84fb41326d34ef8e7d7ea593107afba01369093bf8cc91770621d98f0ea42a5b3b99db868d2f14dc2
Add the script to the shellcheck exception list since the
quoted variables rule needs to be violated in order to get
bitcoind to pick up on $CHAIN_HACK_FLAGS.
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
