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
The util::ResultPtr class is a wrapper for util::Result providing syntax sugar
to make it less awkward to use with returned pointers. Instead of needing to be
deferenced twice with **result or (*result)->member, it only needs to be
dereferenced once with *result or result->member. Also when it is used in a
boolean context, instead of evaluating to true when there is no error and the
pointer is null, it evaluates to false so it is straightforward to determine
whether the result points to something.
This PR is only partial a solution to #26004 because while it makes it easier
to check for null pointer values, it does not make it impossible to return a
null pointer value inside a successful result. It would be possible to enforce
that successful results always contain non-null pointers by using a not_null
type (https://github.com/bitcoin/bitcoin/issues/24423) in combination with
ResultPtr, though.
Add util::Result support for returning warning messages and multiple errors,
not just a single error string. This provides a way for functions to report
errors and warnings in a standard way, and simplifies interfaces.
The functionality is unit tested here, and put to use in followup PR
https://github.com/bitcoin/bitcoin/pull/25722
Add util::Result Update method to make it possible to change the value of an
existing Result object. This will be useful for functions that can return
multiple error and warning messages, and want to be able to change the result
value while keeping existing messages.
Add util::Result support for returning more error information. For better error
handling, adds the ability to return a value on failure, not just a value on
success. This is a key missing feature that made the result class not useful
for functions like LoadChainstate() which produce different errors that need to
be handled differently.
This change also makes some more minor improvements:
- Smaller type size. util::Result<int> is 16 bytes, and util::Result<void> is 8
bytes. Previously util::Result<int> and util::Result<void> were 72 bytes.
- More documentation and tests.
7239ddb7ce test: make sure node has all transactions (brunoerg)
ee1b9bef00 test: replace `is not` to `!=` when comparing block hash (brunoerg)
Pull request description:
`test_invalid_tx_in_compactblock` tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions.
In this test, after sending the block with invalid transactions, this test checks two things: the tip in the receiver node did not advance and the sender did not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have all transactions to reconstruct the valid and would request them back. This PR fixes it by sending all the transactions.
Also, comparing block hash (int) using `is not` can lead to subtle bugs, this PR fixes it by replacing it to `!=`.
--------------
Can be tested by applying:
```diff
diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 274ef9532c..419153a32f 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -723,11 +723,8 @@ class CompactBlocksTest(BitcoinTestFramework):
utxo = self.utxos[0]
block = self.build_block_with_transactions(node, utxo, 5)
- del block.vtx[3]
block.hashMerkleRoot = block.calc_merkle_root()
# Drop the coinbase witness but include the witness commitment.
- add_witness_commitment(block)
- block.vtx[0].wit.vtxinwit = []
block.solve()
# Make sure node has the transactions to reconstruct the block
```
ACKs for top commit:
instagibbs:
ACK 7239ddb7ce
glozow:
ACK 7239ddb7ce
lucasbalieiro:
Tested ACK for commit [7239ddb](7239ddb7ce)
Tree-SHA512: 6d04fb7c50b5e635c83ede75c12130cbd8e1b229887a86a2e1bfe747e4208731faecc7265cae063c1ace187b20c5f37080d5116760766fa2948f38971e5f6fbf
fae76393bd test: Avoid F541 (f-string without any placeholders) (MarcoFalke)
Pull request description:
An extra `f` string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.
Try to avoid the confusion and mistakes by applying the `F541` linter rule.
ACKs for top commit:
lucasbalieiro:
**Tested ACK** [fae7639](fae76393bd)
danielabrozzoni:
ACK fae76393bd
tdb3:
Code review ACK fae76393bd
Tree-SHA512: 4992a74fcf0c19b32e4d95f7333e087b4269b5c5259c556789fb86721617db81c7a4fe210ae136c92824976f07f71ad0f374655e7008b1967c02c73324862d9a
97a18c8545 cmake: Fix `IF_CHECK_PASSED` option handling (Hennadii Stepanov)
Pull request description:
`IF_CHECK_PASSED` is a multi-value keyword, resulting in a list value. Convert it to a string before applying any `string()` command.
Split from https://github.com/bitcoin/bitcoin/pull/30861.
No current CMake code is affected by this bug.
ACKs for top commit:
theuni:
utACK 97a18c8545
Tree-SHA512: d2556ca38c35a8992175e9f948c2028a789e71c2b2d5fdf365b31710c8ed3d5edf5d0363853c5d750d29abb58cfda3c78cdc2971a627e5b4c61aca4ec2a33356
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
cccca8a77f test: Avoid logging error when logging error (MarcoFalke)
Pull request description:
Currently a logging error in the form of `--- Logging error ---` happens when an error is logged in the `_on_data` helper.
Fix it by properly logging the error.
Also, treat pylint errors as errors, to avoid this problem in the future.
Can be tested by running `p2p_addrv2_relay.py` with the following example diff:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 523e1bd068..0f1eb29d13 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -137,7 +137,7 @@ MESSAGEMAP = {
b"notfound": msg_notfound,
b"ping": msg_ping,
b"pong": msg_pong,
- b"sendaddrv2": msg_sendaddrv2,
+ #b"sendaddrv2": msg_sendaddrv2,
b"sendcmpct": msg_sendcmpct,
b"sendheaders": msg_sendheaders,
b"sendtxrcncl": msg_sendtxrcncl,
ACKs for top commit:
fanquake:
ACK cccca8a77f
Tree-SHA512: dd19f3feed0093246cb205903529fb9ebd5ad9a6c9330cfc5987c0154253c9dcec8d0e25ff99e4ac806a464ff58c3787a205378b8dfb7a1a521da25eac429136
f6afca46a1 lint: use clearer wording on error message (willcl-ark)
811a65d3c6 lint: bump MLC to v0.19.0 (willcl-ark)
Pull request description:
Fixes: #31044
This MLC update includes a change which will ignore files being ignored by git, and help avoid false-positives when linting in this repo.
Top commit has no ACKs.
Tree-SHA512: d3edd0125f719c7a4456f7089e298dc851352a082b8119bbd8d642de518bb193827af9994ba416dd18a6a6f1359ee96122d95a31232da1623c679db39b370370
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=
```
0f84cdd266 func: test orphan parent is re-requested from 2nd peer (Greg Sanders)
Pull request description:
Small test which I couldn't find coverage for.
ACKs for top commit:
glozow:
lgtm ACK 0f84cdd266
tdb3:
code review ACK 0f84cdd266
theStack:
ACK 0f84cdd266
marcofleon:
tACK 0f84cdd266. Removing `node.bumpmocktime(GETDATA_TX_INTERVAL)` results in failure.
Tree-SHA512: fe8cb9d56aabc8f2ef1f49b6cd4e87e28a51ada8070c698f60c5fd945a28d849f0c5793f2e3e29f013e610168b860e0bf1c0aa010eec5b339688269d2b9e69af
Fixes: #31044
This MLC update includes a change which will ignore files being ignored
by git, and help avoid false-positives when linting in this repo.
1807df3d9f 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 1807df3d9f
naumenkogs:
ACK 1807df3d9f
achow101:
ACK 1807df3d9f
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-
50cce20013 test, refactor: Compact ccoins_access and ccoins_spend (Lőrinc)
0a159f0914 test, refactor: Remove remaining unbounded flags from coins_tests (Lőrinc)
c0b4b2c1ee test: Validate error messages on fail (Lőrinc)
d5f8d607ab test: Group values and states in tests into CoinEntry wrappers (Lőrinc)
ca74aa7490 test, refactor: Migrate GetCoinsMapEntry to return MaybeCoin (Lőrinc)
15aaa81c38 coins, refactor: Remove direct GetFlags access (Lőrinc)
6b733699cf coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers (Lőrinc)
fc8c282022 coins, refactor: Make AddFlags, SetDirty, SetFresh static (Lőrinc)
cd0498eabc 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 50cce20013
laanwj:
Code review ACK 50cce20013
ryanofsky:
Code review ACK 50cce20013. Looks good! Thanks for the followups.
Tree-SHA512: c0d65f1c7680b4bb9cd368422b218f2473c2ec75a32c7350a6e11e8a1601c81d3c0ae651b9f1dae08400fb4e5d43431d9e4ccca305a718183f9a936fe47c1a6c
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.
95a0104f2e test: Add tests for directories in place of config files (Hodlinator)
e85abe92c7 args: Catch directories in place of config files (Hodlinator)
e4b6b1822c test: Add tests for -noconf (Hodlinator)
483f0dacc4 args: Properly support -noconf (Hodlinator)
312ec64cc0 test refactor: feature_config_args.py - Stop nodes at the end of tests, not at the beginning (Hodlinator)
7402658bc2 test: -norpccookiefile (Hodlinator)
39cbd4f37c args: Support -norpccookiefile for bitcoind and bitcoin-cli (Hodlinator)
e82ad88452 logs: Use correct path and more appropriate macros in cookie-related code (Hodlinator)
6e28c76907 test: Harden testing of cookie file existence (Hodlinator)
75bacabb55 test: combine_logs.py - Output debug.log paths on error (Hodlinator)
bffd92f00f args: Support -nopid (Hodlinator)
12f8d848fd args: Disallow -nodatadir (Hodlinator)
6ff9662760 scripted-diff: Avoid printing version information for -noversion (Hodlinator)
e8a2054edc 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 95a0104f2e
achow101:
ACK 95a0104f2e
ryanofsky:
Code review ACK 95a0104f2e. Looks good! Thanks for all your work on this breaking the changes down and making them simple.
Tree-SHA512: 5174251e6b9196a9c6d135eddcb94130295c551bcfccc78e633d9e118ff91523b1be0d72828fb49603ceae312e6e1f8ee2651c6a2b9e0f195603a73a9a622785
fa3e074304 refactor: Tidy fixups (MarcoFalke)
fa72646f2b move-only: Detail_CheckNumFormatSpecifiers and G_TRANSLATION_FUN (MarcoFalke)
faff8403f0 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 fa3e074304. Nice changes! These should allow related PRs to be simpler.
l0rinc:
ACK fa3e074304
hodlinator:
cr-ACK fa3e074304
Tree-SHA512: 37371181a348610442186b5fbb7a6032d0caf70aae566002ad60be329a3131a2b89f28f6c51e10872079f987986925dc8c0611bde639057bee4f572d2b9ba92a
faf70cc994 Remove wallet::ParseISO8601DateTime, use ParseISO8601DateTime instead (MarcoFalke)
2222aecd5f 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 faf70cc994, I have reviewed the code and it looks OK.
dergoegge:
utACK faf70cc994
Tree-SHA512: 9dd745a356d04acf6200e13a6af52c51a9e2a0eeccea110093ce5da147b3c669c0eda918e46db0164c081a78c8feae3fe557a4759bea18449a8ff2d090095931
