Skip to content

Conversation

@l0rinc
Copy link
Owner

@l0rinc l0rinc commented May 23, 2025

No description provided.

Eunovo and others added 30 commits April 25, 2025 05:54
Non-range desc  are always added to the wallet with the range [0,0]. After the descriptor is added, the wallet will  TopUp the keypool. For non-range descriptors, this process updates the desc range to [0,1].

Any attempts to update this non-range descriptor with a [0,0] range will result in an error because the range checks rejects new ranges not included in the old range.

Since  this is a non-range desc, the range information should be disregarded and AddWalletDescriptor should always succeed regardless of provided range information
This option is now enabled by default.
Back in 2015, in bitcoin#7044, we added configuration option `rpcauth` for
multiple RPC users. At the same time the old settings for single-user
configuration `rpcuser` and `rpcpassword` were "soon" to be deprecated.

The main reason for this deprecation is that while `-rpcpassword` stores
the password in plain text, `-rpcauth` stores a hash, so it doesn't
appear in the configuration in plain text.

As the options are still in active use, actually removing them is
expected to be a hassle to many, and it's not clear that is worth it. As
for the security risk, in many kinds of setups (no wallet,
containerized, single-user-single-application, local-only, etc) it is an
unlikely point of escalation.

In the end, it is good to encourage secure practices, but it is the
responsibility of the user. Log a clear warning but remove the
deprecation notice.

Closes bitcoin#29240.
This gets rid of the special-casing of `strRPCUserColonPass` by hashing
cookies as well as manually provided `-rpcuser`/`-rpcpassword` with a
random salt before storing them.

Also take the opportunity to modernize the surrounding code a bit. There
should be no end-user visible differences in behavior.
-BEGIN VERIFY SCRIPT-
sed -i -e 's|CSemaphoreGrant|SemaphoreGrant|g' -e 's|CSemaphore|Semaphore|g' src/net.h src/net.cpp
-END VERIFY SCRIPT-
Follow-up commits will make a distinction between Semaphore and
BinarySemaphore.
CountingSemaphore and CountingSemaphoreGrant model std::counting_semaphore.

-BEGIN VERIFY SCRIPT-
sed -i -e 's|CSemaphoreGrant|CountingSemaphoreGrant|g' -e 's|CSemaphore|CountingSemaphore|g' src/sync.h
-END VERIFY SCRIPT-
… temporary convenience ones

-BEGIN VERIFY SCRIPT-
sed -i 's|BinarySemaphore|std::binary_semaphore|g' src/wallet/sqlite.h
sed -i 's|SemaphoreGrant|CountingGrant|g' src/net.h src/net.cpp
sed -i 's|Semaphore|std::counting_semaphore<>|g' src/net.h src/net.cpp
sed -i 's|CountingGrant|CountingSemaphoreGrant<>|g' src/net.h src/net.cpp

-END VERIFY SCRIPT-
No need to touch or lint them.
When a block is connected, if the new block had anything relevant to the
wallet, update the best block record on disk. If not, also sync the best
block record to disk every 144 blocks.

Also reuse the new WriteBestBlock method in BackupWallet.
The only reason to call chainStateFlushed during wallet loading is to
ensure that the best block is written. Do these writes explicitly to
prepare for removing chainStateFlushed, while also ensuring that the
wallet's in memory state tracking is written to disk.

Additionally, after rescanning on wallet loading, instead of writing
the locator for the current chain tip, write the locator for the last
block that the rescan had scanned. This ensures that the stored best
block record matches the wallet's current state.

