`UndoWriteToDisk` wasn't really extracting a meaningful subset of the `WriteUndoDataForBlock` functionality, it's tied closely to the only caller (needs the header size twice, recalculated undo serializes size, returns multiple branches, modifies parameter, needs documentation).
The inlined code should only differ in these parts (modernization will be done in other commits):
* renamed `_pos` to `pos` in `WriteUndoDataForBlock` to match the parameter name;
* inlined `hashBlock` parameter usage into `hasher << block.pprev->GetBlockHash()`;
* changed `return false` to `return FatalError`.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
fadd568931 fuzz: Fix misplaced SeedRand::ZEROS (MarcoFalke)
Pull request description:
After commit fae63bf130 this must be placed even before test_setup. This is nice, because it makes the usage consistently appear in the first line.
The change is moving a `SeedRandomForTest(SeedRand::ZEROS)` to happen earlier. This is fine, because it will either have no effect, or make the code more deterministic, because after commit fae63bf, no other re-seeding other than `ZEROS` can happen in fuzz tests.
ACKs for top commit:
marcofleon:
Re ACK fadd568931
brunoerg:
code review ACK fadd568931
hodlinator:
ACK fadd568931
Tree-SHA512: 54eadf19a1e850157a280fb252ece8797f37a9a50d3b0a01aa2c267bacbe8ef4ddea6cf3faadcbaa4ab9f53148edf08e3cee5dfb3eae928db582adf8373a5206
81cea5d4ee Ensure m_tip_block is never ZERO (Sjors Provoost)
e058544d0e Make m_tip_block an std::optional (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1844244309
ACKs for top commit:
fjahr:
re-ACK 81cea5d4ee
tdb3:
code review re ACK 81cea5d4ee
l0rinc:
ACK 81cea5d4ee
Tree-SHA512: 31a75ba29e3d567bab32e4e7925a419d9d7a4d2d85ed1c1012116d8d22adc14d31d5b4ce5f6c499c994188dcd26a01cced05be74f94c892fc90ae17a6783a472
c991cea1a0 Remove processNewBlock() from mining interface (Sjors Provoost)
9a47852d88 Remove getTransactionsUpdated() from mining interface (Sjors Provoost)
bfc4e029d4 Remove testBlockValidity() from mining interface (Sjors Provoost)
Pull request description:
There are three methods in the mining interface that can be dropped. The Template Provider doesn't need them and other application should probably not use them either.
1. `processNewBlock()` was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
Dropping it was suggested in https://github.com/bitcoin/bitcoin/pull/30200#issuecomment-2404460342
2. `getTransactionsUpdated()`: this is used in the implementation of #31003 `waitFeesChanged`. It's not very useful generically because the mempool updates very frequently.
3. `testBlockValidity()`: it might be useful for mining application to have a way to check the validity of a block template they modified, but the Stratum v2 Template Provider doesn't do that, and this method is a bit brittle (e.g. the block needs to build on the tip).
ACKs for top commit:
TheCharlatan:
Re-ACK c991cea1a0
ryanofsky:
Code review ACK c991cea1a0. Since last review, just rebased to avoid conflicts in surrounding code, and edited a commit message
tdb3:
code review ACK c991cea1a0
Tree-SHA512: 2138e54f920b26e01c068b24498c6a210c5c4358138dce0702ab58185d9ae148a18f04c97ac9f043646d40f8031618d80a718a176b1ce4779c237de6fb9c4a67
facb4d010c refactor: Move GuessVerificationProgress into ChainstateManager (MarcoFalke)
Pull request description:
Currently the function is standalone, which means any passed-in data like `TxData` or the block pointer needs to be taken from the `ChainstateManager` and passed in. This is currently verbose and may become even more verbose if the function is reworked in the future. As the function can not be called without a `ChainstateManager` in production code anyway, make it a member function on the class.
ACKs for top commit:
ryanofsky:
Code review ACK facb4d010c. Nice cleanup, that should make this code less awkward to work with
TheCharlatan:
ACK facb4d010c
danielabrozzoni:
reACK facb4d010c
Tree-SHA512: b17977e12cd7c6e308c47e6a1aa920acecd4442696e46d1f30bd7c201e9898ca2d581ff0bf2cc9f7334e146c1b0c50925adb849c8c17f65dcdf6877be1c5f776
processNewBlock was added in 7b4d3249ce, but became unnecessary with the introduction of interfaces::BlockTemplate::submitSolution in 7b4d3249ce.
getTransactionsUpdated() is only needed by the implementation of waitFeesChanged() (not yet part of the interface).
fa9e0489f5 refactor: Use immediate lambda to work around GCC bug 117966 (MarcoFalke)
Pull request description:
Currently the libstdc++ debug mode can only be used with version 11, or 15 (and later), due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117966
This seems restrictive.
Add a temporary workaround for now, which makes the global (temporary) `std::span` local to a lambda.
ACKs for top commit:
theuni:
utACK fa9e0489f5
hebasto:
ACK fa9e0489f5, tested on Ubuntu 24.10.
vasild:
ACK fa9e0489f5
Tree-SHA512: 0cc54f089f329592f7a92a6f938b7de46c92d5362615310748225a42789e858e871432721e3101271b00871d523af5fbaadba2f52433fe79e928b1d1253931f6
fae63bf130 fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457 fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab fuzz: Add missing SeedRandomStateForTest(SeedRand::ZEROS) (MarcoFalke)
Pull request description:
This is the first step toward improving fuzz stability and determinism (https://github.com/bitcoin/bitcoin/issues/29018).
A fuzz target using the global test-only PRNG will now abort if the seed is re-used across fuzz inputs.
Also, temporarily add `SeedRandomStateForTest(SeedRand::ZEROS)` to all affected fuzz targets. This may slow down the libfuzzer leak detector, but it will disable itself after some time, or it can be disabled explicitly with `-detect_leaks=0`.
In a follow-up, each affected fuzz target can be stripped of the global random use and a local `RandomMixin` (or similar) can be added instead.
(Can be tested by removing any one of the re-seed calls and observing a fuzz abort)
ACKs for top commit:
hodlinator:
ACK fae63bf130
dergoegge:
utACK fae63bf130
marcofleon:
Tested ACK fae63bf130
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
f86678156a Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d57288246 refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d90 Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6 Rename merkle branch to path (Sjors Provoost)
Pull request description:
This PR implements the refactors suggested in https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2354931253.
ACKs for top commit:
tdb3:
code review re-ACK f86678156a
itornaza:
re ACK f86678156a
ryanofsky:
Code review ACK f86678156a only changes since last review are a whitespace change and adding an Assume statement to check for size_t -> uint32_t overflows
Tree-SHA512: 661b5d5d0e24b2269bf33ab1484e37c36e67b32a7796d77ca3b1856d3043378b081ad43c32a8638b46fa8c0de51c823fd9747dd9fc81f958f20d327bf330a47c
52fd1511a7 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede4 rpc: rename coinbase_script to coinbase_output_script (Sjors Provoost)
Pull request description:
Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.
Production miners use the `getblocktemplate` RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.
This commit removes the `script_pub_key argument` from `createNewBlock()` in the Mining interface.
A coinbase script can still be passed via `BlockCreateOptions` instead. Tests are modified to do so.
ACKs for top commit:
ryanofsky:
Code review ACK 52fd1511a7. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a7
vasild:
ACK 52fd1511a7
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
To avoid future code changes from reintroducing the ambiguity fixed
by the previous commit, mark m_tip_block private and Assume that
it's not set to uint256::ZERO.
Belt and suspenders for future code changes.
Currently this function is only called from TransactionMerklePath() which sets leaves to the block transactions, so the Assume always holds.
The TARGET_OBJECTS generator expression was introduced in the staging
branch when we aimed to build the libbitcoinconsensus shared library.
However, `bitcoin_consensus` is a STATIC library, not an OBJECT library.
This change updates the build system to link `bitcoin_consensus`
normally to `test_bitcoin`, resolving linking issues when building with
clang-cl.
1dd3af8fbc Add release note for #31223 (Martin Zumsande)
997757dd2b test: add functional test for -port behavior (Martin Zumsande)
0e2b12b92a net, init: derive default onion port if a user specified a -port (Martin Zumsande)
Pull request description:
This resolves#31133 (setups with multiple local nodes each using a different `-port` no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by `-port` incremented by 1 (idea by vasild / laanwj).
Note that with this fix, the chosen `-port` values of two local nodes cannot be adjacent, otherwise there will be port collisions again.
From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use `-bind` instead of `-port`. But more opinions are certainly welcome!
I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR.
Fixes#31133
ACKs for top commit:
achow101:
ACK 1dd3af8fbc
laanwj:
Tested ACK 1dd3af8fbc
tdb3:
Code review ACK 1dd3af8fbc
Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40
62b2d23edb wallet: Migrate non-HD keys to combo() descriptor (Ava Chow)
Pull request description:
Non-HD keys do not have an HD seed ID associated with them, so if this value is the null value (all 0s), then we should not perform any seed ID comparison that would result in excluding the keys from combo() migration.
This changes the migration of non-HD wallets (or blank wallets with imported private keys) to make a single combo() descriptors for the non-HD/imported keys, rather than pk(), pkh(), sh(wpkh()), and wpkh() descriptors for the keys.
Implements https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1876650074
ACKs for top commit:
laanwj:
Concept and code review ACK 62b2d23edb
brunoerg:
code review ACK 62b2d23edb
furszy:
Nice catch. ACK 62b2d23edb
theStack:
ACK 62b2d23edb
rkrux:
tACK 62b2d23edb
Tree-SHA512: 86a80b7dcc1598ab18068a2572ff4b4920b233178b760f7b76c5b21a9e6608005ac872f90e082a8f99b51daab0b049e73e4bee5b8e0b537d56ed0d34122a1f49
c93bf0e6e2 test: Add missing %c character test (Hodlinator)
76cca4aa6f test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba2 test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a465995 refactor test: Profit from using namespace + using detail function (Hodlinator)
Pull request description:
Clarifies and puts the extent of parity under test.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
ACKs for top commit:
maflcko:
re-ACK c93bf0e6e2 🗜
l0rinc:
ACK c93bf0e6e2
ryanofsky:
Code review ACK c93bf0e6e2. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
bb7e686341 fuzz: add cstdlib to FuzzedDataProvider (fanquake)
Pull request description:
Same as https://github.com/llvm/llvm-project/pull/113951.
Avoids compile failures under clang-20 & `D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`:
```bash
In file included from /bitcoin/src/test/fuzz/addition_overflow.cpp:5:
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:209:5: error: use of undeclared identifier 'abort'
209 | abort();
| ^
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:250:5: error: use of undeclared identifier 'abort'
250 | abort();
```
ACKs for top commit:
dergoegge:
utACK bb7e686341
brunoerg:
ACK bb7e686341
Tree-SHA512: 22efd5505273ec7254e8dccbb275e648fe02107397c45eff6752e4a6ea787d9d2e45eb0f2ee309df431e9b92ffd14cbcba4b0f4b11a127664466e20be43c383e
Non-HD keys in legacy wallets without a HD seed ID were being migrated
to separate pk(), pkh(), sh(wpkh()), and wpkh() descriptors for each key.
These could be more compactly represented as combo() descriptors, so
migration should make combo() for them.
It is possible that existing non-HD wallets that were migrated, or
wallets that started blank and had private keys imported into them have
run into this issue. However, as the 4 descriptors produce the same output
scripts as the single combo(), so any previously migrated wallets are
not missing any output scripts. The only observable difference should be
performance related, and the wallet size on disk.
cdd207c0e4 test: add coverage for migrating standalone imported keys (furszy)
297a876c98 test: add coverage for migrating watch-only script (furszy)
932cd1e92b wallet: fix crash during watch-only wallet migration (furszy)
Pull request description:
The crash occurs because we assume the cached scripts structure will not be empty,
but it can be empty for watch-only wallets that start blank.
This also adds test coverage for standalone imported keys, which were also crashing
because pubkey imports are treated the same way as hex script imports through
`importaddress()`.
Testing Notes:
This can be verified by cherry-picking and running any of the test commits on master.
It will crash there but pass on this branch.
ACKs for top commit:
theStack:
re-ACK cdd207c0e4
brunoerg:
reACK cdd207c0e4
achow101:
ACK cdd207c0e4
Tree-SHA512: e05c77cf3e9f35f10f122a73680b3f131f683c56685c1e26b5ffc857f95195b64c8c9d4535960ed3d6f931935aa79b0b1242537462006126bdb68251f0452954
Same as https://github.com/llvm/llvm-project/pull/113951.
Avoids compile failures under clang-20 &
`D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`:
```bash
In file included from /bitcoin/src/test/fuzz/addition_overflow.cpp:5:
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:209:5: error: use of undeclared identifier 'abort'
209 | abort();
| ^
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:250:5: error: use of undeclared identifier 'abort'
250 | abort();
```
31e59d94c6 iwyu: Drop backported mapping (Hennadii Stepanov)
fe9bc5abef ci: Update Clang in "tidy" job (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.
New "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been addressed.
ACKs for top commit:
maflcko:
lgtm ACK 31e59d94c6
l0rinc:
ACK 31e59d94c6
theuni:
ACK 31e59d94c6
Tree-SHA512: ae0ca150673e1bfa78664f2ef35dbc965094b32374cafeeae390c6d368c28169a7f7790debe9a6eeb5efc39c9a468f5032d92f30cc4032b09d8265f6a75de882
- For "%n", which is supposed to write to the argument for printf.
- For string/integer mismatches of width/precision specifiers.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
Also adds BOOST_CHECK_NO_THROW() while touching that line, clarifying part of what we are checking for.
Also removed redundant inline from template functions in .cpp file.
The crash occurs because we assume the cached scripts
structure will not be empty, but it can be empty when
the legacy wallet contained only watch-only and
solvable but not spendable scripts
0184d33b3d scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf) (Ryan Ofsky)
006e4d1d59 refactor: Use + instead of strformat to concatenate translated & untranslated strings (Ryan Ofsky)
831d2bfcf9 refactor: Don't embed translated string in untranslated string. (Ryan Ofsky)
058021969b refactor: Avoid concatenation of format strings (Ryan Ofsky)
Pull request description:
This PR cleans up string formatting in the codebase so other PRs adding compile time checking can be simpler and easier to review (specifically #30928, #31061, #31074, and #31149).
Currently these PRs are hard to review because in addition to changing formatting APIs, they have to update callers that are using the API's in unusual ways. Clean up these callers now so later PRs can be simpler. Specifically:
- Use string literals instead of `std::string` format strings to enable more compile-time checking.
- Avoid using untranslated bilingual strings as format strings. Use originals so they can by checked at compile time.
- Favor `Untranslated(strprintf(...))` over `strprintf(Untranslated(...), ...)` for consistency and to prevent translated and untranslated strings from being unintentionally combined.
ACKs for top commit:
maflcko:
lgtm ACK 0184d33b3d🔹
l0rinc:
ACK 0184d33b3d - no overall difference because of the rebase
Tree-SHA512: 37eb771187d30977f5f054eddb82af6dd69878ace89cede72032bb389e57200898119f9fe486ce6903ebb00cb24648be215ab3e44842b3d206b35e26038da750
edb41e4814 util: use explicit cast in MultiIntBitSet::Fill() (Vasil Dimov)
Pull request description:
The current code does not have a bug, but is implicitly casting -1 to 65535 and the sanitizer has no way to know whether we intend that or not.
```
FUZZ=bitset src/test/fuzz/fuzz /tmp/fuz
error: implicit conversion from type 'int' of value -1 (32-bit, signed)
to type 'value_type' (aka 'unsigned short') changed the value to 65535
(16-bit, unsigned)
Base64: Qv7bX/8=
```
https://api.cirrus-ci.com/v1/task/5685829642747904/logs/ci.log
ACKs for top commit:
sipa:
ACK edb41e4814
maflcko:
lgtm ACK edb41e4814
Empact:
ACK edb41e4814
tdb3:
code review ACK edb41e4814
Tree-SHA512: a53835d654d9a7246ec0dab30fa5fbc08155dadb40d9bee3297060aa90816e0ce3d3e92dbdcd7af9474446d842d03f2781b7645a68ffef7fb5fc32ee02545112
55347a5018 test: Rework migratewallet to use previous release (v28.0) (Ava Chow)
f42ec0f3bf wallet: Check specified wallet exists before migration (Ava Chow)
Pull request description:
This PR reworks wallet_migration.py to use previous releases to produce legacy wallets for testing so that the test will continue to work once legacy wallets are removed.
Split from #28710
ACKs for top commit:
maflcko:
re-ACK 55347a5018🥊
rkrux:
re-ACK 55347a5
Tree-SHA512: f90a2f475febc73d29e8ad3cb20d134c368a40a3b5934c3e4aaa77ae704af6314d4dd2e85c261142bd60a201902ac4ba00b8e2443d3cef7c8cc45d23281fa831
The current code does not have a bug, but is implicitly casting -1 to
65535 and the sanitizer has no way to know whether we intend that or
not.
```
FUZZ=bitset src/test/fuzz/fuzz /tmp/fuz
error: implicit conversion from type 'int' of value -1 (32-bit, signed)
to type 'value_type' (aka 'unsigned short') changed the value to 65535
(16-bit, unsigned)
Base64: Qv7bX/8=
```
