Crazecdwn #1
Open
Crazecdwn #1
+591,980
−218,953
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The chainstate UTXO database only stores unspent outputs; spent entries are removed. Assert after reading a `Coin` so corruption or misuse cannot propagate a spent coin through the `GetCoin()` interface.
Production `GetCoin()` implementations only return unspent coins. Update the `CCoinsView` test backend to match that contract, so tests stop exercising cache states that cannot occur with `CCoinsViewCache` or `CCoinsViewDB`. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
`CoinsViewBottom` roughly simulates a memory-backed `CCoinsViewDB`, which never stores spent coins. Stop returning spent coins from `GetCoin()`, erase spent entries in `BatchWrite()`, and tighten comparisons to expect `std::nullopt` when the simulator has no coin. Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
1808b5a clusterlin: remove unused FixLinearization (cleanup) (Pieter Wuille) 34a7713 txgraph: permit non-topological clusters to defer fixing (optimization) (Pieter Wuille) 3380e0c txgraph: use PostLinearize less prior to linearizing (Pieter Wuille) 62dd886 txgraph: drop NEEDS_SPLIT_ACCEPTABLE (simplification) (Pieter Wuille) 01ffcf4 clusterlin: support fixing linearizations (feature) (Pieter Wuille) Pull request description: Part of #30289, follow-up to #32545. This gets rid of `FixLinearization()` by integrating the functionality into `Linearize()`, and makes txgraph exploit that (by delaying fixing of clusters until their first re-linearization). It also reduces (but does not eliminate) the number of calls to `PostLinearize`, as the SFL linearization effectively performs something very similar to postlinearization when loading in an existing linearization already. ACKs for top commit: instagibbs: reACK 1808b5a marcofleon: code review ACK 1808b5a Tree-SHA512: 81cd9549de2968f5126079cbb532e2cb052ea8157c9c9ce37fd39ad2294105d7c79ee8d946c3d8f7af5b2119299a232c448b42a33e1e43ccc778a5b52957e387
Track what RestoreWallet creates so only those files and directories are removed during a failure and nothing else. Preexisting paths must be left untouched. Note: Using fs::remove_all() instead of fs::remove() in RestoreWallet does not cause any problems currently, but the change is necessary for the next commit which extends RestoreWallet to work with existing directories, which may contain files that must not be deleted.
When migrating any legacy unnamed wallet, a failed migration would cause the cleanup logic to remove its parent directory. Since this type of legacy wallet lives directly in the main '/wallets/' folder, this resulted in unintentionally erasing all wallets, including the backup file. To be fully safe, we will no longer call `fs::remove_all`. Instead, we only erase the individual db files we have created, leaving everything else intact. The created wallets parent directories are erased only if they are empty. As part of this last change, `RestoreWallet` was modified to allow an existing directory as the destination, since we no longer remove the original wallet directory (we only remove the files we created inside it). This also fixes the restore of top-level default wallets during failures, which were failing due to the directory existence check that always returns true for the /wallets/ directory. This bug started after: f6ee59b Previously, the `fs::copy_file` call was failing for top-level wallets, which prevented the `fs::remove_all` call from being reached.
Verifies that a failed migration of the unnamed (default) wallet does not erase the main /wallets/ directory, and also that the backup file exists.
…rune failure The first test verifies that restoring into an existing empty directory or a directory with no .dat db files succeeds, while restoring into a dir with a .dat file fails. The second test covers restoring into the default unnamed wallet (wallet.dat), which also implicitly exercises the recovery path used after a failed migration. The third test covers failure during restore on a prune node. When the wallet last sync was beyond the pruning height.
Right now, after migration the last message users see is "migration completed", but the migration isn't actually finished yet. We still need to load the new wallets to ensure consistency, and if that fails, the migration will be rolled back. This can be confusing for users. This change logs the post-migration loading step and if a wallet fails to load and the migration will be rolled back.
Because the default wallet has no name, the watch-only and solvables wallets created during migration end up having no name either. This fixes it by applying the same prefix name we use for the backup file for an unnamed default wallet. Before: watch-only wallet named "_watchonly" After: watch-only wallet named "default_wallet_watchonly"
fafbc70 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke) Pull request description: It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer. Can be tested via: * Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero` * Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"` * Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server` * Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1` Before: ``` src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned) Invalid parameter, version out of range(1~3) ``` After: ``` JSON integer out of range ACKs for top commit: bensig: ACK fafbc70 achow101: ACK fafbc70 rkrux: utACK fafbc70 theStack: ACK fafbc70 Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
Previously, to determine if a desc is watchonly, `ToPrivateString()`, was used. It returns `false` if there is at least one pubkey in the descriptor for which the provider does not have a private key. ToPrivateString() behaviour will change in the following commits to only return `false` if no priv keys could be found for the pub keys in the descriptor. HavePrivateKeys() is added here to replace the use of ToPrivateString() for determining if a descriptor is 'watchonly'. Co-authored-by: rkrux <rkrux.connect@gmail.com>
ToPrivateString() behaviour will be modified in the following commits. In order to keep the scope of this PR limited to the RPC behaviour, this commit updates wallet migration to use 'Descriptor::HavePrivateKeys()' in place of 'Descriptor::ToPrivateString()' to determine watchonly descriptors. A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the watchonly migration wallet.
This commit modifies the Pubkey providers to return the public string if private data is not available. This is setup for a future commit to make Descriptor::ToPrivateString return strings with missing private key information. Co-authored-by: rkrux <rkrux.connect@gmail.com>
- Refactor Descriptor::ToPrivateString() to allow descriptors with missing private keys to be printed. Useful in descriptors with multiple keys e.g tr() etc. - The existing behaviour of listdescriptors is preserved as much as possible, if no private keys are availablle ToPrivateString will return false
Co-authored-by: rkrux <rkrux.connect@gmail.com>
b7c34d0 test: coverage for migration failure when last sync is beyond prune height (furszy) 82caa81 wallet: migration, fix watch-only and solvables wallets names (furszy) d70b159 wallet: improve post-migration logging (furszy) f011e0f test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy) 36093bd test: add coverage for unnamed wallet migration failure (furszy) f4c7e28 wallet: fix unnamed wallet migration failure (furszy) 4ed0693 wallet: RestoreWallet failure, erase only what was created (furszy) Pull request description: Minimal fix for #34128. The issue occurs during the migration of a legacy unnamed wallet (the legacy "default" wallet). When the migration fails, the cleanup logic is triggered to roll back the state, which involves erasing the newly created descriptor wallets directories. Normally, this only affects the parent directories of named wallets, since they each reside in their own directories. However, because the unnamed wallet resides directly in the top-level `/wallets/` folder, this logic accidentally deletes the main directory. The fix ensures that only the wallet.dat file of the unnamed wallet is touched and restored, preserving the wallet in BDB format and leaving the main `/wallets/` directory intact. #### Story Line: #32273 fixed a different set of issues and, in doing so, uncovered this one. Before the mentioned PR, backups were stored in the same directory as the wallet.dat file. On a migration failure, the backup was then copied to the top-level `/wallets/` directory. For the unnamed legacy wallet, the wallet directory is the `/wallets/` directory, so the source and destination paths were identical. As a result, we threw early in the `fs::copy_file` call ([here](https://github.com/bitcoin/bitcoin/blob/29.x/src/wallet/wallet.cpp#L4572)) because the file already existed, as we were trying to copy the file onto itself. This caused the cleanup logic to abort early on and never reach the removal line. #### Testing Notes: Cherry-pick the test commit on top of master and run it. You will see the failure and realize the reason by reading the test code. ACKs for top commit: achow101: ACK b7c34d0 davidgumberg: crACK b7c34d0 w0xlt: ACK b7c34d0 willcl-ark: ACK b7c34d0 Tree-SHA512: d0be14c0ed6417f999c3f2f429652c2407097d0cc18453c91653e57ae4b5375b327ad3b2553d9ea6ff46a3ae00cdbd5ab325b94eba763072c4fc5a773b85618b
…ir deletion f78f6f1 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow) Pull request description: As pointed out in #34156 (comment), it is possible for `createfromdump` to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error. This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of `fs::remove_all`. ACKs for top commit: waketraindev: lgtm ACK f78f6f1 polespinasa: code review and tACK f78f6f1 rkrux: Code review and tACK f78f6f1 willcl-ark: ACK f78f6f1 pablomartin4btc: ACK f78f6f1 Tree-SHA512: ff1e7668131ec3632c67d990c99e8fddff28605e7e553c7e20695e61017c88476c3636e22f2007e763a00d527e80e4d1d3d45409f6678d28729b8397430bfe7a
The test calls migrate_and_get_rpc(), which sets mock time internally. The caller caches a mock time value and later relies on it to predict the backup filename, so setting the mock time again could cause a naming mismatch. Fix this by calling the migration RPC directly. Since the test expects the migration to fail, migrate_and_get_rpc() is unnecessary here.
…wallet_failure cbf0bd3 test: migration, avoid backup name mismatch in default_wallet_failure (furszy) Pull request description: This is a possible test failure, pushing it in case the CI starts complaining. The change affects only test code; no cpp logic is involved. The `test_default_wallet_failure` migration test calls the function `migrate_and_get_rpc()`, which sets the mock time internally. But, at the same time, the test already caches the mock time value, to later use it in the backup existence check. Setting the mock time twice can lead to a name mismatch during the mentioned check (diff timestamp == diff backup names), which could cause the test to fail. The fix is very simple, just need to call the migration RPC directly. Since the test expects the migration to fail, `migrate_and_get_rpc()` is unnecessary here. I'm surprised the CI hasn't complained about this yet. ACKs for top commit: achow101: ACK cbf0bd3 bensig: ACK cbf0bd3 Tree-SHA512: 10b43a491b8ad0c5bf53e423b7d7587fc631551bf5d598e145e1defe9d8e5786c0869a9aee26209e63ccafd828ece34fc40c75abe246c1301b9f17467d64ef28
They are exactly the same, but the descriptor utils should not prescribe to use the FuzzBufferType. Using a dedicated type for them clarifies that the utils are not tied to FuzzBufferType. Also, while touching the lines, use `const` only where it is meaningful.
… target The same are rejected in the descriptor_parse target, so it makes sense to reject them here as well.
…function 3f5211c test: remove child_one/child_two (w)txid variables (naiyoma) 7cfe790 test: replace ValidWitnessMalleatedTx class with function (naiyoma) 81675a7 test: use pre-generated chain (naiyoma) Pull request description: This PR refactors ` ValidWitnessMalleatedTx` class into a `build_malleated_tx_package` function. As a result, two tests are updated: `mempool_accept_wtxid` and `p2p_p2p_private_broadcast`. Also included are a few small refactors in mempool_accept_wtxid , (switching to MiniWallet, using a pre-mined chain, using txid directly.) Together, these changes reduce complexity and improve test runtime. ACKs for top commit: stratospher: reACK 3f5211c. cedwies: reACK 3f5211c maflcko: review ACK 3f5211c 👥 rkrux: ACK 3f5211c Tree-SHA512: 1fd02be3432fef6b68e54fbe8b15ed56d2699580bb13d0777b21f9cbe4c6d33bbb710541e3ca2fc93eab771d17bf1c427e4b08fa216d561bdb320cc6b36ac8fc
…warn for debug logs) b39291f doc: fix `-logips` description to clarify that non-debug logs can also contain IP addresses (Lőrinc) c7028d3 init: log that additional logs may contain privacy-sensitive information (Lőrinc) 31b771a net: move `privatebroadcast` logs to debug category (Lőrinc) Pull request description: ### Motivation The recently merged [private broadcast](#29415) is a privacy feature, and users may share `debug.log` with support. Unconditional `LogInfo()` messages that mention private broadcast and/or include (w)txids can leak sensitive context (e.g. which transactions a user originated). Since it's meant to be a private broadcast, we should minimize leaks. It's a best effort, it's not invalidated by other logs possibly leaking identifiable information, those can be addressed separately. We're not promising that the logs won't ever contain data that could be used against the user, but we should still try to minimize that data, especially for a feature that's advertised as privacy-focused. Follow up to [#29415 (comment)](#29415 (comment)) ### Changes * Move private-broadcast event logs from `LogInfo()` to `LogDebug(BCLog::PRIVBROADCAST, ...)`, so they are only emitted when `-debug=privatebroadcast` was explicitly provided. * Remove hardcoded `"[privatebroadcast]"` log-string prefixes (category logging already adds the prefix). * Keep warning at the default log level for startup failures. * Add an init log (not a warning since that would require excessive test framework updates) when any `-debug` categories are enabled that additional logs may contain privacy-sensitive information and should not be shared publicly. * Update a related startup arg (`-logips`) to clarify that clarify that non-debug logs can also contain IP addresses. ### Reproducer The new warning can be checked with: ```bash ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 | grep 'Debug logging is enabled' | wc -l 0 ./build/bin/bitcoind -printtoconsole=1 -stopatheight=1 -listen=0 -connect=0 -debug | grep 'Debug logging is enabled' | wc -l 1 ``` ACKs for top commit: janb84: re ACK b39291f vasild: ACK b39291f andrewtoth: ACK b39291f frankomosh: crACK b39291f .The approach and implementation look good. Moving private broadcast logs to debug only would effectively reduce privacy leaks for users sharing logs. sedited: ACK b39291f Tree-SHA512: feca25ebe72a03948ba436e25f9a682947966c4c09627e8f20201ef3872ddbce1c636cd82f06be1afdc09cb80da305058667c0c2eaeadeb351311155325ea06f
Also, fix whitespace in this function, while touching it. Can be reviewed via the git option --ignore-all-space
Add a ci/lint.py script to run the linter both locally or inside the CI (replacing .github/ci-lint-exec.py) which supports running from a worktree. Determines whether we are in a worktree, and mounts the real `.git` directory as a read-only volume if we are.
14f99cf rpc: make `uptime` monotonic across NTP jumps (Lőrinc) a9440b1 util: add `TicksSeconds` (Lőrinc) Pull request description: ### Problem `bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP). This breaks the expectation that uptime reflects process runtime. ### Fix Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC. GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections. ### Reproducer Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`): Or alternatively: ```bash cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc) DATA_DIR=$(mktemp -d) ./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime sleep 1 ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 )) ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime ./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop ``` <details> <summary>Before (uptime jumps with wall clock)</summary> ```bash Bitcoin Core starting 0 20000001 Bitcoin Core stopping ``` </details> <details> <summary>After (uptime stays monotonic)</summary> ```bash Bitcoin Core starting 0 1 Bitcoin Core stopping ``` </details> ---------- Issue: #34326 ACKs for top commit: maflcko: review ACK 14f99cf 🎦 willcl-ark: tACK 14f99cf w0xlt: ACK 14f99cf sedited: ACK 14f99cf Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
Verify that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.
This is used for argument parsing in the retry script, however we don't use the script with any arguments. So remove the unused code, and the dependency on gnu-getopt. This came up in the context of adding new CI jobs, where gnu-getopt might not be available, or working properly. It seemed easier to just remove the unused code, than look for more workarounds.
…nInterfaceQueue e71c4df refactor: replace manual promise with SyncWithValidationInterfaceQueue (ANtutov) Pull request description: `BroadcastTransaction()` now waits for validation callbacks using the built-in `validation_signals>SyncWithValidationInterfaceQueue()` instead of creating a local `std::promise` and scheduling a lambda. This removes an unnecessary allocation and uses the canonical API. ACKs for top commit: maflcko: review ACK e71c4df 🌃 rkrux: lgtm ACK e71c4df sedited: ACK e71c4df Tree-SHA512: 602994ba3c2ac91996068aee6eac7e788c3832d7ab949519a9420d2b59e2a67d2d4e67c3c9191ba60e9caa75f1524a95b0851fcd40b6732f6a9956a011b4a120
…notes fa2e1b8 build: Remove outdated comment about -ffile-prefix-map (MarcoFalke) fa06cd4 doc: Remove outdated -fdebug-prefix-map section in dev notes (MarcoFalke) Pull request description: This removes some docs. See the commit messages for an explanation. ACKs for top commit: l0rinc: ACK fa2e1b8 sedited: ACK fa2e1b8 Tree-SHA512: 6be33bdf9365be5fb75d39a48fd1295b193649775a00e8344123dc0f588da22f7efe80b1490dde2c74aea3d7fec6a3fa75785791296f3fb248ddf45e40b95eb7
Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>
2fccbea Squashed 'src/secp256k1/' changes from d543c0d917..14e56970cb (fanquake) Pull request description: Updates the secp256k1 subtree to latest master (bitcoin-core/secp256k1@14e5697). ACKs for top commit: sedited: ACK 26fbe10 hebasto: ACK 26fbe10. w0xlt: ACK 26fbe10 Tree-SHA512: 51dbd2e5c4574b85064dd0cea67134727487e1363a822e5116ab92d03e2ebde90aa13eddd0f57df613d393e1741eec974e24c7efb4314254b84d8a994bb5b1ef
5aeaa71 lint: pass args from lint.py to cargo run in container (will) c17a2ad lint: upgrade lint scripts for worktrees (will) Pull request description: Fixes #29972 Use a single script to run the linter locally or in CI. Works from inside a worktree. ACKs for top commit: maflcko: review ACK 5aeaa71 🔒 davidgumberg: code review and lightly tested reACK 5aeaa71 l0rinc: Tested (+ lightly reviewed) ACK 5aeaa71 Tree-SHA512: 7c11f649b4752739d31c4f9e6306a98bd2e615b27a0819bbb5e7d9284b9e28bd9f424e145f16361f672f1a63441a1ae2f901c4f99759e997b72a4bf2d56d8d39
…ullptr` args 477c550 coins: replace `std::distance` with unambiguous pointer subtraction (Lőrinc) Pull request description: ### Problem Calling `std::distance(nullptr, nullptr)` has ambiguous status in the C++ standard [iterator.requirements.general](https://eel.is/c++draft/iterator.requirements.general#7): > Iterators can also have singular values that are not associated with any sequence. Results of most expressions are undefined for singular values. It seems to work correctly in every implementation we use, but [LWG 1213](https://cplusplus.github.io/LWG/issue1213) ("Meaning of valid and singular iterator underspecified") has been Open since 2009, acknowledging that the standard's wording on this topic is unclear. <details> <summary>Details</summary> The [iterator.requirements.general](https://eel.is/c++draft/iterator.requirements.general#7) states: > Iterators can also have singular values that are not associated with any sequence. Results of most expressions are undefined for singular values. And [LWG 208](https://cplusplus.github.io/LWG/issue208)'s rationale explicitly confirms: > Null pointers are singular. Therefore they cannot form a valid range required by [std::distance](https://eel.is/c++draft/iterator.operations#4): > Preconditions: last is reachable from first, or InputIterator meets the Cpp17RandomAccessIterator requirements and first is reachable from last. </details> ### Fix A previous version of this PR checked both values for `nullptr`, the current one uses unambiguously well-defined pointer subtraction instead, which is per [expr.add](https://eel.is/c++draft/expr.add#5): > If P and Q both evaluate to null pointer values, the value is 0. This applies on the first call before any memory is allocated, when both pointers are `nullptr`. Using `operator-` directly is simpler and avoids the ambiguity entirely. ACKs for top commit: maflcko: review ACK 477c550 🍶 optout21: ACK 477c550 sedited: ACK 477c550 Tree-SHA512: 5edfb19ab4820e2003928f60f20d4a5893bcd3c316afdfe91c9c06e9b465352769b2cddb0d0e2419ea083a906d35f4aada74149e81f4ea0315f8173ac538789f
fad7bd9 noui: Remove always empty caption while formatting (MarcoFalke) fa8ebeb refactor: [gui] Document that the title is always empty for node message (MarcoFalke) fafe71b refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke) fa8d008 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke) fa01954 refactor: [gui] Use lambdas over std::bind (MarcoFalke) eeee1e3 refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke) Pull request description: Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues: * It is always hard-coded to the empty string. * This is confusing and tedious when reading or maintaining the code. * It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`. * The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used. Fix all issues by removing it. ACKs for top commit: hebasto: ACK fad7bd9, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10. sedited: ACK fad7bd9 Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
ddae1b4 ci: remove gnu-getopt usage (fanquake) Pull request description: This is used for argument parsing in the `retry` script, however we don't use the script with any arguments. So remove the unused code, and the dependency on `gnu-getopt`. This came up in the context of adding new CI jobs, where gnu-getopt might not be available, or working properly. It seemed easier to just remove the unused code, than look for more workarounds. ACKs for top commit: maflcko: review ACK ddae1b4 🔀 sedited: ACK ddae1b4 Tree-SHA512: a73cf61fe0965127f87f1725b3a25a305ebfd354c318f5f44ecfa20da02ba72fef42dca656dae07f6e1ece956b9d7c58e99edb124d968a4bffb2ce6ac8fc018b
3400db8 doc: add missing param description to SRD (yancy) Pull request description: The params documentation is missing `change_fee` and the description is lacking recent changes. ACKs for top commit: murchandamus: ACK 3400db8 brunoerg: code review ACK 3400db8 Tree-SHA512: 8f6fac0d92873c5c9f77b19fbc0c6ecfb425b2a6b3d5f5ad69c82ed706b21cf4627e68c71acbc43661000e6063e8f8dbcd3b8ff60e3c727bdcba497d13ee1383
On merges to master we set LINT_CI_SANITY_CHECK_COMMIT_SIG (when "GITHUB_REPOSITORY == bitcoin/bitcoin") which runs verify-commits.py. This requires write access to the .git directory. Make the mounted .git directory writable. This is currently not run on PR branches or locally which caused a miss during review.
c8abac9 ci: mount .git dir rw (ci) Pull request description: On merges to master we set LINT_CI_SANITY_CHECK_COMMIT_SIG (when "GITHUB_REPOSITORY == bitcoin/bitcoin") which runs verify-commits.py. This requires write access to the .git directory. Make the mounted .git directory writable. This is currently not run on PR branches or locally which caused a miss during review. Ideally we can have the same checks running in PRs as on merges to master to avoid future discrepancies like this. ACKs for top commit: maflcko: lgtm ACK c8abac9 l0rinc: untested code review ACK c8abac9 Tree-SHA512: 7ae4f63227ecffe1dc9003454a7473d6d592550af2e1c899457f34a947e5604b04c13319fb8979f36789ae7787bed62066be60697d163ad5ebedde3fbe8ce45f
…em as errors fdc9fe2 ci, iwyu: Fix warnings in `src/primitives` and treat them as errors (Hennadii Stepanov) Pull request description: This PR [continues](#33725 (comment)) the ongoing effort to enforce IWYU warnings. See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu). ACKs for top commit: maflcko: review ACK fdc9fe2 📀 janb84: ACK fdc9fe2 sedited: ACK fdc9fe2 Tree-SHA512: d290545c7aab477b4a5bf121b694899a78e0526be72efa31fa4205b0fd840e6e8240d32f9134a18c9dc58c5f91e7847d7f20ca34f8d2edc4d541ac858ec0dccc
fa578d9 lint: [move-only] Move python related lints to lint_py.rs (MarcoFalke) fa392c3 lint: [move-only] Move repo related lints to lint_repo_hygiene.rs (MarcoFalke) fab0cfa lint: [move-only] Move cpp related lints to lint_cpp.rs (MarcoFalke) fa3e48e lint: [move-only] Move docs related lints to lint_docs.rs (MarcoFalke) fad09e7 lint: [move-only] Move text related lints to text_format.rs (MarcoFalke) faf40c2 lint: [move-only] Move util functions to util.rs (MarcoFalke) Pull request description: The single, large `main.rs` file is fine, but at some point it becomes harder to read. So reduce the size by pulling functions out into modules. This can be reviewed with the git option: `--color-moved=dimmed-zebra` ACKs for top commit: l0rinc: Lightly tested code review ACK fa578d9 sedited: ACK fa578d9 Tree-SHA512: f1e29fd3cf695fb6634d0b9f9e55508992b4b9885afee9dbe4d5d9e99cad3061e7141f39acbfe69d698422888169128cd7658a6dc991fd904b8520328b51586d
ab649ce guix: documented shasum gathering command (janb84) Pull request description: When a PR requires proof of Guix builds (sha256sums), the PR author or reviewer uses a not well documented command to collect the sha256sums of build outputs or manually gathers them from files. This pull request introduces a new section in the documentation, providing some documentation on the command's functionality and usage. ACKs for top commit: willcl-ark: ACK ab649ce sedited: ACK ab649ce Tree-SHA512: 0188663ad117b636c7d32a1b655db97610f558cfcffe4abd6f0fb097b3990db0dc6d23ab972926fefd2531b21f429742dcbea6b0fa579d22d5da7a7d6a4c753e
…onditionally fa9c92d log: Print warning about privacy-sensitive log info unconditionally (MarcoFalke) Pull request description: There is a warning about logs containing privacy-sensitive information. However, it is only printed when at least one debug log category is enabled. This is confusing, because: * Setting (let's say) `-debug=reindex` enables this warning, but it is hard to see what sensitive logs could be contained in reindex debug logs. * Dropping `-debug=reindex` again disabled this warning, but the wallet continues to log txids (and other sensitive stuff) at info level. So instead of implying the wrong thing, it would be better to remove this log line (because it should be common sense), or log it unconditionally. ACKs for top commit: l0rinc: ACK fa9c92d sedited: ACK fa9c92d Tree-SHA512: 42f71b030e7722203f225f04e979143e829dae3556f64e322a791361a3b9c16150d53bb7bb9a99839c975d9052115770b9473138acc58baeee457253526fd892
905dfde test: use ModuleNotFoundError in interface_ipc.py (fanquake) Pull request description: Change this so we catch the case where the capnp shared libs have been updated, and can no-longer be loaded by the Python module, resulting in a skipped test, even though pycapnp is installed. i.e: ```bash stderr: Traceback (most recent call last): File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 20, in <module> import capnp # type: ignore[import] # noqa: F401 ^^^^^^^^^^^^ File "/usr/local/lib64/python3.14/site-packages/capnp/__init__.py", line 36, in <module> from .version import version as __version__ File "/usr/local/lib64/python3.14/site-packages/capnp/version.py", line 1, in <module> from .lib.capnp import _CAPNP_VERSION_MAJOR as LIBCAPNP_VERSION_MAJOR # noqa: F401 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ImportError: libcapnpc.so.1.0.1: cannot open shared object file: No such file or directory ``` Failing in this way should make it clear that `pycapnp` needs to be reinstalled/rebuilt. If `pycapnp` is not installed, the test still skips as expected: ```bash Remaining jobs: [interface_ipc.py] 1/1 - interface_ipc.py skipped (capnp module not available.) TEST | STATUS | DURATION interface_ipc.py | ○ Skipped | 0 s ``` Fixes: #34016. ACKs for top commit: maflcko: lgtm ACK 905dfde hebasto: ACK 905dfde, I have reviewed the code and it looks OK. However, I'm [not able](#34016 (comment)) to reproduce #34016. sedited: ACK 905dfde Tree-SHA512: 3cedbe8fc51cc18f1c993f7747d20905f3bf94c736db99a9c4090f5823bf8c09dfbc19ef03c573d504dcdfba6ea0f7d088a7f4563b220742c9a441167c04cfd6
…sactions 1f60ca3 wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande) Pull request description: `removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos. The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again. The added test should fail on master. ACKs for top commit: achow101: ACK 1f60ca3 fjahr: tACK 1f60ca3 furszy: utACK 1f60ca3 vasild: ACK 1f60ca3 Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
…t coins 2ee7f9b coins: assume `GetCoin` only returns unspent coins (Andrew Toth) eec551a fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth) 3e4155f test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth) ee1e40f txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins (Lőrinc) Pull request description: This PR is split out from #33018 to keep that PR focused on removing the `FRESH-but-not-DIRTY` cache state. ### Problem `::GetCoin()` is an interface for querying the UTXO set, so production implementations should only ever return unspent coins. Tests should mimic this to provide useful feedback. ### Fix: * Add a fail-fast assertion that `CCoinsViewDB::GetCoin()` never returns a spent coin. * Align unit tests and fuzz simulations with the production `GetCoin()` contract by never returning spent coins. * Replace the unreachable “spent coin returned by parent” handling in `CCoinsViewCache::FetchCoin()` with `Assert(!coin.IsSpent())`, drop outdated `spent+FRESH` docs, and tighten `SanityCheck()` invariants. Behavior is unchanged, it just aligns our tests to exercise valid states. ACKs for top commit: andrewtoth: re-ACK 2ee7f9b optout21: crACK 2ee7f9b achow101: ACK 2ee7f9b w0xlt: reACK 2ee7f9b Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
…ling e770392 test: addrman: test self-announcement time penalty handling (Bruno Garcia) Pull request description: This PR adds a test case for addrman that verifies that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty. It fixes the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L561): ```diff diff --git a/src/addrman.cpp b/src/addrman.cpp index 206b541..c6a045fd8d 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -558,7 +558,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c AddrInfo* pinfo = Find(addr, &nId); // Do not set a penalty for a source's self-announcement - if (addr == source) { + if (addr != source) { time_penalty = 0s; } ``` ACKs for top commit: maflcko: review ACK e770392 🐤 achow101: ACK e770392 fjahr: Code review ACK e770392 naiyoma: tACK e770392 Tree-SHA512: ec029d1e1e979f91840af944984cad530a1ce9a0eceb123230817f0ef3b9ad47253eebc4c953d350de2d904b59496fcd4757123c8bd63cf0e09c3581da48fff8
fad0422 refactor: Remove remaining std::bind, check via clang-tidy (MarcoFalke) Pull request description: `std::bind` has many issues: * It is verbose in a meaningless way * Overriden args are silently accepted and dropped at runtime without a compile error. Same for accidental duplicates. One could use `std::bind_front` similar to commit fa26755. Though, I think the remaining cases are better off with lambdas. So do that here, and enable the `modernize-avoid-bind` clang-tidy rule to avoid `std::bind` bugs in the future. ACKs for top commit: fjahr: Code review ACK fad0422 purpleKarrot: Code review ACK fad0422 Tree-SHA512: 38b17e26eda3ae47d84a8c34298309dc1eeb4ed434fda58b5803ef031c4c2edfb17222f5208f37af727bf340e32b37c7f81784f461d2b65fbc6227f3cd53eea4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.