Any blocks dis/connected during the rescan are processed after the
rescan and the last block processed will be updated accordingly.
Migrating a wallet requires having a bestblock record. This is always
the case in normal operation as such a record is always written on
wallet loading if it didn't already exist. However, within the unit
tests and benchmarks, this is not guaranteed. Since migration requires
the record, WalletMigration needs to also add this record before the
benchmark.
chainStateFlushed is no longer needed since the best block is updated
after a block is scanned. Since the chainstate being flushed does not
necessarily coincide with the wallet having processed said block, it
does not entirely make sense for the wallet to be recording that block
as its best block, and this can cause race conditions where some blocks
are not processed. Thus, remove this notification.
StopWallets, which was being called prior to UnloadWallets, performs an
unnecessary database closing step. This causes issues in UnloadWallets
which does additional database cleanups. Since the database closing step
is unnecessary, StopWallets is removed, and UnloadWallets is now called
from WalletLoaderImpl::stop.
Since CWallet::chainStateFlushed is now no-op, this test no longer tests
the concurrent writes scenario. There are no other cases where multiple
DatabaseBatches are open at the same time.
Needed for later validation of sighash types.
hebasto and others added 27 commits May 20, 2025 11:12
`ENABLE_SSE41` is already conditionally defined for the `bitcoin_crypto`
target, so defining it in `bitcoin-build-config.h` is redundant.
`ENABLE_AVX2` is already conditionally defined for the `bitcoin_crypto`
target, so defining it in `bitcoin-build-config.h` is redundant.
`ENABLE_X86_SHANI` is already conditionally defined for the
`bitcoin_crypto` target, so defining it in `bitcoin-build-config.h` is
redundant.
`ENABLE_ARM_SHANI` is already conditionally defined for the
`bitcoin_crypto` target, so defining it in `bitcoin-build-config.h` is
redundant.
The windows code adds an unnecessary extra space to the command line.
This can cause subtle issues, so avoid it.

Github-Pull: arun11299/cpp-subprocess#119
Rebased-From: 777cfa77d1f84bb08b3e445d5f7fc6c87282223b
c7c3bfa doc: add & amend copyright headers (fanquake)
f667000 contrib: remove outdated entries from copyright_header.py (fanquake)
0817f2d doc: update MIT license URL (fanquake)
6854497 contrib: remove GPL-3+ from debian/copyright (fanquake)

Pull request description:

  Add & amends a number of copyright headers.
  Remove some now obselete doc/code.

ACKs for top commit:
  willcl-ark:
    ACK c7c3bfa

Tree-SHA512: 746ca4217e310f7f907fcd5d1e03d481b65045d57048b892a2c993be690898b6681fe6003b4f6a448b551c0ddcb32cad07998de208284a64cc4853fd4d63df52
4b2cd0b test: check that creating a wallet does not log version info (Ava Chow)
39a483c test: Check that the correct versions are logged on wallet load (Ava Chow)
359ecd3 walletdb: Log the wallet version after it has been read from disk (Ava Chow)

Pull request description:

  The wallet's version (in the minversion record) needs to be logged only after we have read it from disk. Otherwise, we always log the lowest version number of 10500 which is incorrect. Furthermore, it doesn't make sense to log the last client version number if the record didn't exist. This is a regression caused by bitcoin#26021.

  The wallet file version logging is moved inside of `LoadMinVersion` so that it is logged after the record is read. It will also log unconditionally if a version is read so that the version number is reported even when there is an error. The last client logging is split into its own log line that will only occur if a last client record is read. The only situation where we expect no version numbers to be logged is when a wallet is being created.

  A test is added in the second commit to check that the version number is correctly logged on loading. This commit can be cherrypicked to master to verify that it fails there. The last commit adds an additional check that creating a new wallet does not log any version info at all.

ACKs for top commit:
  laanwj:
    Code review ACK 4b2cd0b
  janb84:
    ACK bitcoin@4b2cd0b
  furszy:
    ACK 4b2cd0b
  rkrux:
    ACK 4b2cd0b

Tree-SHA512: b30c76f414d87be6c14b42d2d3c8794a91a7e8601501f4c24641d51ff2b5c5144776563baf41ca1c38415844740b760b19a3e5791f78013b39984dfedd3b1de7
3a18075 ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option (Hennadii Stepanov)
719fa9f build: Re-enable external signer support for Windows (Hennadii Stepanov)
6e5fc2b test: Reintroduce Windows support in `system_tests/run_command` test (Hennadii Stepanov)

Pull request description:

  This PR partially reverts:
  - bitcoin#28967
  - bitcoin#29489

  After this PR, we can proceed to actually remove the [unused code](bitcoin#28981 (review)) from `src/util/subprocess.h`.

ACKs for top commit:
  Sjors:
    ACK 3a18075.
  theStack:
    Light ACK 3a18075
  laanwj:
    Code review and lightly tested ACK 3a18075

Tree-SHA512: 00d200685906e716750aae7cffa0794cca451653738ea590f50dfa28e1f3c5762a9be0ae0917aa0cf7436f00fe1e565236bff2853896530a5879466f7f45cb25
faf55fc doc: Remove ParseInt mentions in documentation (MarcoFalke)
3333282 refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face251 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123af Reject + sign when checking -ipcfd (MarcoFalke)
fa47985 Reject + sign in SplitHostPort (MarcoFalke)
fab4c29 net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c455 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa98041 refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)

Pull request description:

  The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:

  * Useless, because the `+` sign was already rejected.
  * Erroneous and inconsistent, when third party parsers reject it. (C.f. bitcoin#32365)
  * Confusing, because the `+` sign is  neither documented, nor can it be assumed to be present.

  Fix all issues by removing the legacy int parsing.

ACKs for top commit:
  stickies-v:
    re-ACK faf55fc
  brunoerg:
    code review ACK faf55fc

Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
…lockchain RPC

135a0f0 doc: Add missing top-level description to pruneblockchain RPC (nervana21)

Pull request description:

  Previously, the `pruneblockchain` RPC help output included only the method signature and arguments, with no top-level description explaining its purpose or constraints.

  This PR adds a top-level description, improving documentation consistency and alerting users to the potential impacts of using the command.

ACKs for top commit:
  maflcko:
    lgtm ACK 135a0f0
  yancyribbens:
    cr ACK bitcoin@135a0f0
  achow101:
    ACK 135a0f0
  janb84:
    re ACK [135a0f0](bitcoin@135a0f0)

Tree-SHA512: e51475238e779555315668b7389ed312a5d2c4ad1c0b251f2314895ac473092fa458b6f931f70385e14047adb7e340e44fe2198643603da9e129f1c874578a28
…::counting_semaphore

6f7052a threading: semaphore: move CountingSemaphoreGrant to its own header (Cory Fields)
fd15469 threading: semaphore: remove temporary convenience types (Cory Fields)
1f89e2a scripted-diff: threading: semaphore: use direct types rather than the temporary convenience ones (Cory Fields)
f21365c threading: replace CountingSemaphore with std::counting_semaphore (Cory Fields)
1acacfb threading: make CountingSemaphore/CountingSemaphoreGrant template types (Cory Fields)
e6ce5f9 scripted-diff: rename CSemaphore and CSemaphoreGrant (Cory Fields)
793166d wallet: change the write semaphore to a BinarySemaphore (Cory Fields)
6790ad2 scripted-diff: rename CSemaphoreGrant and CSemaphore for net (Cory Fields)
d870bc9 threading: add temporary semaphore aliases (Cory Fields)
7b816c4 threading: rename CSemaphore methods to match std::semaphore (Cory Fields)

Pull request description:

  This is relatively simple, but done in a bunch of commits to enable scripted diffs.

  I wanted to add a semaphore in a branch I've been working on, but it was unclear if I should use `std::counting_semaphore` or stick with our old `CSemaphore`. I couldn't decide, so I just decided to remove all doubt and get rid of ours :)

  This replaces our old `CSemaphore` with `std::counting_semaphore` everywhere we used it. `CSemaphoreGrant` is still there as an RAII wrapper, but is now called `CountingSemaphoreGrant` and `BinarySemaphoreGrant` to match. Those have been moved out of `sync.h` to their own file.

ACKs for top commit:
  purpleKarrot:
    ACK 6f7052a
  achow101:
    ACK 6f7052a
  TheCharlatan:
    ACK 6f7052a
  hebasto:
    ACK 6f7052a, I have reviewed the code and it looks OK.

Tree-SHA512: 5975d13aa21739174e3a22c544620ae3f36345f172b51612346d3b7baf0a07c39ef6fd54f647c87878c21a67951b347a5d4a5f90e897f3f6c0db360a3779d0df
…,0] Trigger Unexpected Wallet Errors in AddWalletDescriptor

97d383a Test updating non-ranged descriptor with [0,0] range succeeds (Novo)
2ae1788 Skip range verification for non-ranged desc (Novo)

Pull request description:

  Closes bitcoin#31728

  This PR updates the `DescriptorScriptPubKeyMan` to skip range checks for non-ranged descriptors, which previously caused errors when updating a non-ranged descriptor with the range [0,0]

  #### Testing
  A unit test was added to test the new behaviour

ACKs for top commit:
  achow101:
    ACK 97d383a
  rkrux:
    ACK 97d383a

Tree-SHA512: 6dbd058376d9e57d26477d9d6d89646e80a32e3ffcc9f4e30eeda273575d12583ce520cc0032cc67c12ea0b3ad344fbd3945d9fc5e389b6a6bce1ea7ad5d6e59
… unify sighash type match checking

