Skip to content

Conversation

l0rinc
Copy link
Contributor

@l0rinc l0rinc commented Aug 7, 2025

Avoid mutating the shared active tip CBlockIndex in the blockmanager_readblock_hash_mismatch test.
Instead, construct a local CBlockIndex with only the required fields set, ensuring the test remains self-contained and hopefully eliminating the data race reported in #33150.

@DrahtBot DrahtBot added the Tests label Aug 7, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 7, 2025

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33154.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@l0rinc l0rinc force-pushed the l0rinc/readblock-hash-mismatch-test branch from 1f7821b to 73e0f07 Compare August 8, 2025 23:10
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK

…id data race

Co-authored-by: stickies-v <stickies-v@protonmail.com>
@l0rinc l0rinc force-pushed the l0rinc/readblock-hash-mismatch-test branch from 73e0f07 to cb173b8 Compare August 12, 2025 18:35
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK cb173b8

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK cb173b8

fake_index->phashBlock = &uint256::ONE; // invalid block hash
CBlockIndex index;
{
LOCK(cs_main);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could use the m_node.chainman->GetMutex() alias for new code.

@maflcko maflcko added this to the 30.0 milestone Aug 20, 2025
@fanquake fanquake merged commit bc797d2 into bitcoin:master Aug 20, 2025
19 checks passed
alexanderwiederin added a commit to alexanderwiederin/rust-bitcoinkernel that referenced this pull request Aug 21, 2025
…88ae28a

bce88ae28a kernel: Fix bitcoin-chainstate for windows
3a7e9f0eaf kernel: Add Purpose section to header documentation
5bae79ace5 kernel: Allowing reducing exports
d0308a2489 kernel: Add pure kernel bitcoin-chainstate
05a569070c kernel: Add functions to get the block hash from a block
8566ec6e83 kernel: Add block index utility functions to C header
b4d0e80f84 kernel: Add function to read block undo data from disk to C header
488999ac77 kernel: Add functions to read block from disk to C header
3dc76bb7f7 kernel: Add function for copying block data to C header
6151b45a42 kernel: Add functions for the block validation state to C header
5d00432f27 kernel: Add validation interface to C header
facf209aee kernel: Add interrupt function to C header
129f553e4e kernel: Add import blocks function to C header
f7ed7b944d kernel: Add chainstate load options for in-memory dbs in C header
67d9f53a98 kernel: Add options for reindexing in C header
ebc826319f kernel: Add block validation to C header
511a1c8a78 kernel: Add chainstate loading when instantiating a ChainstateManager
aad295899e kernel: Add chainstate manager option for setting worker threads
c701cb2405 kernel: Add chainstate manager object to C header
1df8b87602 kernel: Add notifications context option to C header
571c1a2acb kernel: Add chain params context option to C header
a2cab9f1cd kernel: Add kernel library context object
944ef6b630 kernel: Add logging to kernel library C header
d0cb841fba kernel: Introduce initial kernel C header API
04c115dfde Merge bitcoin/bitcoin#33078: kernel: improve BlockChecked ownership semantics
bc797d2271 Merge bitcoin/bitcoin#33154: test: use local `CBlockIndex` in block read hash mismatch check
d3c58a5be9 Merge bitcoin/bitcoin#33193: Release: Prepare "Translation string freeze" step
9cf7b3d90c Merge bitcoin/bitcoin#33211: test: modify logging_filesize_rate_limit params
f5f853d952 Merge bitcoin/bitcoin#32878: index: fix wrong assert of current_tip == m_best_block_index
5dda364c4b test: modify logging_filesize_rate_limit params
0df2c3c42e qt: Update `src/qt/locale/bitcoin_en.xlf` translation source file
22e689587a Merge bitcoin/bitcoin#33209: cmake: Drop python dependency for translate
be356fc49b Merge bitcoin/bitcoin#32896: wallet, rpc: add v3 transaction creation and wallet support
3c4a109aa8 cmake: Drop python dependency for translate
f58de8749e Merge bitcoin/bitcoin#32345: ipc: Handle unclean shutdowns better
d31dc8f818 Merge bitcoin/bitcoin#33200: cmake: Introduce translate.cmake script for translate target
05255d5d1e cmake: Drop dependency on sed for translate target
d5054beca5 cmake: Introduce translate.cmake script for translate target
57e8f34fe2 Merge bitcoin/bitcoin#32977: wallet: Remove wallet version and several legacy related functions
97593c1fd3 Merge bitcoin/bitcoin#32975: assumevalid: log every script validation state change
5c8bf7b39e doc: add release notes for version 3 transactions
4ef8065a5e test: add truc wallet tests
5d932e14db test: extract `bulk_vout` from `bulk_tx` so it can be used by wallet tests
2cb473d9f2 rpc: Support version 3 transaction creation
4c20343b4d rpc: Add transaction min standard version parameter
c5a2d08011 wallet: don't return utxos from multiple truc txs in AvailableCoins
da8748ad62 wallet: limit v3 tx weight in coin selection
85c5410615 wallet: mark unconfirmed v3 siblings as mempool conflicts
0804fc3cb1 wallet: throw error at conflicting tx versions in pre-selected inputs
cc155226fe wallet: set m_version in coin control to default value
2e9617664e  wallet: don't include unconfirmed v3 txs with children in available coins
ec2676becd wallet: unconfirmed ancestors and descendants are always truc
7b4a1350df Merge bitcoin/bitcoin#33183: validation: rename block script verification error from "mandatory" to "block"
c99f5c5e1b Merge bitcoin/bitcoin#33106: policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee
578b512bdd Merge bitcoin/bitcoin#33011: log: rate limiting followups
8405fdb06e Merge bitcoin/bitcoin#33169: interfaces, chain, refactor: Remove unused getTipLocator and incaccurate getActiveChainLocator
c0d91fc69c Add release note for #33050 and #33183 error string changes
e17b5da0d6 Merge bitcoin/bitcoin#33179: doc: update wallet build instruction
9b1a7c3e8d Merge bitcoin/bitcoin#33116: refactor: Convert uint256 to Txid
b3f781a0ef contrib: adapt max reject string size in tracing demo
9a04635432 scripted-diff: validation: rename mandatory errors into block errors
dbf8b0980b Merge bitcoin/bitcoin#33171: ci: Update `actions/checkout` version
d6887f0cec Merge bitcoin/bitcoin#33178: guix: increase maximum allowed (runtime) GCC to 7
2b00030af8 interfaces, chain, refactor: Remove inaccurate getActiveChainLocator
110a0f405c interfaces, chain, refactor: Remove unused getTipLocator
dadf15f88c Merge bitcoin/bitcoin#33050: net, validation: don't punish peers for consensus-invalid txs
cb173b8e93 test: use local `CBlockIndex` in block read hash mismatch test to avoid data race
73972d5617 Merge bitcoin/bitcoin#31296: wallet: Translate [default wallet] string in progress messages
67e186deb0 doc: update wallet build instruction
5c74a0b397 config: add DEBUG_ONLY -logratelimit
9f3b017bcc test: logging_filesize_rate_limit improvements
350193e5e2 test: don't leak log category mask across tests
05d7c22479 test: add ReadDebugLogLines helper function
3d630c2544 log: make m_limiter a shared_ptr
ec484bd5ce Merge bitcoin/bitcoin#31453: util: detect and warn when using exFAT on MacOS
776a163374 guix: increase maximum allowed (runtime) GCC to 7
ba84a25dee [doc] update mempool-replacements.md for incremental relay feerate change
273e600e65 Merge bitcoin/bitcoin#33021: test/refactor: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL
18720bc5d5 [doc] release note for min feerate changes
6da5de58ca [policy] lower default minrelaytxfee and incrementalrelayfee to 100sat/kvB
2e515d2897 [prep/test] make wallet_fundrawtransaction's minrelaytxfee assumption explicit
457cfb61b5 [prep/util] help MockMempoolMinFee handle more precise feerates
3eab8b7240 [prep/test] replace magic number 1000 with respective feerate vars
5f2df0ef78 [miner] lower default -blockmintxfee to 1sat/kvB
d6213d6aa1 [doc] assert that default min relay feerate and incremental are the same
1fbee5d7b6 [test] explicitly check default -minrelaytxfee and -incrementalrelayfee
72dc18467d [test] RBF rule 4 for various incrementalrelayfee settings
85f498893f [test] check bypass of minrelay for various minrelaytxfee settings
e5f896bb1f [test] check miner doesn't select 0fee transactions
de0675f9de refactor: Move `transaction_identifier.h` to primitives
6f068f65de Remove implicit uint256 conversion and comparison
9c24cda72e refactor: Convert remaining instances from uint256 to Txid
d2ecd6815d policy, refactor: Convert uint256 to Txid
f6c0d1d231 mempool, refactor: Convert uint256 to Txid
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid
41642d43b3 Merge bitcoin/bitcoin#33162: test: fix scripts in `blockfilter_basic_test`
f83c01d882 ci: Update `actions/checkout` version
a27430e259 Merge bitcoin/bitcoin#32473: Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts
34b366fa2c Merge bitcoin/bitcoin#33155: contrib: drop `bitcoin-util` exception from FORTIFY check
ca64b71ed5 test: fix scripts in `blockfilter_basic_test`
fab2980bdc assumevalid: log every script validation state change
e8f9c37a3b log: clean up LogPrintStr_ and Reset, prefix all logs with "[*]" when there are suppressions
3c7cae49b6 log: change LogLimitStats to struct LogRateLimiter::Stats
876dbdfb47 tests: drop expect_disconnect behaviour for tx relay
b29ae9efdf validation: only check input scripts once
266dd0e10d net_processing: drop MaybePunishNodeForTx
db3228042b util: detect and warn when using exFAT on macOS
4bff4ce561 contrib: drop bitcoin-util exception from FORTIFY check
83950275ed qa: unit test sighash caching
b221aa80a0 qa: simple differential fuzzing for sighash with/without caching
92af9f74d7 script: (optimization) introduce sighash midstate caching
8f3ddb0bcc script: (refactor) prepare for introducing sighash midstate cache
9014d4016a tests: add sighash caching tests to feature_taproot
49b3d3a92a Clean up `FindTxForGetData`
2581258ec2 ipc: Handle bitcoin-wallet disconnections
2160995916 ipc: Add Ctrl-C handler for spawned subprocesses
0c28068ceb doc: Improve IPC interface comments
7f65aac78b ipc: Avoid waiting for clients to disconnect when shutting down
6eb09fd614 test: Add unit test coverage for Init and Shutdown code
9a9fb19536 ipc: Use EventLoopRef instead of addClient/removeClient
5c45bc989b Merge commit 'e886c65b6b37aaaf5d22ca68bc14e55d8ec78212' into pr/ipc-stop-base
e886c65b6b Squashed 'src/ipc/libmultiprocess/' changes from 27c7e8e5a581..b4120d34bad2
3aef38f44b test: exercise index reorg assertion failure
acf50233cd index: fix wrong assert of current_tip == m_best_block_index
1d9f1cb4bd kernel: improve BlockChecked ownership semantics
554befd873 test: revive `getcoinscachesizestate`
64ed0fa6b7 refactor: modernize `LargeCoinsCacheThreshold`
1b40dc02a6 refactor: extract `LargeCoinsCacheThreshold` from `GetCoinsCacheSizeState`
8319a13468 log: clarify RATELIMIT_MAX_BYTES comment, use RATELIMIT_WINDOW
5f70bc80df log: remove const qualifier from arguments in LogPrintFormatInternal
b8e92fb3d4 log: avoid double hashing in SourceLocationHasher
616bc22f13 test: remove noexcept(false) comment in ~DebugLogHelper
9ba1fff29e kernel: refactor: ConnectTip to pass block pointer by value
60d1042b9a wallet: Remove unused `WalletFeature` enums
66de58208a wallet: Remove `CWallet::nWalletVersion` and related functions
7cda3d0f5b wallet: Remove `IsFeatureSupported()` and `CanSupportFeature()`
ba01585229 wallet: `MigrateToDescriptor` no longer calls `CanSupportFeature`
63acee2797 wallet: Remove `GetClosestWalletFeature()`
e27da3150b wallet: Remove `GetVersion()`
db225cea56 wallet, refactor: Replace GetDisplayName() with LogName()
01737883b3 wallet: Translate [default wallet] string in progress messages
REVERT: 35fced5df7 kernel: Fix bitcoin-chainstate for windows
REVERT: c164264fd8 kernel: Add Purpose section to header documentation
REVERT: 9096c35c05 kernel: Allowing reducing exports
REVERT: 6d0d4b2507 kernel: Add pure kernel bitcoin-chainstate
REVERT: ccd85f0333 kernel: Add functions to get the block hash from a block
REVERT: 925f21c37b kernel: Add block index utility functions to C header
REVERT: b7441841c9 kernel: Add function to read block undo data from disk to C header
REVERT: bc0e6f098e kernel: Add functions to read block from disk to C header
REVERT: 5120302f96 kernel: Add function for copying block data to C header
REVERT: 718ccee732 kernel: Add functions for the block validation state to C header
REVERT: eb363ab30e kernel: Add validation interface to C header
REVERT: 246886c6ea kernel: Add interrupt function to C header
REVERT: f3b34ca457 kernel: Add import blocks function to C header
REVERT: fe08857d52 kernel: Add chainstate load options for in-memory dbs in C header
REVERT: f93f171e01 kernel: Add options for reindexing in C header
REVERT: dca7b4c26e kernel: Add block validation to C header
REVERT: f031e9ce03 kernel: Add chainstate loading when instantiating a ChainstateManager
REVERT: 3cb99f73ec kernel: Add chainstate manager option for setting worker threads
REVERT: 9454ad8512 kernel: Add chainstate manager object to C header
REVERT: 3bead9ebdd kernel: Add notifications context option to C header
REVERT: dda805dfb6 kernel: Add chain params context option to C header
REVERT: ea5334925d kernel: Add kernel library context object
REVERT: 36fafbaef9 kernel: Add logging to kernel library C header
REVERT: d28eef5cf2 kernel: Introduce initial kernel C header API

git-subtree-dir: libbitcoinkernel-sys/bitcoin
git-subtree-split: bce88ae28ab2cd12f32aead1fbf47153c50c3b05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants