fa904fc683 lint: Remove confusing, redundant, and brittle lint-spelling (MarcoFalke)
Pull request description:
`codespell` was a fun experiment. However, it has many issues, when used in this project:
* The number of false-positives and true-positives are in the same ballpark. There are also many false-negatives, so the overall net-benefit is questionable.
* There is often confusion around spelling errors leading to a failing CI (they do not, which was intended).
* LLMs released this year are capable to detect typos with less false-positives and less false-negatives, so the `codespell` integration is a bit redundant in that sense.
Fix all issues by removing it.
Going forward, anyone is free to continue to use `codespell`, or any LLM, or any other tool, locally. Also, DrahtBot has the LLM typo linter integrated in the summary comment. I think the options are plenty, and are more than sufficient for now.
ACKs for top commit:
l0rinc:
ACK fa904fc683
rkrux:
ACK fa904fc683
pablomartin4btc:
ACK fa904fc683
Tree-SHA512: 5e2008a77c2c313605f30d73286111eba034a2a6bb2a0a48e2f77ec6ccc7afaa274e00bbfcb727be0ac5e547b8ae9c801d30c43589b0cad2099565e6716b9ec7
5ac3579520 refactor: Add compile-time-checked hex txid (rustaceanrob)
Pull request description:
Suggested by l0rinc as a comment in #34004.
There are tests that utilize `FromHex` that will only fail during runtime if malformed. Adds a compile time constructor that can be caught by LSPs.
ACKs for top commit:
l0rinc:
ACK 5ac3579520
maflcko:
review ACK 5ac3579520🦎
rkrux:
crACK 5ac3579520
Tree-SHA512: b0bae2bf0b8cd8c9a90765a14c46146313cf8b224a29d58a253e65ca95c4205c0beddea9c49ae58901e72c8c5202b91695d074ffb1c48e448d2e5606eb1bd5b4
fa5ed16aa4 move-only: MAX_BLOCK_TIME_GAP to src/qt (MarcoFalke)
Pull request description:
`MAX_BLOCK_TIME_GAP` was used in some incorrect heuristics, which were removed in commit e30b6ea194.
This leaves a single module in src/qt using the constant.
Instead of exposing it in a central kernel header, just move it to the single gui module that uses it.
ACKs for top commit:
sedited:
ACK fa5ed16aa4
hebasto:
ACK fa5ed16aa4, I have reviewed the code and it looks OK.
Tree-SHA512: d0e0e5257f6585d793bfed118d61a3e5d56b2be397fa3b09b34db64e3e018eba9f223cd56541d258b422119fdd7501f07cd3bb8ad5dc28b535922aa21ea76fa6
e7e51952dc contrib: Avoid outputting binary data to TTY (Hodlinator)
Pull request description:
Verify that we wouldn't be writing encoded asmap binary data directly to the TTY since it is the default but makes no sense. (Having stdout as default does make sense when piping to other applications however).
Found while exploring the ASMap data pipeline (https://github.com/asmap/asmap-data/pull/38#pullrequestreview-3547352533) from Kartograf into Bitcoin Core.
ACKs for top commit:
fjahr:
tACK e7e51952dc
sipa:
ACK e7e51952dc
Tree-SHA512: e1ae1ee129715471cbb824268e68cec267d159d4073297af35c06eadfb6b98eeae040beaafeb6489c2853ea9b83cd04471bcd0b27f0ae8fcb377e6e10b4ae6c5
faa8ee62f5 ci: Pin native tests on cross-builds to same commit (MarcoFalke)
Pull request description:
After commit 13809b867a, the native tests may check out a different commit than the cross-build task that produced the artefacts they run on.
Obviously, this may lead to test failures.
Fix it, by first determining a fixed commit, to be used for both the build and the native tests.
An alternative could be to fully or partially revert 13809b867a, but that comes again with the downsides making it harder to detect silent merge conflicts by re-running CI, or clearing unrelated and fixed intermittent test issues by re-running CI. Then, the only alternative would be to close and re-open the pull request.
ACKs for top commit:
janb84:
ACK faa8ee62f5
ryanofsky:
Code review ACK faa8ee62f5. Thanks for the naming & display updates since last review!
hodlinator:
crACK faa8ee62f5
Tree-SHA512: 01391cdfad34e3f2f5b3a6247b1aeb412d023e368bc17572aa66324688439786e79e263288276053ffcfa8521635b07339dbf087b8a30d6670373556a77c22ee
59b93f11e8 rest: print also HTTP response reason in case of an error (Roman Zeyde)
7fe94a0493 rest: add a test for unsuported `/blockpart/` request type (Roman Zeyde)
55d0d19b5c rest: deduplicate `interface_rest.py` negative tests (Roman Zeyde)
89eb531024 rest: update release notes for `/blockpart/` endpoint (Roman Zeyde)
41118e17f8 blockstorage: simplify partial block read validation (Roman Zeyde)
599effdeab rest: reformat `uri_prefixes` initializer list (Roman Zeyde)
Pull request description:
The commits below should resolve a few leftovers from #33657.
ACKs for top commit:
l0rinc:
ACK 59b93f11e8
hodlinator:
re-ACK 59b93f11e8
Tree-SHA512: ae45e08edd315018e11283b354fb32f9658f5829c956554dc662a81c2e16397def7c3700e6354e0a91ff03c850def35638a69ec2668b7c015d25d6fed42b92bb
7e9de20c0c fuzz: exercise `ComputeMerkleRoot` without mutated parameter (Lőrinc)
Pull request description:
The `mutated` parameter in `ComputeMerkleRoot` unlocks a different path that was always exercised in the fuzz test.
Adjusted to be fuzzer to pass `nullptr` as well to make sure that path is also tested: 24ed820d4f/src/consensus/merkle.cpp (L49-L53)
Follow-up to https://github.com/bitcoin/bitcoin/pull/33805#discussion_r2589073735
ACKs for top commit:
frankomosh:
ACK [7e9de20](7e9de20c0c)
hodlinator:
ACK 7e9de20c0c
sedited:
ACK 7e9de20c0c
Tree-SHA512: bf27029ac04003447b24a95544ec863f9ceca6c28d51ea811dd6ca2b412a2a780bb9fdbcdc82719f39dd710a746eb2446263e8377d67a8be52a1694571d03498
d8fe5f0326 test: improve interface_ipc.py waitNext tests (Ryan Ofsky)
a5e61b1917 test: interface_ipc.py minor fixes and cleanup (Ryan Ofsky)
ded11fb04d test: fix interface_ipc.py template destruction (Ryan Ofsky)
Pull request description:
This PR cleans up the `interface_ipc.py` test, fixing broken checks, fixing missing await calls, removing to_dict calls, renaming variables, reducing `.result` accesses, and giving template objects explicit lifetimes. More details are in the commit messages.
The first commit changes a lot of indentation so is easiest to review ignoring whitespace.
ACKs for top commit:
Sjors:
ACK d8fe5f0326
sedited:
ACK d8fe5f0326
Tree-SHA512: f0de309a15cb23f109cf6909e51ddd132a60bd4d4cb25b20bdc74545516670f1cdb0c9cc98c397c2f24e67e2380c2dac9d00435009618a3c00b6b85cca5c3e2e
82be652e40 doc: Improve ChainstateManager documentation, use consistent terms (Ryan Ofsky)
af455dcb39 refactor: Simplify pruning functions (TheCharlatan)
ae85c495f1 refactor: Delete ChainstateManager::GetAll() method (Ryan Ofsky)
6a572dbda9 refactor: Add ChainstateManager::ActivateBestChains() method (Ryan Ofsky)
491d827d52 refactor: Add ChainstateManager::m_chainstates member (Ryan Ofsky)
e514fe6116 refactor: Delete ChainstateManager::SnapshotBlockhash() method (Ryan Ofsky)
ee35250683 refactor: Delete ChainstateManager::IsSnapshotValidated() method (Ryan Ofsky)
d9e82299fc refactor: Delete ChainstateManager::IsSnapshotActive() method (Ryan Ofsky)
4dfe383912 refactor: Convert ChainstateRole enum to struct (Ryan Ofsky)
352ad27fc1 refactor: Add ChainstateManager::ValidatedChainstate() method (Ryan Ofsky)
a229cb9477 refactor: Add ChainstateManager::CurrentChainstate() method (Ryan Ofsky)
a9b7f5614c refactor: Add Chainstate::StoragePath() method (Ryan Ofsky)
840bd2ef23 refactor: Pass chainstate parameters to MaybeCompleteSnapshotValidation (Ryan Ofsky)
1598a15aed refactor: Deduplicate Chainstate activation code (Ryan Ofsky)
9fe927b6d6 refactor: Add Chainstate m_assumeutxo and m_target_utxohash members (Ryan Ofsky)
6082c84713 refactor: Add Chainstate::m_target_blockhash member (Ryan Ofsky)
de00e87548 test: Fix broken chainstatemanager_snapshot_init check (Ryan Ofsky)
Pull request description:
This PR contains the first part of #28608, which tries to make assumeutxo code more maintainable, and improve it by not locking `cs_main` for a long time when the snapshot block is connected, and by deleting the snapshot validation chainstate when it is no longer used, instead of waiting until the next restart.
The changes in this PR are just refactoring. They make `Chainstate` objects self-contained, so for example, it is possible to determine what blocks to connect to a chainstate without querying `ChainstateManager`, and to determine whether a Chainstate is validated without basing it on inferences like `&cs != &ActiveChainstate()` or `GetAll().size() == 1`.
The PR also tries to make assumeutxo terminology less confusing, using "current chainstate" to refer to the chainstate targeting the current network tip, and "historical chainstate" to refer to the chainstate downloading old blocks and validating the assumeutxo snapshot. It removes uses of the terms "active chainstate," "usable chainstate," "disabled chainstate," "ibd chainstate," and "snapshot chainstate" which are confusing for various reasons.
ACKs for top commit:
maflcko:
re-review ACK 82be652e40🕍
fjahr:
re-ACK 82be652e40
sedited:
Re-ACK 82be652e40
Tree-SHA512: 81c67abba9fc5bb170e32b7bf8a1e4f7b5592315b4ef720be916d5f1f5a7088c0c59cfb697744dd385552f58aa31ee36176bae6a6e465723e65861089a1252e5
09dfa4d3f8 test: fix race condition in p2p_v2_misbehaving.py peerid assertion (stratospher)
Pull request description:
Remove the hard-coded peer id from the debug message in `p2p_v2_misbehaving.py`.
asyncio's non-deterministic task scheduling might cause [peer2](938d7aacab/test/functional/p2p_v2_misbehaving.py (L181))'s connection to happen before [peer1](938d7aacab/test/functional/p2p_v2_misbehaving.py (L179))'s. since we test that peer2 [remains connected](938d7aacab/test/functional/p2p_v2_misbehaving.py (L182)), any disconnection must originate from peer1, making the specific peer id not necessary for test correctness. so we can remove the hard coded peer id from the expected debug log message.
Fixes#34035.
ACKs for top commit:
maflcko:
lgtm ACK 09dfa4d3f8
mzumsande:
Code Review ACK 09dfa4d3f8
Tree-SHA512: 542b08ddae09db7454e8c08b1d26aade50a53c2505683df99556cf071a6a38195b64f8700f6db3f4e1b318497fc4b5232246ad4e9d6f3af45fad83e333fa91fb
14371fd1fc gui: Add a menu item to restore then migrate a wallet file (Ava Chow)
f11a7d248c gui: Add restore_and_migrate function to restore then migrate a wallet (Ava Chow)
16ab6dfc10 gui: Move actual migration part of migrate() to its own function (Ava Chow)
4ec2d18a07 wallet, interfaces, gui: Expose load_after_restore parameter (Ava Chow)
Pull request description:
Some users will have a backup of their legacy wallet. These cannot be restored since the "Restore Wallet" action expects to be able to load the wallet after restoring, and this fails for legacy wallets now that they are deleted. Furthermore, the "Migrate Wallet" action only allows users to migrate wallets that are in the wallets directory, so such backups cannot be migrated from the GUI.
This PR resolves this issue by adding a menu item in the "Migrate Wallet" menu which allows users to select their backup file so that it will first be restored but not loaded, and then migrated.
Depends on https://github.com/bitcoin/bitcoin/pull/32620
ACKs for top commit:
hebasto:
ACK 14371fd1fc.
Tree-SHA512: 2b09c012f4c70d0cb283305bf3d1a18ae5a2bfb80977c91544ac1fbc29d6360df49438cfdc8f66661ddb42ddab728c8ef1f9e0d7031877fbd846f9cea957398e
fa8a5d215c log: Remove brittle and confusing LogPrintLevel (MarcoFalke)
fac24bbec8 test: Clarify logging_SeverityLevels test (MarcoFalke)
f273167661 ipc: separate log statements per level (stickies-v)
94c51ae540 libevent: separate log statements per level (stickies-v)
Pull request description:
`LogPrintLevel` has many issues:
* It encourages to log several levels in one source location. This is problematic, because all levels (even warnings and errors) will be rate limited equally for the same location.
* Its warning and error logs are specially formatted compared to all other warning and error logs in the codebase, making them harder to spot (both in the debug log and in the code).
* It is verbose to type and read.
* It is confusing, because the majority of code uses the `Log$LEVEL(...)` macros. Having less ways to achieve the same makes the code more consistent and easier to review.
Fix all issues by removing it
ACKs for top commit:
stickies-v:
re-ACK fa8a5d215c
ajtowns:
ACK fa8a5d215c
pablomartin4btc:
re-ACK fa8a5d215c
Tree-SHA512: 9fbb04962d9c26e566338694a7725b3c0e88ef733322d890bcc6aeddb45266c754e7c885c69bbfebd1588cc09912c6784cfc00e69882f1271a8c87d201490478
a70a14a3f4 refactor: Separate out logic for building a tree-shaped dependency graph (marcofleon)
ce29d7d626 fuzz: Fix variable in `clusterlin_postlinearize_tree` check (marcofleon)
876e2849b4 fuzz: Fix incorrect loop bounds in `clusterlin_postlinearize_tree` (marcofleon)
Pull request description:
Addresses two issues in the `clusterlin_postlinearize_tree` target:
1. The loop iteration while creating tree dependency graphs was incorrect.
2. We were accidentally passing in `post_linearization` to `PostLinearize` instead of the copy we just made, resulting in an ineffective check.
ACKs for top commit:
sipa:
ACK a70a14a3f4
instagibbs:
ACK a70a14a3f4
Tree-SHA512: 2cc1b70d572250d8e7b8db8957ae1f3447f8524c09e638ce08af27ff3f6b7aace3cf834c300f2a7947553cc919e2feedfd64355ff94eb2311fb9cd632cb7358a
Suggested by @l0rinc in #34004
Message by @l0rinc:
This adds a consteval constructor to transaction_identifier (Txid/Wtxid) to allow parsing hex strings at compile-time.
This replaces runtime FromHex checks in tests, ensuring that malformed hardcoded hashes cause build failures rather than runtime test failures.
Test variables are explicitly marked constexpr. This is required to workaround a regression in GCC 14 (Bug 117501) where the compiler incorrectly flags consteval initialization of non-constexpr variables as "statements with no effect".
GCC Bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117501
Reproducer: https://godbolt.org/z/xb5TMaPs6
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
The test was a bit confusing, because it just referred to the "global
log level" without explicitly specifying what it is. The level is set
though the LogSetup constructor. However, it is easier to follow unit
tests, if they are self-contained. So just set the level to Debug
explicitly here.
Also, add a new debug_3 log, to further document the intended behavior
of the unit test.
Also, replace the LogPrintLevel with the shorter and exact replacements
LogTrace and LogDebug.
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "ipc:".
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "libevent:".
The test intends to verify that running `PostLinearize` a
second time on a tree-structured graph doesn't change the
result. But `PostLinearize` was being called on the original
variable, not the copy. So the check was comparing the
unmodified copy against itself, which is useless.
Fix by post-linearizing the correct variable.
The dependency graphs generated by this test can have holes
(unused indices) in them. This means some of the transactions
were skipped when using `depgraph_gen.TxCount()` as the upper
bound of the loop. Switch to using `depgraph.Positions()` to
correctly handle sparse graphs.
due to asyncio's non-deterministic task scheduling, peer2's
connection might happen before peer1's, causing peer2 to get
assigned peer_id=1 on bitcoind side and peer1 to get assigned
peer_id=2 on bitcoind side.
since we test that peer2 remains connected, any disconnection
must originate from peer1, making the specific peer id unnecessary
for test correctness. so we can remove the specific peer_id from
the expected debug log.
5f5c1ea019 net: Cache -capturemessages setting (Anthony Towns)
cea443e246 net: Pass time to InactivityChecks fuctions (Anthony Towns)
Pull request description:
Cuts out some wasted time in net socket handling. First, only calculates the current time once every 50ms, rather than once for each peer, which given we only care about second-level precision seems more than adequate. Second, caches the value of the `-capturemessages` setting in `CConnman` rather than re-evaluating it every time we invoke `PushMessaage`.
ACKs for top commit:
maflcko:
review ACK 5f5c1ea019🏣
vasild:
ACK 5f5c1ea019
sedited:
ACK 5f5c1ea019
mzumsande:
ACK 5f5c1ea019
Tree-SHA512: 0194143a3a4481c6355ac9eab27ce6ae4bed5db1d483ba5d06288dd92f195ccb9f0f055a9eb9d7e16e9bbf72f145eca1ff17c6700ee9aa42730103a8f047b32c
Deduplicate code looping over chainstate objects and calling
ActivateBestChain() and avoid need for code outside ChainstateManager to use
the GetAll() method.
Use to replace m_active_chainstate, m_ibd_chainstate, and m_snapshot_chainstate
members. This has several benefits:
- Ensures ChainstateManager treats chainstates instances equally, making
distinctions based on their attributes, not having special cases and making
assumptions based on their identities.
- Normalizes ChainstateManager representation so states that should be
impossible to reach and validation code has no handling for (like
m_snapshot_chainstate being set and m_ibd_chainstate being unset, or both
being set but m_active_chainstate pointing to the m_ibd_chainstate) can no
longer be represented.
- Makes ChainstateManager more extensible so new chainstates can be added for
different purposes, like indexing or generating and validating assumeutxo
snapshots without interrupting regular node operations. With the
m_chainstates member, new chainstates can be added and handled without needing
to make changes all over validation code or to copy/paste/modify the existing
code that's been already been written to handle m_ibd_chainstate and
m_snapshot_chainstate.
- Avoids terms that are confusing and misleading:
- The term "active chainstate" term is confusing because multiple chainstates
will be active and in use at the same time. Before a snapshot is validated,
wallet code will use the snapshot chainstate, while indexes will use the IBD
chainstate, and netorking code will use both chainstates, downloading
snapshot blocks at higher priority, but also IBD blocks simultaneously.
- The term "snapshot chainstate" is ambiguous because it could refer either
to the chainstate originally loaded from a snapshot, or to the chainstate
being used to validate a snapshot that was loaded, or to a chainstate being
used to produce a snapshot, but it is arbitrary used to refer the first
thing. The terms "most-work chainstate" or "assumed-valid chainstate" should
be less ambiguous ways to refer to chainstates loaded from snapshots.
- The term "IBD chainstate" is not just ambiguous but actively confusing
because technically IBD ends and the node is considered synced when the
snapshot chainstate finishes syncing, so in practice the IBD chainstate
will mostly by synced after IBD is complete. The term "fully-validated" is
a better way of describing the characteristics and purpose of this
chainstate.
SnapshotBlockhash() is only called two places outside of tests, and is used
redundantly in some tests, checking the same field as other checks. Simplify by
dropping the method and using the m_from_snapshot_blockhash field directly.
IsSnapshotValidated() is only called one place outside of tests, and is use
redundantly in some tests, asserting that a snapshot is not validated when a
snapshot chainstate does not even exist. Simplify by dropping the method and
checking Chainstate m_assumeutxo field directly.
IsSnapshotActive() method is only called one place outside of tests and
asserts, and is confusing because it returns true even after the snapshot is
fully validated.
The documentation which said this "implies that a background validation
chainstate is also in use" is also incorrect, because after the snapshot is
validated, the background chainstate gets disabled and IsUsable() would return
false.
Change ChainstateRole parameter passed to wallets and indexes. Wallets and
indexes need to know whether chainstate is historical and whether it is fully
validated. They should not be aware of the assumeutxo snapshot validation
process.
CurrentChainstate() is basically the same as ActiveChainstate() except it
requires cs_main to be locked when it is called, instead of locking cs_main
internally.
The name "current" should also be less confusing than "active" because multiple
chainstates can be active, and CurrentChainstate() returns the chainstate
targeting the current network tip, regardless of what chainstates are being
downloaded or how they are used.
Use to simplify code determining the chainstate leveldb paths. New method is
the now the only code that needs to figure out the storage path, so the path
doesn't need to be constructed multiple places and backed out of leveldb.
Remove hardcoded references to m_ibd_chainstate and m_snapshot_chainstate so
MaybeCompleteSnapshotValidation function can be simpler and focus on validating
the snapshot without dealing with internal ChainstateManager states.
This is a step towards being able to validate the snapshot outside of
ActivateBestChain loop so cs_main is not locked for minutes when the snapshot
block is connected.
Move duplicate code from ChainstateManager::ActivateSnapshot and
ChainstateManager::ActivateExistingSnapshot methods to a new
ChainstateManager::AddChainstate method.
The "AddChainstate" method name doesn't mention snapshots even though it is
only used to add snapshot chainstates now, because it becomes more generalized
in a later commit in this PR ("refactor: Add ChainstateManager::m_chainstates
member")
Get rid of m_disabled/IsUsable members. Instead of marking chains disabled for
different reasons, store chainstate assumeutxo status explicitly and use that
information to determine how chains should be treated.
Make Chainstate objects aware of what block they are targeting. This makes
Chainstate objects more self contained, so it's possible for validation code to
look at one Chainstate object and know what blocks to connect to it without
needing to consider global validation state or look at other Chainstate
objects.
The motivation for this change is to make validation and networking code more
readable, so understanding it just requires knowing about chains and blocks,
not reasoning about assumeutxo download states. This change also enables
simplifications to the ChainstateManager interface in subsequent commits, and
could make it easier to implement new features like creating new Chainstate
objects to generate UTXO snapshots or index UTXO data.
Note that behavior of the MaybeCompleteSnapshotValidation function is not
changing here but some checks that were previously impossible to trigger like
the BASE_BLOCKHASH_MISMATCH case have been turned into asserts.
The following test code never checked anything because the if statement was
always false:
if (cs != &chainman_restarted.ActiveChainstate()) {
BOOST_CHECK_EQUAL(cs->m_chain.Height(), 109);
}
Also, the height of the background chainstate it was intending to check is 110,
not 109. Fix both problems by rewriting the check.