ee045b6 rpc, psbt: Require sighashes match for descriptorprocesspsbt (Ava Chow)
2b7682c psbt: use sighash type field to determine whether to remove non-witness utxos (Ava Chow)
28781b5 psbt: Add sighash types to PSBT when not DEFAULT or ALL (Ava Chow)
15ce1bd psbt: Enforce sighash type of signatures matches psbt (Ava Chow)
1f71cd3 wallet: Remove sighash type enforcement from FillPSBT (Ava Chow)
4c7d767 psbt: Check sighash types in SignPSBTInput and take sighash as optional (Ava Chow)
a118256 script: Add IsPayToTaproot() (Ava Chow)
d6001dc wallet: change FillPSBT to take sighash as optional (Ava Chow)
e58b680 psbt: Return PSBTError from SignPSBTInput (Ava Chow)
2adfd81 tests: Test PSBT sighash type mismatch (Ava Chow)
5a5d26d psbt: Require ECDSA signatures to be validly encoded (Ava Chow)

Pull request description:

  Currently, we do not add the sighash field to PSBTs at all, even when we have signed with a non-default sighash. This PR changes the behavior such that when we (attempt to) sign with a sighash other than DEFAULT or ALL, the sighash type field will be added to the PSBT to inform the later signers that a different sighash type was used by a signer. Notably, this is necessary for MuSig2 support as all signers must sign using the same sighash type, but the sighash is not provided in partial signatures.

  Furthermore, because the sighash type can also be provided on the command line, we require that if both a command line sighash type and the sighash field is present, they must specify the same sighash type. However, this was being checked by the wallet, rather than the signing code, so the `descriptorprocesspsbt` RPC was not enforcing this restriction at all, and in fact ignored the sighash field entirely. This PR refactors the checking code so that the underlying PSBT signing function `SignPSBTInput` does the check.

ACKs for top commit:
  theStack:
    re-ACK ee045b6
  rkrux:
    re-ACK ee045b6
  fjahr:
    Code review ACK ee045b6

Tree-SHA512: 4ead5be1ef6756251b827f594beba868a145d75bf7f4ef6f15ad21f0ae4b8d71b38c83494e5a6b75f37fadd097178cddd93d614b962a2c72fc134f00ba2f74ae
e8661aa wallet: drop watch-only things from interface (Sjors Provoost)
e99188e qt: drop unused watch-only functionality (Sjors Provoost)

Pull request description:

  The watch-only functionality in the GUI was only used for legacy wallets. Watch-only descriptor wallets do not use this.

  The only visible changes of this PR should be:
  - dropped "Spendable:" label from the overview tab
  - column width cache is reset

  This PR also removes some unused variables from the interface.

ACKs for top commit:
  davidgumberg:
    Review ACK bitcoin@e8661aa.
  hebasto:
    ACK e8661aa, I have reviewed the code and it looks OK. The `src/qt/forms/overviewpage.ui` form was reviewed in Qt Designer.

Tree-SHA512: d7edb0f167e0b934075398a76eddca69890bb36848a918c932b1c2cea85ee87285e876cbfdf1f6dec7adf26b9f405fb558c70bec0c84585c0a9df33c2af78393
e63a703 subprocess: Don't add an extra whitespace at end of Windows command line (laanwj)

Pull request description:

  A list of the backported PRs:
  - arun11299/cpp-subprocess#119

  The following PRs were skipped for backporting:
  - arun11299/cpp-subprocess#118 because there is no changes in the header code.

  Required for bitcoin#32566.

ACKs for top commit:
  laanwj:
    Code review ACK e63a703

Tree-SHA512: 69a74aa7f9c611a9ec910e27161c5e9e147067d37f8335953cd3875fcc88dc840a2f7b206bb603f22507159e406b1449f1dc4702fffe890bb824672641b4feed
fab97f5 ci: Avoid && dropping errors (MarcoFalke)

Pull request description:

  In bash, `&&` will ignore errexit. This can lead to silently ignoring errors. Compare the output of:

  ```
  $ bash -c 'set -xe;   false && false   ; true; echo $?'
  + false
  + true
  + echo 0
  0
  ```

  In theory this could be fixed by using a subshell:

  ```
  $ bash -c 'set -xe; ( false && false ) ; true; echo $?'
  + false
  ```

  However, it is easier to just remove the `&&`.

  This was introduced in  commit faa807b

