Commit graph

43479 commits

Author SHA1 Message Date
fanquake
f6496a8388
guix: disable gcov in base-linux-gcc
In a `x86_64-linux-gnu` build, this drops:
```bash
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-dump
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-tool
x86_64-linux-gnu/lib/gcc/x86_64-linux-gnu/12.4.0: libgcov.a
```

For mingw-w64-gcc, `--disable-gcov` is currently passed for this
target in Guix, due to issues with mingw-w64, see
8bed031e58/gnu/packages/gcc.scm (L99-L102).
However we'll add it in any case, in case it's re-enabled in future,
when the underlying issues are fixed.
2024-12-09 15:28:25 +00:00
merge-script
35000e34cf
Merge bitcoin/bitcoin#31433: test: #31212 follow up (spelling, refactor)
Some checks are pending
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
41d934c72d chore: Typo Overriden -> Overridden (Hodlinator)
c9fb38a590 refactor test: Cleaner combine_logs.py logic (Hodlinator)

Pull request description:

  - Fixes typo caught by spelling linter (https://github.com/bitcoin/bitcoin/runs/33979284676).
  - Minor but nice refactoring of *combine_logs.py* change that was suggested late: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1869307947

ACKs for top commit:
  l0rinc:
    ACK 41d934c72d
  maflcko:
    lgtm ACK 41d934c72d
  theStack:
    ACK 41d934c72d
  BrandonOdiwuor:
    Code Review ACK 41d934c72d
  tdb3:
    ACK 41d934c72d

Tree-SHA512: cf8ecc070d0b01df9c4e57a75820e17d4535591e85bf9d271c7b8f60875f7e04b9978c56e9b88c10e89e69ff755c35b23ed291949c32c875a91c3317105a3c79
2024-12-08 16:33:31 +00:00
merge-script
18d0cfb194
Merge bitcoin/bitcoin#31306: ci: Update Clang in "tidy" job
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
2024-12-08 16:30:38 +00:00
Hodlinator
c93bf0e6e2
test: Add missing %c character test
Proves tinyformat doesn't trigger an exception for \0 characters.

Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
2024-12-06 21:56:17 +01:00
Hodlinator
76cca4aa6f
test: Document non-parity between tinyformat and ConstevalFormatstring
- 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>
2024-12-06 21:56:16 +01:00
Hodlinator
533013cba2
test: Prove+document ConstevalFormatString/tinyformat parity
Co-Authored-By: Lőrinc <pap.lorinc@gmail.com>
Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
2024-12-06 21:45:21 +01:00
Hodlinator
b81a465995
refactor test: Profit from using namespace + using detail function
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.
2024-12-06 21:45:18 +01:00
furszy
cdd207c0e4
test: add coverage for migrating standalone imported keys 2024-12-06 14:13:09 -05:00
furszy
297a876c98
test: add coverage for migrating watch-only script 2024-12-06 14:13:09 -05:00
furszy
932cd1e92b
wallet: fix crash during watch-only wallet migration
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
2024-12-06 11:26:28 -05:00
Hodlinator
41d934c72d
chore: Typo Overriden -> Overridden 2024-12-06 15:34:37 +01:00
Hodlinator
c9fb38a590
refactor test: Cleaner combine_logs.py logic 2024-12-06 15:34:37 +01:00
merge-script
22723c809a
Merge bitcoin/bitcoin#31072: refactor: Clean up messy strformat and bilingual_str usages
Some checks failed
CI / test each commit (push) Has been cancelled
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Has been cancelled
CI / macOS 14 native, arm64, fuzz (push) Has been cancelled
CI / Win64 native, VS 2022 (push) Has been cancelled
CI / Win64 native fuzz, VS 2022 (push) Has been cancelled
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Has been cancelled
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
2024-12-06 11:38:50 +00:00
glozow
b1f0f3c288
Merge bitcoin/bitcoin#31406: test: fix test_invalid_tx_in_compactblock in p2p_compactblocks
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
2024-12-06 06:29:52 -05:00
merge-script
1a35447595
Merge bitcoin/bitcoin#31417: test: Avoid F541 (f-string without any placeholders)
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
2024-12-06 10:26:58 +00:00
merge-script
eb2ebe6f30
Merge bitcoin/bitcoin#31231: cmake: Fix IF_CHECK_PASSED option handling
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
2024-12-06 10:15:56 +00:00
merge-script
5b283fa147
Merge bitcoin/bitcoin#31431: util: use explicit cast in MultiIntBitSet::Fill()
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
2024-12-06 10:00:39 +00:00
Sjors Provoost
37946c0aaf
Set notifications m_tip_block in LoadChainTip()
Ensure KernelNotifications m_tip_block is set even if no new block arrives.

Additionally, have node init always wait for this to happen.
2024-12-06 14:24:21 +07:00
MarcoFalke
fa6e599cf9
test: Call generate through test framework only 2024-12-06 08:20:52 +01:00
Ryan Ofsky
2eccb8bc5e
Merge bitcoin/bitcoin#31248: test: Rework wallet_migration.py to use previous releases
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
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
2024-12-05 15:47:43 -05:00
brunoerg
7239ddb7ce test: make sure node has all transactions 2024-12-05 16:12:38 -03:00
merge-script
6d973f86f7
Merge bitcoin/bitcoin#31408: test: Avoid logging error when logging error
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
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
2024-12-05 17:17:39 +00:00
merge-script
6a1e613e85
Merge bitcoin/bitcoin#31427: lint: bump MLC to v0.19.0
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
2024-12-05 16:34:06 +00:00
Vasil Dimov
edb41e4814
util: use explicit cast in MultiIntBitSet::Fill()
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=
```
2024-12-05 16:55:36 +01:00
Hennadii Stepanov
31e59d94c6
iwyu: Drop backported mapping
See: https://github.com/include-what-you-use/include-what-you-use/pull/1560
2024-12-05 14:37:55 +00:00
Hennadii Stepanov
fe9bc5abef
ci: Update Clang in "tidy" job
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>
2024-12-05 14:37:47 +00:00
glozow
083770adbe
Merge bitcoin/bitcoin#31414: test: orphan parent is re-requested from 2nd peer
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
2024-12-05 07:48:58 -05:00
willcl-ark
f6afca46a1
lint: use clearer wording on error message 2024-12-05 11:26:27 +00:00
willcl-ark
811a65d3c6
lint: bump MLC to v0.19.0
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.
2024-12-05 11:24:48 +00:00
MarcoFalke
fae76393bd
test: Avoid F541 (f-string without any placeholders) 2024-12-05 08:39:09 +01:00
Ava Chow
e8cc790fe2
Merge bitcoin/bitcoin#30445: test: addrman: tried 3 times and never a success so isTerrible=true
Some checks are pending
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
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
2024-12-04 15:34:59 -05:00
Ryan Ofsky
0184d33b3d scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf)
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-
2024-12-04 15:09:05 -04:00
Ryan Ofsky
17372d788e
Merge bitcoin/bitcoin#30906: refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests
Some checks are pending
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
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
2024-12-04 14:09:05 -05:00
Ryan Ofsky
006e4d1d59 refactor: Use + instead of strformat to concatenate translated & 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.
2024-12-04 15:09:05 -04:00
Ryan Ofsky
831d2bfcf9 refactor: Don't embed translated string in untranslated string.
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.
2024-12-04 15:09:05 -04:00
Ryan Ofsky
058021969b refactor: Avoid concatenation of format strings
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.
2024-12-04 15:09:05 -04:00
Ava Chow
11f68cc810
Merge bitcoin/bitcoin#31212: util: Improve documentation and negation of args
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
2024-12-04 13:20:46 -05:00
merge-script
893ccea7e4
Merge bitcoin/bitcoin#31419: test: fix MIN macro redefinition
00c1dbd26d test: fix MIN macro-redefinition (0xb10c)

Pull request description:

  Renames the `MIN` macro to `_TRACEPOINT_TEST_MIN`.

  From #31418:

  ```
  stderr:
  /virtual/main.c:70:9: warning: 'MIN' macro redefined [-Wmacro-redefined]
     70 | #define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; })
        |         ^
  include/linux/minmax.h:329:9: note: previous definition is here
    329 | #define MIN(a,b) __cmp(min,a,b)
        |         ^
  1 warning generated.
  ```

  fixes: https://github.com/bitcoin/bitcoin/issues/31418

ACKs for top commit:
  maflcko:
    review ACK 00c1dbd26d

Tree-SHA512: 1d91ed8b3c0b0410d42a11004286359546c17613c3ff03dd51c26896ee050e9280fff69fa2eaa6e6085f9b611663bcacedec80997a6c5e37874a79ba86bfa507
2024-12-04 17:13:00 +00:00
Ryan Ofsky
39950e148d
Merge bitcoin/bitcoin#31295: refactor: Prepare compile-time check of bilingual format strings
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
2024-12-04 11:15:58 -05:00
0xb10c
00c1dbd26d
test: fix MIN macro-redefinition
Renames the `MIN` macro to `_TRACEPOINT_TEST_MIN`.

From #31418:

```
stderr:
/virtual/main.c:70:9: warning: 'MIN' macro redefined [-Wmacro-redefined]
   70 | #define MIN(a,b) ({ __typeof__ (a) _a = (a); __typeof__ (b) _b = (b); _a < _b ? _a : _b; })
      |         ^
include/linux/minmax.h:329:9: note: previous definition is here
  329 | #define MIN(a,b) __cmp(min,a,b)
      |         ^
1 warning generated.
```

fixes: https://github.com/bitcoin/bitcoin/issues/31418
2024-12-04 15:51:17 +01:00
merge-script
ae69fc37e4
Merge bitcoin/bitcoin#31391: util: Drop boost posix_time in ParseISO8601DateTime
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
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
2024-12-04 11:21:43 +00:00
Sjors Provoost
52fd1511a7
test: drop scriptPubKeyIn arg from CreateNewBlock
This removes the temporary overload added in the previous commit.

Also drop unneeded custom coinbase output scripts.
2024-12-04 12:46:33 +07:00
Sjors Provoost
ff41b9e296
Drop script_pub_key arg from createNewBlock
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.
2024-12-04 12:44:57 +07:00
Sjors Provoost
7ab733ede4
rpc: rename coinbase_script to coinbase_output_script 2024-12-04 12:44:57 +07:00
Ava Chow
ff873a20a7
Merge bitcoin/bitcoin#31313: refactor: Clamp worker threads in ChainstateManager constructor
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
8f85d36d68 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 09ef322acc, 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 8f85d36d68 🛳
  achow101:
    ACK 8f85d36d68
  furszy:
    Code ACK 8f85d36d68
  stickies-v:
    ACK 8f85d36d68

Tree-SHA512: 32d7cc177d6726ee9df62ac9eb43e49ba676f35bfcff47834bd97a1e33f2a9ea7be65d0a8a37be149de04e58c9c500ecef730e498f4e3909042324d3136160e9
2024-12-03 18:02:37 -05:00
Ava Chow
c9a7418a8d
Merge bitcoin/bitcoin#31096: Package validation: accept packages of size 1
32fc59796f 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 32fc59796f
  achow101:
    ACK 32fc59796f
  glozow:
    ACK 32fc59796f

Tree-SHA512: ffed353bfdca610ffcfd53b40b76da05ffc26df6bac4b0421492e067bede930380e03399d2e2d1d17f0e88fb91cd8eb376e3aabebbabcc724590bf068d09807c
2024-12-03 17:46:23 -05:00
Ava Chow
6f24662eb9
Merge bitcoin/bitcoin#31175: rpc: Remove submitblock pre-checks
73db95c65c kernel: Make bitcoin-chainstate's block validation mirror submitblock's (TheCharlatan)
bb53ce9bda tests: Add functional test for submitting a previously pruned block (Greg Sanders)
1f7fc73825 rpc: Remove submitblock duplicate pre-check (TheCharlatan)
e62a8abd7d rpc: Remove submitblock invalid-duplicate precheck (TheCharlatan)
36dbebafb9 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 73db95c65c
  achow101:
    ACK 73db95c65c
  instagibbs:
    ACK 73db95c65c
  mzumsande:
    ACK 73db95c65c

Tree-SHA512: 2d02e851cf402ecf6a1968c058df3576aac407e200cbf922a1a6391b7f97b4f42c6d9f6b0a78b9d1af0a6d40bdd529a7b11a1e6d88885bd7b8b090f6d1411861
2024-12-03 17:38:41 -05:00
Ava Chow
3867d2421a
Merge bitcoin/bitcoin#31112: Improve parallel script validation error debug logging
492e1f0994 [validation] merge all ConnectBlock debug logging code paths (Pieter Wuille)
b49df703f0 [validation] include all logged information in BlockValidationState (Pieter Wuille)
7b267c034f [validation] Add detailed txin/txout information for script error messages (Pieter Wuille)
146a3d5426 [validation] Make script error messages uniform for parallel/single validation (Pieter Wuille)
1ac1c33f3f [checkqueue] support user-defined return type through std::optional (Pieter Wuille)

Pull request description:

  ~~Builds on top of #31097~~ (now merged). Fixes #30960.

  So far, detailed information about script validation failures is only reported when running with `-par=1`, due to a lack of ability to transfer information from the script validation threads to the validation thread. Fix this by extending the `CCheckQueue` functionality to pass more results through than just success/failure, and use this to report the exact Script error, as well as the transaction input in which it occurred.

ACKs for top commit:
  achow101:
    ACK 492e1f0994
  furszy:
    Code review ACK 492e1f0
  maflcko:
    re-ACK 492e1f0994 🍈
  dergoegge:
    ACK 492e1f0994
  instagibbs:
    ACK 492e1f0994
  mzumsande:
    Code Review ACK 492e1f0994

Tree-SHA512: 234f2e7dfd03bdcd2a56200875fe370962f211ea7ed334038a6a9279a758030bf94bb6246f60d06dd0473dac4b9dbf050d9a32ecaa4176f7727eff63572bf4fd
2024-12-03 15:22:38 -05:00
Ryan Ofsky
8e02b48059
Merge bitcoin/bitcoin#31284: ci: Skip broken Wine64 tests by default
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Win64 native, VS 2022 (push) Waiting to run
CI / Win64 native fuzz, VS 2022 (push) Waiting to run
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
fa5e706459 ci: Skip broken Wine64 tests by default (MarcoFalke)

Pull request description:

  I don't think the unit tests run in Wine after the Windows cross-compilation have ever shown a true positive since the MSVC task was added. However, they are a source of frequent false-positives.

  Thus, disable them by default for now. Anyone can still enable them by setting `RUN_UNIT_TESTS=true`.

  A follow-up could run them on real Windows, see https://github.com/bitcoin/bitcoin/pull/31176.

  Conceptually there are many other nightly tasks, which rarely find issues and are not run by default, like the valgrind or s390x tasks. So putting the Wine unit tests in the same bucket should be fine.

ACKs for top commit:
  hebasto:
    ACK fa5e706459, to avoid false-positives.
  willcl-ark:
    ACK fa5e706459

Tree-SHA512: 6bd54470e4d5ce18923c5d724aba0dbf475d053d7097d3f87e822a455cc537b6ce5f0dfcc8ccd0719c12c5d0c8fc7355a6c84185a6c9b5d484d98aee763d0c49
2024-12-03 12:21:31 -05:00
Greg Sanders
0f84cdd266 func: test orphan parent is re-requested from 2nd peer 2024-12-03 11:12:49 -05:00