| Age | Commit message (Collapse) | Author |
|---|
|
GCC bug 117966
fa9e0489f57968945d54ef56b275f51540f3e5e4 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 fa9e0489f57968945d54ef56b275f51540f3e5e4
hebasto:
ACK fa9e0489f57968945d54ef56b275f51540f3e5e4, tested on Ubuntu 24.10.
vasild:
ACK fa9e0489f57968945d54ef56b275f51540f3e5e4
Tree-SHA512: 0cc54f089f329592f7a92a6f938b7de46c92d5362615310748225a42789e858e871432721e3101271b00871d523af5fbaadba2f52433fe79e928b1d1253931f6
|
|
fae63bf13033adec80c7e6d73144a21ea3cfbc6d fuzz: Clarify that only SeedRandomStateForTest(SeedRand::ZEROS) is allowed (MarcoFalke)
fa18acb457e91cc0fa6b3640b6b55c6bc61572ee fuzz: Abort when using global PRNG without re-seed (MarcoFalke)
fa7809aeab838752af94c52977936a8c6555d315 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 fae63bf13033adec80c7e6d73144a21ea3cfbc6d
dergoegge:
utACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d
marcofleon:
Tested ACK fae63bf13033adec80c7e6d73144a21ea3cfbc6d
Tree-SHA512: 4a0db69af7f715408edf4f8b08b44f34ce12ee2c79d33b336ad19a6e6bd079c4ff7c971af0a3efa428213407c1171f4e2837ec6a2577086c2f94cd15618a0892
|
|
f86678156a3d3ce6488b383a2d6d91d28fcd2b73 Check leaves size maximum in MerkleComputation (Sjors Provoost)
4d572882463b20818fcfbd0a2f6fa6c0168e4e4a refactor: use CTransactionRef in submitSolution (Sjors Provoost)
2e81791d907288c174aa05dc1b3816e6d988127c Drop TransactionMerklePath default position arg (Sjors Provoost)
39d3b538e6a2af8db85077e958970cdcd0ee7f7d 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 f86678156a3d3ce6488b383a2d6d91d28fcd2b73
itornaza:
re ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73
ryanofsky:
Code review ACK f86678156a3d3ce6488b383a2d6d91d28fcd2b73 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
|
|
52fd1511a774d1ff9e747b2ce88aa1fcb778ced8 test: drop scriptPubKeyIn arg from CreateNewBlock (Sjors Provoost)
ff41b9e296aba0237e068fab181523c50bf8c9d0 Drop script_pub_key arg from createNewBlock (Sjors Provoost)
7ab733ede444aee61ab52670c228eec811268c6f 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 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8. No change since last review other than rebase
TheCharlatan:
Re-ACK 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
vasild:
ACK 52fd1511a774d1ff9e747b2ce88aa1fcb778ced8
Tree-SHA512: c4b3a53774d9a5dc90950e77f47a64dbb68f971baffbb9a0d8f59332ef8e52d0c039130c925bde73135b3d0e79e65d91d1df30dc4cff13f32d8a72e5c56669d8
|
|
|
|
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.
|
|
specified a -port
1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4 Add release note for #31223 (Martin Zumsande)
997757dd2b4d7b20b17299fbd21970b2efb8bbc8 test: add functional test for -port behavior (Martin Zumsande)
0e2b12b92a28a2949e75bf50f31563f52e647d6e 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 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
laanwj:
Tested ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
tdb3:
Code review ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40
|
|
37946c0aafeebc1585f1316fb05f252f7fb51e91 Set notifications m_tip_block in LoadChainTip() (Sjors Provoost)
Pull request description:
Ensure KernelNotifications `m_tip_block` is set even if no new block arrives.
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486457573
ACKs for top commit:
ryanofsky:
Code review ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91, fixing comment bug caught by @mzumsande in https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1870315593 in another really helpful clarification
mzumsande:
Code Review ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91
TheCharlatan:
ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91
Tree-SHA512: 931bf820440a0cdda276f6dbd63f03fdbcdc90b18e7d5e160a74bdd9d0290acc706c35aab15bbdcd6e5e0b77565b3d07ff49b0dcf6551cb83961bae67be5d1bb
|
|
|
|
62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8 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 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
brunoerg:
code review ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
furszy:
Nice catch. ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
theStack:
ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
rkrux:
tACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
Tree-SHA512: 86a80b7dcc1598ab18068a2572ff4b4920b233178b760f7b76c5b21a9e6608005ac872f90e082a8f99b51daab0b049e73e4bee5b8e0b537d56ed0d34122a1f49
|
|
|
|
Signed-off-by: RiceChuan <lc582041246@gmail.com>
|
|
ConstevalFormatString/tinyformat parity
c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 test: Add missing %c character test (Hodlinator)
76cca4aa6fcd363dee821c837b1b627ea137b7a4 test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba20664e3581a8e4633cc188d5be3175a test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a4659950a6c4e22316f66b55cae8afc4f4d9a 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 c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 🗜
l0rinc:
ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1
ryanofsky:
Code review ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
|
|
bb7e686341e437b2e7aae887827710918c00ae0f 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 bb7e686341e437b2e7aae887827710918c00ae0f
brunoerg:
ACK bb7e686341e437b2e7aae887827710918c00ae0f
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.
|
|
migration
cdd207c0e48081ab13e2c8c9886f3e2d5da1857e test: add coverage for migrating standalone imported keys (furszy)
297a876c9809e267e37481fc776cbec90056b078 test: add coverage for migrating watch-only script (furszy)
932cd1e92b6d39c6879f546867698bc8441d09cd 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 cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
brunoerg:
reACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
achow101:
ACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
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();
```
|
|
31e59d94c67b58f24dd701fc7ccfee7d79f311cb iwyu: Drop backported mapping (Hennadii Stepanov)
fe9bc5abef3dcbe0f6395c3233f9d122579cd1f0 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 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
l0rinc:
ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
theuni:
ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
Tree-SHA512: ae0ca150673e1bfa78664f2ef35dbc965094b32374cafeeae390c6d368c28169a7f7790debe9a6eeb5efc39c9a468f5032d92f30cc4032b09d8265f6a75de882
|
|
Proves tinyformat doesn't trigger an exception for \0 characters.
Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
|
|
- 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>
|
|
Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
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
|
|
bilingual_str usages
0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf) (Ryan Ofsky)
006e4d1d5984d841c9ac0a6f3c40cfd51e774eda refactor: Use + instead of strformat to concatenate translated & untranslated strings (Ryan Ofsky)
831d2bfcf94117957a90f60fa5bc84a53bb61f7c refactor: Don't embed translated string in untranslated string. (Ryan Ofsky)
058021969b542fc865d17d22fa21e48c9abe4a6e 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 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 🔹
l0rinc:
ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 - no overall difference because of the rebase
Tree-SHA512: 37eb771187d30977f5f054eddb82af6dd69878ace89cede72032bb389e57200898119f9fe486ce6903ebb00cb24648be215ab3e44842b3d206b35e26038da750
|
|
edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf 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 edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
maflcko:
lgtm ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
Empact:
ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
tdb3:
code review ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
Tree-SHA512: a53835d654d9a7246ec0dab30fa5fbc08155dadb40d9bee3297060aa90816e0ce3d3e92dbdcd7af9474446d842d03f2781b7645a68ffef7fb5fc32ee02545112
|
|
Ensure KernelNotifications m_tip_block is set even if no new block arrives.
Additionally, have node init always wait for this to happen.
|
|
previous releases
55347a5018b2c252c56548f0a6dc1e88e42f66b6 test: Rework migratewallet to use previous release (v28.0) (Ava Chow)
f42ec0f3bfbea8c2cd0c02f4b4d64b71ded6d081 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 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊
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=
```
|
|
This change switches to the latest IWYU 0.23, which is compatible with
Clang 19.
Fixed new "modernize-use-starts-ends-with" warnings.
The new "bugprone-use-after-move" warning in `result_tests.cpp` is a
false positive caused by a bug in Boost.Test versions < 1.87. This has
been addressed by introducing a local variable.
See upstream references:
- Issue: https://github.com/boostorg/test/issues/343
- Fix: https://github.com/boostorg/test/pull/348
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
|
|
success so `isTerrible=true`
1807df3d9fb0135057a33e01173080ea14b0403d test: addrman: tried 3 times and never a success so `isTerrible=true` (brunoerg)
Pull request description:
This PR adds test coverage for the following verification:
```cpp
if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
return true;
}
```
If we've tried an address for 3 or more times and were unsuccessful, this address should be pointed out as "terrible".
-------
You can test this by applying:
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 054a9bee32..93a9521b59 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -81,7 +81,7 @@ bool AddrInfo::IsTerrible(NodeSeconds now) const
}
if (TicksSinceEpoch<std::chrono::seconds>(m_last_success) == 0 && nAttempts >= ADDRMAN_RETRIES) { // tried N times and never a success
- return true;
+ return false;
}
```
ACKs for top commit:
jonatack:
re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d
naumenkogs:
ACK 1807df3d9fb0135057a33e01173080ea14b0403d
achow101:
ACK 1807df3d9fb0135057a33e01173080ea14b0403d
Tree-SHA512: e3cc43c98bddfe90f585d5b4bd00543be443b77ecaf038615261aa8cc4d14fc2f1006d0b00c04188040eaace455c5c6dbb3bb200a2c0f29c3b4ef5128bf0973a
|
|
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
|
|
CCoinsCacheEntry and remove invalid tests
50cce20013c97e1257cb9f4c9319701a09b0a196 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914775756dcac2d9fa7fe4e4d4e70ba0c test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1eef95c0b626573a9a2e217f4a541a880 test: Validate error messages on fail (Lőrinc)
d5f8d607ab1f8fd9fc17aba4105867b450117be2 test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490a5005d227da75dc8f2d1ab73c6e9d2 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c3818b4138602c127d6a16018aae75687 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cfc79253ffae1527106baa428dd62f39 coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022e6ce4eb3ce526800a6ada3b4a2996d coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc910efa3ed7a6d32e687107248bb5be coins, refactor: Split up AddFlags to remove invalid states (Lőrinc)
Pull request description:
Similarly to https://github.com/bitcoin/bitcoin/pull/30849, this cleanup is intended to de-risk https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1739909068 by simplifying the coin cache public interface.
`CCoinsCacheEntry` provided general access to its internal flags state, even though, in reality, it could only be `clean`, `fresh`, `dirty`, or `fresh|dirty` (in the follow-up, we will remove `fresh` without `dirty`).
Once it was marked as `dirty`, we couldn’t set the state back to clean with `AddFlags(0)`—tests explicitly checked against that.
This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of `CCoinsCacheEntry`, as many tests were simply validating invalid combinations in this way.
The last few commits contain significant test refactorings to make `coins_tests` easier to change in follow-ups.
ACKs for top commit:
andrewtoth:
Code Review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196
laanwj:
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196
ryanofsky:
Code review ACK 50cce20013c97e1257cb9f4c9319701a09b0a196. Looks good! Thanks for the followups.
Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
|
|
untranslated strings
This change manually removes two strprintf(Untranslated...) calls. All
remaining calls are removed in the next scripted-diff commit.
Removing these calls makes code more consistent and makes it easier to
implement compile-time checking enforcing that format strings contain valid
specifiers, by avoiding the need for the Untranslated() function to be involved
in formatting.
Additionally, using + and += instead of strprintf here makes code a little
shorter, and more type-safe because + unlike strprintf only works on strings of
the same type, making it less likely english strings and bilingual strings will
be unintentionally combined.
|
|
This could produce an english error message containing non-english string
fragments if PopulateAndValidateSnapshot started returning any translated
strings in the future. This change is also needed to make the next
scripted-diff commit work.
|
|
Instead just concatenate already formatted strings. This allows untranslated
format strings to be checked at compile time now, and translated format strings
to be checked at compile time in #31061.
|
|
95a0104f2e9869799db84add108ae8c57b56d360 test: Add tests for directories in place of config files (Hodlinator)
e85abe92c7cc5380489c028479f0d42f91827efd args: Catch directories in place of config files (Hodlinator)
e4b6b1822ce004365be11c54c8f5f02f95303fb0 test: Add tests for -noconf (Hodlinator)
483f0dacc413f4b1ba1b74c2429c4367b87e7f11 args: Properly support -noconf (Hodlinator)
312ec64cc0619f58c6e8abc5855fd2fa0e920c3f test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658bc2b9d835b240edb9c1dac308859687c3 test: -norpccookiefile (Hodlinator)
39cbd4f37c3d3a32cd993cbc78052d53f700989b args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88452bce4132e4583727e610d52dcf9ad9e logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76907ca0012b1a4556d9a982dfffb5abaf6 test: Harden testing of cookie file existence (Hodlinator)
75bacabb55f3d54ad9b2c660cafc82c167e4f644 test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f00f5bfbb5a622d0bd20bfeed9f8b10928 args: Support -nopid (Hodlinator)
12f8d848fd91b11d5cffe21dfc3ba124102ee236 args: Disallow -nodatadir (Hodlinator)
6ff9662760099c405cf13153dd1de22900045f5e scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054edc814b2c4661b96a3dce91da9be68fa4 doc args: Document narrow scope of -color (Hodlinator)
Pull request description:
- Document `-color` as only applying to `-getinfo`, to be less confusing for bitcoin-cli users.
- No longer print version information when getting passed `-noversion`.
- Disallow `-nodatadir` as we cannot run without one. It was previously interpreted as a mix of unset and as a relative path of "0".
- Support `-norpccookiefile`
- Support `-nopid`
- Properly support `-noconf` (instead of working by accident). Also detect when directories are specified instead of files.
Prompted by investigation in https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013.
ACKs for top commit:
l0rinc:
utACK 95a0104f2e9869799db84add108ae8c57b56d360
achow101:
ACK 95a0104f2e9869799db84add108ae8c57b56d360
ryanofsky:
Code review ACK 95a0104f2e9869799db84add108ae8c57b56d360. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
|
|
bilingual format strings
fa3e074304780549b1e7972217930e34fa55f59a refactor: Tidy fixups (MarcoFalke)
fa72646f2b197810a324cb0544d9a1fac37d3f9c move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0aac3b5ec26d3c7fc98240f879f9906 refactor: Pick translated string after format (MarcoFalke)
Pull request description:
The changes are required for https://github.com/bitcoin/bitcoin/pull/31061, however they also make sense on their own. For example, they are fixing up an `inline namespace`, which lead to compile errors otherwise (can be tested by observing the compile error after reverting the changes to `src/util/strencodings.h`). Also, a unit test comment is fixed.
ACKs for top commit:
ryanofsky:
Code review ACK fa3e074304780549b1e7972217930e34fa55f59a. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304780549b1e7972217930e34fa55f59a
hodlinator:
cr-ACK fa3e074304780549b1e7972217930e34fa55f59a
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
|
|
faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba Remove wallet::ParseISO8601DateTime, use ParseISO8601DateTime instead (MarcoFalke)
2222aecd5f8059785e655da7b7e3fcc59204245c util: Implement ParseISO8601DateTime based on C++20 (MarcoFalke)
Pull request description:
`boost::posix_time` in `ParseISO8601DateTime` has many issues:
* It parses random strings that are clearly invalid and returns a time value for them, see [1] below.
* None of the separators `-`, or `:`, or `T`, or `Z` are validated.
* It may crash when running under a hardened C++ library, see https://github.com/bitcoin/bitcoin/issues/28917.
* It has been unmaintained for years, so reporting or fixing any issues will most likely be useless.
* It pulls in a third-party dependency, when the functionality is already included in vanilla C++20.
Fix all issues by replacing it with a simple helper function written in C++20.
Fixes https://github.com/bitcoin/bitcoin/issues/28917.
[1] The following patch passes on current master:
```diff
diff --git a/src/wallet/test/rpc_util_tests.cpp b/src/wallet/test/rpc_util_tests.cpp
index 32f6f5ab46..c1c94c7116 100644
--- a/src/wallet/test/rpc_util_tests.cpp
+++ b/src/wallet/test/rpc_util_tests.cpp
@@ -12,6 +12,14 @@ BOOST_AUTO_TEST_SUITE(wallet_util_tests)
BOOST_AUTO_TEST_CASE(util_ParseISO8601DateTime)
{
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("964296"), 242118028800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("244622"), 15023836800);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("+INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7000802 01"), 158734166400);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("7469-2 +INfINITy"), 9223372036854);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("maXimum-datE-time"), 253402300799);
+ BOOST_CHECK_EQUAL(ParseISO8601DateTime("577737 114maXimum-datE-time"), 253402300799);
+
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1970-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("1960-01-01T00:00:00Z"), 0);
BOOST_CHECK_EQUAL(ParseISO8601DateTime("2000-01-01T00:00:01Z"), 946684801);
```
ACKs for top commit:
hebasto:
ACK faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba, I have reviewed the code and it looks OK.
dergoegge:
utACK faf70cc9941ce2b0ce4fd48ecfdbe28194adb8ba
Tree-SHA512: 9dd745a356d04acf6200e13a6af52c51a9e2a0eeccea110093ce5da147b3c669c0eda918e46db0164c081a78c8feae3fe557a4759bea18449a8ff2d090095931
|
|
This removes the temporary overload added in the previous commit.
Also drop unneeded custom coinbase output scripts.
|
|
Providing a script for the coinbase transaction is only done in test code
and for 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.
A coinbase script can still be passed via BlockCreateOptions instead.
A temporary overload is added so that the test can be modified in the
next commit.
|
|
|
|
ChainstateManager constructor
8f85d36d68ab33ba237407a2ed16667eb149d61f refactor: Clamp worker threads in ChainstateManager constructor (TheCharlatan)
Pull request description:
This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.
This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6, used to make applying the mempool options consistent.
---
This is part of the libbitcoinkernel project https://github.com/bitcoin/bitcoin/issues/27587
ACKs for top commit:
maflcko:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f 🛳
achow101:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
furszy:
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
stickies-v:
ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f
Tree-SHA512: 32d7cc177d6726ee9df62ac9eb43e49ba676f35bfcff47834bd97a1e33f2a9ea7be65d0a8a37be149de04e58c9c500ecef730e498f4e3909042324d3136160e9
|
|
32fc59796f74a2941772b5ec2755b1319132cd9c rpc: Allow single transaction through submitpackage (glozow)
Pull request description:
There's no particular reason to restrict single transaction submissions with submitpackage. This change relaxes the RPC checks as enables the `AcceptPackage` flow to accept packages of a single transaction.
Resolves #31085
ACKs for top commit:
naumenkogs:
ACK https://github.com/bitcoin/bitcoin/commit/32fc59796f74a2941772b5ec2755b1319132cd9c
achow101:
ACK 32fc59796f74a2941772b5ec2755b1319132cd9c
glozow:
ACK 32fc59796f74a2941772b5ec2755b1319132cd9c
Tree-SHA512: ffed353bfdca610ffcfd53b40b76da05ffc26df6bac4b0421492e067bede930380e03399d2e2d1d17f0e88fb91cd8eb376e3aabebbabcc724590bf068d09807c
|
|
73db95c65c1d372822166045ca8b9f173d5fd883 kernel: Make bitcoin-chainstate's block validation mirror submitblock's (TheCharlatan)
bb53ce9bdae2f02d7bd95cf5d8ca4ccf5136466a tests: Add functional test for submitting a previously pruned block (Greg Sanders)
1f7fc738255205a64374686aca9a4c53089360f1 rpc: Remove submitblock duplicate pre-check (TheCharlatan)
e62a8abd7df21795dcd173773f689b6d4c8feab6 rpc: Remove submitblock invalid-duplicate precheck (TheCharlatan)
36dbebafb9b54764005e6fffa7ad28d4cadfe5e4 rpc: Remove submitblock coinbase pre-check (TheCharlatan)
Pull request description:
With the introduction of a mining ipc interface and the potential future introduction of a kernel library API it becomes increasingly important to offer common behaviour between them. An example of this is ProcessNewBlock, which is used by ipc, rpc, net_processing and (potentially) the kernel library. Having divergent behaviour on suggested pre-checks and checks for these functions is confusing to both developers and users and is a maintenance burden.
The rpc interface for ProcessNewBlock (submitblock) currently pre-checks if the block has a coinbase transaction and whether it has been processed before. While the current example binary for how to use the kernel library, bitcoin-chainstate, imitates these checks, the other interfaces do not.
The coinbase check is repeated again early during ProcessNewBlock. Pre-checking it may also shadow more fundamental problems with a block. In most cases the block header is checked first, before validating the transactions. Checking the coinbase first therefore masks potential issues with the header. Fix this by removing the pre-check.
Similary the duplicate checks are repeated early in the contextual checks of ProcessNewBlock. If duplicate blocks are detected much of their validation is skipped. Depending on the constitution of the block, validating the merkle root of the block is part of the more intensive workload when validating a block. This could be an argument for moving the pre-checks into block processing. In net_processing this would have a smaller effect however, since the block mutation check, which also validates the merkle root, is done before.
Testing spamming a node with valid, but duplicate unrequested blocks seems to exhaust a CPU thread, but does not seem to significantly impact keeping up with the tip. The benefits of adding these checks to net_processing are questionable, especially since there are other ways to trigger the more CPU-intensive checks without submitting a duplicate block. Since these DOS concerns apply even less to the RPC interface, which does not have banning mechanics built in, remove them too.
Finally, also remove the pre-checks from `bitcoin-chainstate.cpp`.
---
This PR is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
Sjors:
re-utACK 73db95c65c1d372822166045ca8b9f173d5fd883
achow101:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
instagibbs:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
mzumsande:
ACK 73db95c65c1d372822166045ca8b9f173d5fd883
Tree-SHA512: 2d02e851cf402ecf6a1968c058df3576aac407e200cbf922a1a6391b7f97b4f42c6d9f6b0a78b9d1af0a6d40bdd529a7b11a1e6d88885bd7b8b090f6d1411861
|