ACKs for top commit:
  janb84:
    Code review ACK bitcoin@fab97f5
  hebasto:
    ACK fab97f5.
  laanwj:
    ACK fab97f5

Tree-SHA512: 9d034829e03ef3aefdaad82c3cab59bf3fe18529762271c1ad3c838357e337e94bd403b77e30c0cf69715254b65addff6d12f2fb497d7a0e2cdcbcbf78858d47
…escriptor` instead of returning `nullptr` for some errors

785e140 wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)

Pull request description:

  bitcoin#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.

  The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.

ACKs for top commit:
  Sjors:
    utACK 785e140
  ryanofsky:
    Code review ACK 785e140
  furszy:
    Code review ACK 785e140

Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
…SHANI}` from `bitcoin-build-config.h`

800b7cc cmake: Add missed `SSE41_CXXFLAGS` (Hennadii Stepanov)
028476e cmake: Remove `ENABLE_ARM_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
1e90052 cmake: Remove `ENABLE_X86_SHANI` from `bitcoin-build-config.h` (Hennadii Stepanov)
8689628 cmake: Remove `ENABLE_AVX2` from `bitcoin-build-config.h` (Hennadii Stepanov)
a8e2342 cmake: Remove `ENABLE_SSE41` from `bitcoin-build-config.h` (Hennadii Stepanov)

Pull request description:

  `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}`  are already conditionally defined for the [`bitcoin_crypto`](https://github.com/bitcoin/bitcoin/blob/master/src/crypto/CMakeLists.txt) target, and they are not used by any other targets. Defining them globally in `bitcoin-build-config.h` is therefore redundant.

  Additionally, the previously missing `SSE41_CXXFLAGS` variable has been [added](bitcoin#32550 (comment)).

ACKs for top commit:
  fanquake:
    ACK 800b7cc

Tree-SHA512: da792a0b780c67b432b09c9288ca98d62545315c721fed13510d1c11f8bb0cddd9a4ed7a009b4d052471dda19d0641bbc1eae4805fc306d23bf9b4ef510089c8
Since production code only uses keys of length 8, we're not testing with other values anymore
Since a previous PR already solved the tiny byte-array xors during serialization, we're only concentrating on big continuous chunks now.

>  cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release \
&& cmake --build build -j$(nproc) \
&& build/bin/bench_bitcoin -filter='ObfuscationBench' -min-time=10000

C++ compiler .......................... AppleClang 17.0.0.17000013

|              ns/MiB |               MiB/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|          731,927.62 |            1,366.26 |    0.2% |     10.67 | `ObfuscationBench`

C++ compiler .......................... GNU 13.3.0

|              ns/MiB |               MiB/s |    err% |         ins/MiB |         cyc/MiB |    IPC |        bra/MiB |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|          941,015.26 |            1,062.68 |    0.0% |    9,437,186.97 |    3,378,911.52 |  2.793 |   1,048,577.15 |    0.0% |     10.99 | `ObfuscationBench`
This simplifies the next commits.
Since `CDBWrapper::Read` still supports vector only, we can initialize `m_obfuscation` directly instead of using a separate helper.
`CreateObfuscation` was also inlined, replaced `key_exists` with `key_missing`, and simplified the `if` condition that writes a new obfuscation key.
Since we're already throwing via `HandleError` here, the obfuscation key is also validated by throwing if invalid.
…yte>` to `uint64_t`

Since `util::Xor` now operates on `uint64_t` keys, migrated the obfuscation key end‑to‑end - from use sites in util::Xor up through streams, LevelDB wrappers, and disk (de)serialization - replacing all former `std::vector<std::byte>` keys with `uint64_t` (we still serialize them as vectors but convert immediately to `uint64_t` on load). This is why tests still generate vector keys and convert them to `uint64_t` later instead of generating them directly.

We also short‑circuit `Xor` calls when the key is zero to avoid unnecessary calculations (e.g., `MakeWritableByteSpan`).

In `Obfuscation::Unserialize` we can safely throw an `std::logic_error` since during mempool fuzzing `mempool_persist.cpp#L141` catches and ignored these errors safely.

>  cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release \
&& cmake --build build -j$(nproc) \
&& build/bin/bench_bitcoin -filter='ObfuscationBench' -min-time=10000

C++ compiler .......................... AppleClang 17.0.0.17000013

|              ns/MiB |               MiB/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|           14,730.40 |           67,886.80 |    0.1% |     11.01 | `ObfuscationBench`

C++ compiler .......................... GNU 13.3.0

|              ns/MiB |               MiB/s |    err% |         ins/MiB |         cyc/MiB |    IPC |        bra/MiB |   miss% |     total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
|           51,187.17 |           19,536.15 |    0.0% |      327,683.95 |      183,747.58 |  1.783 |      65,536.55 |    0.0% |     11.00 | `ObfuscationBench`

----

A few other benchmarks that seem to have improved as well (tested with Clang only):
Before:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        2,202,618.49 |              454.01 |    0.2% |     11.01 | `ReadBlockBench`
|          734,444.92 |            1,361.57 |    0.3% |     10.66 | `ReadRawBlockBench`

After:

|               ns/op |                op/s |    err% |     total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
|        1,912,308.06 |              522.93 |    0.4% |     10.98 | `ReadBlockBench`
|           49,092.93 |           20,369.53 |    0.2% |     10.99 | `ReadRawBlockBench`

Co-authored-by: Hodlinator <[email protected]>
@l0rinc l0rinc closed this Jun 8, 2025
l0rinc pushed a commit that referenced this pull request Jun 25, 2025
Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and:
```bash
export CC=clang
export CXX=clang++
cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address
cmake --build build
export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan"
ctest --test-dir build
```

```bash
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms
********* Finished testing of AddressBookTests *********

=================================================================
==21869==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 88 byte(s) in 1 object(s) allocated from:
    #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
    #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
    #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
    #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18
    #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5
    #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75
    #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13
    #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5
    #8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5
    #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
    #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
    #11 0xffff8cf57c54  (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #12 0xffff8cf5fa18  (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #13 0xffff8cf6067c  (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #14 0xffff8cf610a4  (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
    #17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30
    #18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
    #19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
    #20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
```

This happens when building using depends:
```bash
Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #1 0xfbff97f8c164  (<unknown module>)
    #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
    #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o
    #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)

SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s).
```
l0rinc pushed a commit that referenced this pull request Jun 25, 2025
5be31b2 lsan: add more Qt suppressions (fanquake)

