-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: Merge (partial) bitcoin#28725, 28771, 28814, 28546, 28741, 28727, 22764 #7280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
6f1fd46
1d6d0e0
df650a8
fd77c25
88e298f
c09b847
c6a6a5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,3 +17,4 @@ export RUN_FUZZ_TESTS=true | |
| export GOAL="install" | ||
| export BITCOIN_CONFIG="--enable-zmq --enable-fuzz --with-sanitizers=fuzzer,address,undefined,float-divide-by-zero,integer \ | ||
| CC='clang-19 -ftrivial-auto-var-init=pattern' CXX='clang++-19 -ftrivial-auto-var-init=pattern'" | ||
| export LLVM_SYMBOLIZER_PATH="/usr/bin/llvm-symbolizer-17" | ||
|
knst marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The active CI image sets Useful? React with 👍 / 👎. |
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,7 +6,6 @@ | |||||||||
| import sys | ||||||||||
| import re | ||||||||||
| import multiprocessing | ||||||||||
| from typing import Dict, List, Set | ||||||||||
|
|
||||||||||
| MAPPING = { | ||||||||||
| 'core_read.cpp': 'core_io.cpp', | ||||||||||
|
|
@@ -38,13 +37,13 @@ def module_name(path): | |||||||||
| return None | ||||||||||
|
|
||||||||||
| files = dict() | ||||||||||
| deps: Dict[str, Set[str]] = dict() | ||||||||||
| deps: dict[str, Set[str]] = dict() | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
After removing Useful? React with 👍 / 👎. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Line 37 still uses 🐛 Proposed fix-deps: dict[str, Set[str]] = dict()
+deps: dict[str, set[str]] = dict()📝 Committable suggestion
Suggested change
🧰 Tools🪛 Flake8 (7.3.0)[error] 37-37: undefined name 'Set' (F821) 🪛 Ruff (0.15.20)[error] 37-37: Undefined name (F821) 🤖 Prompt for AI AgentsSource: Linters/SAST tools There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Complete bitcoin#28725's nested Set-to-set transformation The typing import was removed, but this annotation still references Set. Running the actual script on Python 3.10.19 raises NameError before any dependency analysis begins, breaking the circular-dependency lint; F821 is also enabled by the active Python linter. Upstream converts both levels to dict[str, set[str]]. Complete the nested conversion rather than leaving the removed import's name in use.
Suggested change
source: ['claude'] |
||||||||||
|
|
||||||||||
| # Defined at module level (reading the global `deps`) so it pickles by reference | ||||||||||
| # for multiprocessing.Pool; forked workers inherit the populated `deps`. | ||||||||||
| def handle_module2(module): | ||||||||||
| # Build the transitive closure of dependencies of module | ||||||||||
| closure: Dict[str, List[str]] = dict() | ||||||||||
| closure: dict[str, list[str]] = dict() | ||||||||||
| for dep in deps[module]: | ||||||||||
| closure[dep] = [] | ||||||||||
| while True: | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1157,23 +1157,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx | |
| // If wallet doesn't have a chain (e.g when using dash-wallet tool), | ||
| // don't bother to update txn. | ||
| if (HaveChain()) { | ||
| bool active; | ||
| auto lookup_block = [&](const uint256& hash, int& height, TxState& state) { | ||
| // If tx block (or conflicting block) was reorged out of chain | ||
| // while the wallet was shutdown, change tx status to UNCONFIRMED | ||
| // and reset block height, hash, and index. ABANDONED tx don't have | ||
| // associated blocks and don't need to be updated. The case where a | ||
| // transaction was reorged out while online and then reconfirmed | ||
| // while offline is covered by the rescan logic. | ||
| if (!chain().findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) { | ||
| state = TxStateInactive{}; | ||
| } | ||
| }; | ||
| if (auto* conf = wtx.state<TxStateConfirmed>()) { | ||
| lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, wtx.m_state); | ||
| } else if (auto* conf = wtx.state<TxStateConflicted>()) { | ||
| lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, wtx.m_state); | ||
| } | ||
| wtx.updateState(chain()); | ||
| } | ||
| if (/* insertion took place */ ins.second) { | ||
| wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx)); | ||
|
|
@@ -4044,8 +4028,10 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const | |
| { | ||
| AssertLockHeld(cs_wallet); | ||
| if (auto* conf = wtx.state<TxStateConfirmed>()) { | ||
| assert(conf->confirmed_block_height >= 0); | ||
| return GetLastBlockHeight() - conf->confirmed_block_height + 1; | ||
| } else if (auto* conf = wtx.state<TxStateConflicted>()) { | ||
| assert(conf->conflicting_block_height >= 0); | ||
|
Comment on lines
4030
to
+4034
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Blocking: Missing prerequisite: bitcoin#28609 before asserting transaction heights These assertions turn an existing migration state bug into a node abort. Transaction deserialization assigns confirmed/conflicted heights of -1. MigrateLegacyToDescriptor loads the source wallet without a chain, and ApplyMigrationData copies its transaction states into the watchonly wallet through AddToWallet, which does not resolve those heights. The success path reloads only the main wallet, so querying an affected watchonly transaction reaches the new assertion with height -1. The extracted updateState helper does not fix this because its call remains in LoadToWallet behind HaveChain(). Upstream bitcoin#28609 reloads the auxiliary wallets and changes migration state handling before bitcoin#28546 adds these assertions. Backport that prerequisite and its tests, adapting the migration path before enabling the assertions. source: ['claude'] |
||
| return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1); | ||
| } else { | ||
| return 0; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -593,6 +593,13 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati | |||||||||||||||||||||||||||||
| * <0 : conflicts with a transaction this deep in the blockchain | ||||||||||||||||||||||||||||||
| * 0 : in memory pool, waiting to be included in a block | ||||||||||||||||||||||||||||||
| * >=1 : this many blocks deep in the main chain | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * Preconditions: it is only valid to call this function when the wallet is | ||||||||||||||||||||||||||||||
| * online and the block index is loaded. So this cannot be called by | ||||||||||||||||||||||||||||||
| * bitcoin-wallet tool code or by wallet migration code. If this is called | ||||||||||||||||||||||||||||||
| * without the wallet being online, it won't be able able to determine the | ||||||||||||||||||||||||||||||
| * the height of the last block processed, or the heights of blocks | ||||||||||||||||||||||||||||||
| * referenced in transaction, and might cause assert failures. | ||||||||||||||||||||||||||||||
|
Comment on lines
+596
to
+602
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix wording typos in precondition comment. Line 535 has a duplicated word (“able able”), and the phrasing around block references is awkward. Small cleanup will avoid confusion in a precondition that now matters more. ✏️ Proposed comment fix- * without the wallet being online, it won't be able able to determine the
- * the height of the last block processed, or the heights of blocks
- * referenced in transaction, and might cause assert failures.
+ * without the wallet being online, it won't be able to determine the
+ * height of the last block processed, or the heights of blocks
+ * referenced by transactions, and might cause assert failures.📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is non-functional upstream comment wording in a backport. Upstream cosmetic style is explicitly excluded from general backport findings. |
||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); | ||||||||||||||||||||||||||||||
| bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: dashpay/dash
Length of output: 446
Address LLVM version mismatch and verify CI-scope approval.
Line 19 pins
llvm-symbolizer-17while line 18 compiles withclang-19. This major-version mismatch between the compiler and symbolizer can degrade stack trace quality during fuzzing and sanitizer reporting. Additionally, this change modifies theci/**directory, which per coding guidelines should not be changed unless explicitly prompted. Confirm that this CI modification is in scope and that the symbolizer version mismatch is intentional.🤖 Prompt for AI Agents