Pull request description:

  Using Clang clang version 20.1.6 (Fedora 20.1.6-9.fc43) and:
  ```bash
  export CC=clang
  export CXX=clang++
  cmake -B build -DBUILD_GUI=ON -DSANITIZERS=address
  cmake --build build
  export LSAN_OPTIONS="suppressions=/root/bitcoin/test/sanitizer_suppressions/lsan"
  ctest --test-dir build
  ```

  ```bash
  Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 1589ms
  ********* Finished testing of AddressBookTests *********

  =================================================================
  ==21869==ERROR: LeakSanitizer: detected memory leaks

  Direct leak of 88 byte(s) in 1 object(s) allocated from:
      #0 0xaaaab5d5af40 in operator new(unsigned long) (/root/bitcoin/build/bin/test_bitcoin-qt+0x39af40) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
      #1 0xffff8c8f56cc in QLayoutPrivate::createWidgetItem(QLayout const*, QWidget*) (/lib64/libQt6Widgets.so.6+0x1a56cc) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
      #2 0xffff8c8d2f90 in QBoxLayout::insertWidget(int, QWidget*, int, QFlags<Qt::AlignmentFlag>) (/lib64/libQt6Widgets.so.6+0x182f90) (BuildId: 8b7b9e470f4d4cd920282a4f963abb01225814fa)
      #3 0xaaaab5fc7188 in SendCoinsDialog::addEntry() /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:596:18
      #4 0xaaaab5fc4eec in SendCoinsDialog::SendCoinsDialog(PlatformStyle const*, QWidget*) /root/bitcoin/build/src/qt/./qt/sendcoinsdialog.cpp:84:5
      #5 0xaaaab5da67ac in (anonymous namespace)::MiniGUI::MiniGUI(interfaces::Node&, PlatformStyle const*) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:235:75
      #6 0xaaaab5da2000 in (anonymous namespace)::TestGUI(interfaces::Node&, std::shared_ptr<wallet::CWallet> const&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:270:13
      #7 0xaaaab5d9ebc8 in (anonymous namespace)::TestGUI(interfaces::Node&) /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:453:5
      #8 0xaaaab5d9ebc8 in WalletTests::walletTests() /root/bitcoin/build/src/qt/test/./qt/test/wallettests.cpp:475:5
      #9 0xffff8b1c5314 in QMetaMethodInvoker::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195314) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
      #10 0xffff8b1c5dc8 in QMetaMethod::invokeImpl(QMetaMethod, void*, Qt::ConnectionType, long long, void const* const*, char const* const*, QtPrivate::QMetaTypeInterface const* const*) (/lib64/libQt6Core.so.6+0x195dc8) (BuildId: eacb2d1228362560e5df1a1ce496c99ad61960e7)
      #11 0xffff8cf57c54  (/lib64/libQt6Test.so.6+0x27c54) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #12 0xffff8cf5fa18  (/lib64/libQt6Test.so.6+0x2fa18) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #13 0xffff8cf6067c  (/lib64/libQt6Test.so.6+0x3067c) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #14 0xffff8cf610a4  (/lib64/libQt6Test.so.6+0x310a4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #15 0xffff8cf61aa4 in QTest::qRun() (/lib64/libQt6Test.so.6+0x31aa4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #16 0xffff8cf61eb4 in QTest::qExec(QObject*, int, char**) (/lib64/libQt6Test.so.6+0x31eb4) (BuildId: 96bb1cdeead53af0ced36d7970cf9cd79c4c4ccd)
      #17 0xaaaab5d7d77c in main /root/bitcoin/build/src/qt/test/./qt/test/test_main.cpp:95:30
      #18 0xffff8aad6398 in __libc_start_call_main (/lib64/libc.so.6+0x26398) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
      #19 0xffff8aad6478 in __libc_start_main@GLIBC_2.17 (/lib64/libc.so.6+0x26478) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)
      #20 0xaaaab5c74cac in _start (/root/bitcoin/build/bin/test_bitcoin-qt+0x2b4cac) (BuildId: c0e038f1c507ea6860d1cfd499ac54ad83359872)
  ```

  This happens when building using depends:
  ```bash
  Indirect leak of 24 byte(s) in 1 object(s) allocated from:
      #0 0xaaaabdbe86f8 in malloc (/root/bitcoin/build/bin/test_bitcoin-qt+0x4386f8) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #1 0xfbff97f8c164  (<unknown module>)
      #2 0xaaaabf0cfaa4 in QDBusConnectionPrivate::QDBusConnectionPrivate() (/root/bitcoin/build/bin/test_bitcoin-qt+0x191faa4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #3 0xaaaabf0c9e30 in QDBusConnectionManager::doConnectToStandardBus(QDBusConnection::BusType, QString const&, bool) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919e30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #4 0xaaaabf0cb0e4 in QtPrivate::QCallableObject<QDBusConnectionPrivate* (QDBusConnectionManager::*)(QDBusConnection::BusType, QString const&, bool), QtPrivate::List<QDBusConnection::BusType&, QString const&, bool&>, QDBusConnectionPrivate*>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x191b0e4) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #5 0xaaaabf5cbaf0 in QObject::event(QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e1baf0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #6 0xaaaabf5a4ce0 in QCoreApplicationPrivate::notify_helper(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df4ce0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #7 0xaaaabf5a486c in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df486c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #8 0xaaaabf5a575c in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df575c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #9 0xaaaabf66b858 in QEventDispatcherUNIX::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1ebb858) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #10 0xaaaabf5a9e3c in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1df9e3c) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #11 0xaaaabf632a44 in QThread::exec() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1e82a44) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #12 0xaaaabf0c9bd0 in QDBusConnectionManager::run() (/root/bitcoin/build/bin/test_bitcoin-qt+0x1919bd0) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #13 0xaaaabf669c30 in QThreadPrivate::start(void*) (/root/bitcoin/build/bin/test_bitcoin-qt+0x1eb9c30) (BuildId: dd54811dc11325890f7bac3e3a49d38f5a7ffef5)
      #14 0xaaaabdbe5f2c in asan_thread_start(void*) asan_interceptors.cpp.o
      #15 0xffff99538608 in thread_start (/lib64/libc.so.6+0xf8608) (BuildId: 627f878dd454ee3cc1dfdbd347bb565f1ffb53e7)

  SUMMARY: AddressSanitizer: 3592 byte(s) leaked in 37 allocation(s).
  ```

ACKs for top commit:
  maflcko:
    lgtm ACK 5be31b2

Tree-SHA512: 0c33661c7ec83ea9b874c1ee4ee2de513131690287363e216a88560dfb31a59ef563a50af756c86a991583aa64a600a74e20fd5d6a104cf4c0a27532de8d2211
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.