Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filter#899
Merge bitcoin/bitcoin#27675: p2p: Drop m_recently_announced_invs bloom filter#899DashCoreAutoGuix wants to merge 1 commit intobackport-0.26-batch-647from
Conversation
fb02ba3 mempool_entry: improve struct packing (Anthony Towns) 1a11806 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns) 6fa4993 test: Check tx from disconnected block is immediately requestable (glozow) e4ffabb net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns) 6ec1809 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns) a70beaf validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns) 1e9684f mempool_entry: add mempool entry sequence number (Anthony Towns) Pull request description: This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally). The philosophy here (compare with bitcoin#18861 and bitcoin#19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughThis change introduces a transaction sequence number to mempool entries and updates relay logic to use this sequence for transaction announcements and requests. The codebase is refactored to pass and track this sequence number throughout mempool entry creation, relay filtering, and peer management. Associated tests and utilities are updated to reflect the new constructor signatures and relay behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Critical Issues Found:🚨 Massive Scope Explosion (1282% vs Expected 80-150%)
🚨 Fundamental Implementation MismatchWhat Bitcoin Did: Modified only
🚨 CI Status: CRITICAL
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 No reviewer feedback to address at this time. Fundamental Problem:This PR appears to be a completely different implementation that coincidentally shares the same title as Bitcoin PR bitcoin#27675. The actual Bitcoin commit Recommended Actions:
This is not a faithful Bitcoin backport - it represents a fundamental implementation difference requiring complete rewrite or clarification of intent. |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/net_processing.cpp (1)
2653-2674: Scope creep in transaction relay logic needs to be revertedThe changes in src/net_processing.cpp (lines 2653–2674) introduce a brand-new
info_for_relay()call and sequence-number-based relay logic that do not appear in Bitcoin PR bitcoin#27675 (which only tweaked test/functional/p2p_filter.py). This is a fundamental alteration of core P2P behavior rather than a backport of the original 17-line test change.Please revert or isolate these relay-mechanism changes so that this PR only contains the intended backport.
• Affected file:
- src/net_processing.cpp (lines 2653–2674)
• Original Bitcoin change:
- test/functional/p2p_filter.py only
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
src/bench/mempool_eviction.cpp(1 hunks)src/bench/mempool_stress.cpp(1 hunks)src/bench/rpc_mempool.cpp(1 hunks)src/net_processing.cpp(5 hunks)src/node/interfaces.cpp(1 hunks)src/test/util/setup_common.cpp(2 hunks)src/txmempool.cpp(2 hunks)src/txmempool.h(4 hunks)src/validation.cpp(1 hunks)test/functional/mempool_reorg.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
C++20 codebase should be placed under src/
Files:
src/node/interfaces.cppsrc/bench/mempool_stress.cppsrc/test/util/setup_common.cppsrc/validation.cppsrc/txmempool.hsrc/bench/rpc_mempool.cppsrc/bench/mempool_eviction.cppsrc/txmempool.cppsrc/net_processing.cpp
**
⚙️ CodeRabbit Configuration File
**: # CodeRabbit AI Review Instructions for Dash BackportsYour Role
You are reviewing Bitcoin Core backports to Dash Core. Your ONLY job is to validate that the Dash commit faithfully represents the original Bitcoin commit with minimal, necessary adaptations.
Critical Validation Rules
1. File Operations Must Match (AUTO-REJECT if violated)
- If Bitcoin modifies an existing file → Dash MUST modify (not create new)
- If Bitcoin creates a new file → Dash creates
- If Bitcoin deletes a file → Dash deletes
- Common failure: Bitcoin modifies keys.txt, Dash creates new file with 58 keys
2. Size Ratio Check (80-150% of Bitcoin)
- Count functional lines changed (exclude comments/whitespace)
- Dash changes should be 80-150% of Bitcoin's size
- Red flag: 2-line Bitcoin fix becoming 150+ lines in Dash
3. No Scope Creep
- Reject if you see: "TODO:", "FIXME:", "while we're here", "also fix"
- No unrelated refactoring or style changes
- Only Bitcoin's intended changes + minimal Dash adaptations
4. Bitcoin-Specific Code Detection
- Auto-reject witness/segwit code:
msg_wtxidrelay,MSG_WTX, witness imports- Auto-reject RBF (replace-by-fee) functionality
- Note: PSBT is supported in Dash (don't flag)
5. Mandatory Adaptations Only
bitcoin→dashin strings/pathsBitcoin→Dashin user-facing text- Port numbers: 8332→9998 (RPC), 8333→9999 (P2P)
- Hardcoded test values specific to Dash
- No other changes unless absolutely required
6. Completeness Check
- All files changed in Bitcoin must be present
- Extra files need clear justification (Dash-specific compatibility)
- Missing files = incomplete backport
Review Process
- First: Check file operations match exactly
- Second: Calculate size ratio
- Third: Scan for scope creep patterns
- Fourth: Detect Bitcoin-specific code
- Fifth: Verify all changes are minimal adaptations
Output Format
VALIDATION: [PASS/FAIL] File Operatio...
Files:
src/node/interfaces.cppsrc/bench/mempool_stress.cppsrc/test/util/setup_common.cppsrc/validation.cppsrc/txmempool.htest/functional/mempool_reorg.pysrc/bench/rpc_mempool.cppsrc/bench/mempool_eviction.cppsrc/txmempool.cppsrc/net_processing.cpp
src/bench/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Performance benchmarks should be placed in src/bench/ and use nanobench
Files:
src/bench/mempool_stress.cppsrc/bench/rpc_mempool.cppsrc/bench/mempool_eviction.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cpp
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be placed in test/functional/ and written in Python
Files:
test/functional/mempool_reorg.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T19:54:21.426Z
Learning: In Dash backports from Bitcoin Core, including necessary compilation fixes (such as API compatibility changes like UniValue get_int() → getInt<int>()) alongside the core backport is standard and expected practice. These compatibility fixes ensure the backported code compiles in Dash's evolved codebase while preserving Bitcoin's original functionality and intent.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-27T22:35:10.176Z
Learning: In Dash backports, src/dashbls files are vendored dependencies that should not be modified during Bitcoin Core backports unless there is specific justification. Unauthorized modifications to vendored dependencies should be removed to maintain code integrity.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: In Dash backports from Bitcoin Core, when the DIFFICULTY_ADJUSTMENT_INTERVAL constant is missing, it should be defined as 24 for Dash (different from Bitcoin's value), as seen in the getnetworkhashps RPC backport fix.
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions as defined in src/evo/specialtx.h
📚 Learning: during multiple verification attempts of bitcoin core commit 06d469c backport to dash pr #566, da...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T22:03:12.364Z
Learning: During multiple verification attempts of Bitcoin Core commit 06d469c26b backport to Dash PR #566, DashCoreAutoGuix consistently identified scope creep in interface_usdt_utxocache.py where additional pruning test functionality was added beyond the original Bitcoin commit. The user provided comprehensive fixes including both scope creep removal and missing mempool test file additions, but couldn't push due to authentication restrictions. The scope creep fix was identified as the priority to resolve CI failures.
Applied to files:
src/bench/mempool_stress.cppsrc/test/util/setup_common.cpptest/functional/mempool_reorg.pysrc/bench/rpc_mempool.cppsrc/bench/mempool_eviction.cppsrc/net_processing.cpp
📚 Learning: during dash backport verification of bitcoin core commit 06d469c, scope creep was detected when a...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-28T20:34:29.061Z
Learning: During Dash backport verification of Bitcoin Core commit 06d469c26b, scope creep was detected when additional pruning test functionality was added to interface_usdt_utxocache.py beyond what was in the original Bitcoin commit. The fix involved removing the extra test block while maintaining the core compiler flag fixes for USDT compilation errors.
Applied to files:
src/bench/mempool_stress.cppsrc/test/util/setup_common.cpptest/functional/mempool_reorg.pysrc/bench/rpc_mempool.cpp
📚 Learning: applies to src/evo/specialtx.h : special transactions use payload extensions as defined in src/evo/s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions as defined in src/evo/specialtx.h
Applied to files:
src/bench/mempool_stress.cppsrc/test/util/setup_common.cppsrc/txmempool.hsrc/bench/mempool_eviction.cpp
📚 Learning: dashcoreautoguix successfully fixed scope creep in bitcoin core commit fcdb39d backport by removi...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T21:29:32.827Z
Learning: DashCoreAutoGuix successfully fixed scope creep in Bitcoin Core commit fcdb39d3ee backport by removing the parse test case from src/test/uint256_tests.cpp that was not part of the original Bitcoin commit. The fix was implemented in commit 16748115ce and verified through range-diff analysis.
Applied to files:
src/test/util/setup_common.cppsrc/net_processing.cpp
📚 Learning: applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : unit tests should be placed in s...
Learnt from: CR
PR: DashCoreAutoGuix/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-28T23:09:09.522Z
Learning: Applies to src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp} : Unit tests should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Applied to files:
src/test/util/setup_common.cpp
📚 Learning: in dash backports from bitcoin core, witness transaction-related code (msg_wtx, wtxid) should be rep...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T22:13:54.797Z
Learning: In Dash backports from Bitcoin Core, witness transaction-related code (MSG_WTX, wtxid) should be replaced with regular transaction handling (MSG_TX, txid) for compatibility, as demonstrated in the p2p_filter.py test fix where MSG_WTX was replaced with MSG_TX and irr_wtxid usage was replaced with irr_txid.
Applied to files:
test/functional/mempool_reorg.pysrc/net_processing.cpp
📚 Learning: when backporting bitcoin core commits that use python's textwrap.dedent() function in test files, th...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-29T17:13:35.087Z
Learning: When backporting Bitcoin Core commits that use Python's textwrap.dedent() function in test files, the textwrap import statement needs to be explicitly added if it's missing in the Dash test file.
Applied to files:
test/functional/mempool_reorg.py
📚 Learning: the `add_wallet_options` method is not available in dash's test framework and should be removed when...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: The `add_wallet_options` method is not available in Dash's test framework and should be removed when backporting Bitcoin Core tests that use this method, as demonstrated in wallet_rescan_unconfirmed.py.
Applied to files:
test/functional/mempool_reorg.py
📚 Learning: dashcoreautoguix successfully completed a complex bitcoin core backport (pr bitcoin#29412) for block mutati...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-31T01:14:55.631Z
Learning: DashCoreAutoGuix successfully completed a complex Bitcoin Core backport (PR #29412) for block mutation detection by implementing the IsBlockMutated function, adding net processing integration, creating comprehensive unit tests, and properly adapting all Bitcoin-specific witness code for Dash compatibility. The backport maintains full security functionality while respecting Dash's non-witness transaction architecture.
Applied to files:
test/functional/mempool_reorg.pysrc/net_processing.cpp
📚 Learning: in dash backports from bitcoin core test files, the `address_to_scriptpubkey` function should be imp...
Learnt from: DashCoreAutoGuix
PR: DashCoreAutoGuix/dash#0
File: :0-0
Timestamp: 2025-07-30T14:45:15.700Z
Learning: In Dash backports from Bitcoin Core test files, the `address_to_scriptpubkey` function should be imported from `test_framework.wallet` instead of `test_framework.address` as the import location differs between Bitcoin and Dash test frameworks.
Applied to files:
test/functional/mempool_reorg.py
🧬 Code Graph Analysis (2)
src/txmempool.h (2)
src/txmempool.cpp (2)
info_for_relay(1331-1340)info_for_relay(1331-1331)src/node/interfaces.cpp (6)
txid(892-897)txid(892-892)txid(898-904)txid(898-898)txid(913-918)txid(913-913)
src/bench/mempool_eviction.cpp (2)
src/txmempool.cpp (1)
CTxMemPoolEntry(51-69)src/txmempool.h (1)
CTxMemPoolEntry(98-140)
🪛 Ruff (0.12.2)
test/functional/mempool_reorg.py
36-36: Missing return type annotation for public function test_reorg_relay
Add return type annotation: None
(ANN201)
111-111: Missing return type annotation for public function run_test
Add return type annotation: None
(ANN201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (11)
src/node/interfaces.cpp (1)
928-928: Please verify constructor update against Bitcoin Core PR bitcoin#27675The extra
0parameter forentry_sequenceon line 928 ofsrc/node/interfaces.cppaligns with the mempool sequence‐tracking feature, but the local history doesn’t include Bitcoin Core PR bitcoin#27675. Before merging, confirm that this change exactly matches the upstream diff and that no unrelated scope creep was introduced.
- Verify that Bitcoin Core PR bitcoin#27675 modifies the
CTxMemPoolEntryconstructor call insrc/node/interfaces.cppat line 928.- Ensure no additional changes beyond that PR were pulled into this backport.
src/validation.cpp (1)
856-857: Ignore scope mismatch warning—validation.cpp is modified in Bitcoin PR bitcoin#27675Fetching the PR details confirms that Bitcoin Core PR bitcoin#27675 changes 15 files (151 additions, 91 deletions), including
src/validation.cpp. The backport’s update toCTxMemPoolEntryconstruction therefore aligns with the original Bitcoin commit’s scope.•
src/validation.cppis listed among the files modified by PR bitcoin#27675
• Total files changed in the original PR: 15 (not just a single test file)No further action needed—this change matches the upstream implementation.
Likely an incorrect or invalid review comment.
src/bench/mempool_eviction.cpp (1)
15-21: Constructor adaptation matches upstream PR bitcoin#27675The addition of
uint64_t sequence = 0and its insertion into theCTxMemPoolEntryconstructor exactly mirrors the Bitcoin Core PR bitcoin#27675 diff forsrc/bench/mempool_eviction.cpp. No extra changes have been introduced.• Bitcoin patch snippet (PR bitcoin#27675):
static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& pool) { int64_t nTime = 0; unsigned int nHeight = 1; + uint64_t sequence = 0; bool spendsCoinbase = false; unsigned int sigOpCost = 4; LockPoints lp; pool.addUnchecked(CTxMemPoolEntry( - tx, nFee, nTime, nHeight, + tx, nFee, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp)); }• Our backport matches this change exactly (using Dash’s
sigOpsalias).No further action required.
src/bench/rpc_mempool.cpp (1)
17-17: LGTM - Clean constructor adaptation.The inline parameter comments make the constructor call very readable, and the addition of
entry_sequence=0is consistent with the constructor signature change.src/bench/mempool_stress.cpp (1)
17-21: Consistent constructor adaptation across benchmark files.The pattern of adding a local
sequencevariable and passing it to theCTxMemPoolEntryconstructor is consistent with the previous benchmark files, indicating a coordinated codebase update.src/test/util/setup_common.cpp (3)
659-659: Test utility constructor update is mechanically correct.The addition of the sequence parameter (0) to the
CTxMemPoolEntryconstructor in the test utility is consistent with the constructor signature change.
673-674: Helper function correctly adapted to new constructor signature.The
TestMemPoolEntryHelper::FromTxmethod properly includes the new sequence parameter (0) in the constructor call.
659-674: Scope assessment incorrect—no scope creep
The original Bitcoin Core PR bitcoin#27675 modified 15 files (benchmarks, net_processing, node interfaces, txmempool, validation, and tests), not just a 17-line test update. Your backport mirrors that same set of changes across core and test code. No drastic scope reduction is needed—please proceed.Likely an incorrect or invalid review comment.
src/txmempool.cpp (1)
51-59: Cannot verify original Bitcoin commit; manual validation requiredThe commit ID
1f352cf2fd7isn’t present in the local history, so we can’t inspect the original 17-line change. Please:
- Confirm you’ve referenced the correct Bitcoin Core commit or branch.
- Fetch the upstream repository (e.g.
git fetch upstream) and rungit show <commit>to review the exact diff.- Ensure this PR only includes the minimal bloom-filter removal from the original, without introducing new constructor or architectural changes (e.g. the added
entry_sequenceparameter inCTxMemPoolEntry).Once you’ve validated the true upstream diff, please adjust this backport to match that scope or document why any extra changes are necessary.
src/txmempool.h (1)
1-1053: Unable to Verify Bitcoin Commit ReferenceI attempted to locate commit
1f352cf2fd7in the repository but it was not found. Please confirm that the commit hash is correct and accessible in this branch, or provide the exact Bitcoin commit details for backport validation.src/net_processing.cpp (1)
1-6331: Incorrect backport assumption—ignore previous feedbackThe changes in this PR correspond to Bitcoin Core PR bitcoin#27675, which adds an
entry_sequencefield toCTxMemPoolEntryand integrates it intonet_processing.cppto control how recently‐added transactions are relayed. The earlier review compared against the wrong Bitcoin PR (which only touched a test file) and is not applicable here.Likely an incorrect or invalid review comment.
| /** The mempool sequence num at which we sent the last `inv` message to this peer. | ||
| * Can relay txs with lower sequence numbers than this (see CTxMempool::info_for_relay). */ | ||
| uint64_t m_last_inv_sequence GUARDED_BY(NetEventsInterface::g_msgproc_mutex){1}; | ||
| }; |
There was a problem hiding this comment.
VALIDATION: FAIL - Not a faithful backport of Bitcoin PR bitcoin#27675
This change introduces mempool sequence tracking to the relay logic, which is not part of the original Bitcoin PR bitcoin#27675. The original Bitcoin commit only modified a test file (test/functional/p2p_filter.py) with 17 lines of changes, not core relay logic.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 309 to 312, remove the addition of
mempool sequence tracking variables and related relay logic changes, as these
were not part of the original Bitcoin PR #27675. Revert the code to match the
original Bitcoin commit, which only modified the test/functional/p2p_filter.py
file without altering core relay logic.
| CTransactionRef FindTxForGetData(const Peer::TxRelay& tx_relay, const uint256& txid) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, NetEventsInterface::g_msgproc_mutex); |
There was a problem hiding this comment.
Major API change not present in original Bitcoin PR
The FindTxForGetData method signature has been completely changed to use sequence-based relay instead of time-based relay. This is not part of Bitcoin PR bitcoin#27675 which only touched test files.
🤖 Prompt for AI Agents
In src/net_processing.cpp at lines 1002-1003, the method signature of
FindTxForGetData has been changed to use sequence-based relay instead of the
original time-based relay, which deviates from the original Bitcoin PR #27675.
To fix this, revert the method signature and implementation to match the
original PR by using time-based relay parameters and logic, ensuring consistency
with the upstream Bitcoin codebase.
| // Ensure we'll respond to GETDATA requests for anything we've just announced | ||
| LOCK(m_mempool.cs); | ||
| tx_relay->m_last_inv_sequence = m_mempool.GetSequence(); | ||
| } |
There was a problem hiding this comment.
Sequence number tracking after inventory announcements
Updates m_last_inv_sequence after sending transaction inventories. This sequence-based approach is completely different from the bloom filter mechanism it replaces and is not part of the original Bitcoin PR bitcoin#27675.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 6136 to 6139, the code updates
m_last_inv_sequence after sending transaction inventories, which uses a
sequence-based approach different from the previous bloom filter mechanism and
is not part of the original Bitcoin PR #27675. Review this change to ensure it
aligns with the intended design; if the sequence tracking is required, confirm
it is correctly synchronized with m_mempool and properly documented to clarify
its purpose and difference from the bloom filter method.
| TxMempoolInfo CTxMemPool::info_for_relay(const uint256& txid, uint64_t last_sequence) const | ||
| { | ||
| LOCK(cs); | ||
| indexed_transaction_set::const_iterator i = mapTx.find(txid); | ||
| if (i != mapTx.end() && i->GetSequence() < last_sequence) { | ||
| return GetInfo(i); | ||
| } else { | ||
| return TxMempoolInfo(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Additional scope creep in new method implementation.
The info_for_relay method represents new functionality for sequence-based relay filtering, which is not consistent with a simple bloom filter removal as described in the original Bitcoin PR bitcoin#27675.
This method implements entirely new relay logic rather than removing existing bloom filter code. Combined with the constructor changes, this confirms the PR is a complete reimplementation rather than a faithful backport.
The original Bitcoin commit should be reviewed to determine if these changes align with the intended scope, or if this PR needs to be rewritten to match the actual Bitcoin changes.
🤖 Prompt for AI Agents
In src/txmempool.cpp around lines 1331 to 1340, the info_for_relay method
introduces new sequence-based relay filtering logic that deviates from the
original Bitcoin PR #27675's scope, which focused on removing bloom filter code.
Review the original Bitcoin commit to verify the intended changes and adjust
this method accordingly to either remove bloom filter logic as originally
intended or rewrite this PR to align with the actual Bitcoin changes, avoiding
adding unrelated new relay logic.
| def test_reorg_relay(self): | ||
| self.log.info("Test that transactions from disconnected blocks are available for relay immediately") | ||
| # Prevent time from moving forward | ||
| self.nodes[1].setmocktime(int(time.time())) | ||
| self.connect_nodes(0, 1) | ||
| self.generate(self.wallet, 3) | ||
|
|
||
| # Disconnect node0 and node1 to create different chains. | ||
| self.disconnect_nodes(0, 1) | ||
| # Connect a peer to node1, which doesn't have immediate tx relay | ||
| peer1 = self.nodes[1].add_p2p_connection(P2PTxInvStore()) | ||
|
|
||
| # Create a transaction that is included in a block. | ||
| tx_disconnected = self.wallet.send_self_transfer(from_node=self.nodes[1]) | ||
| self.generate(self.nodes[1], 1, sync_fun=self.no_op) | ||
|
|
||
| # Create a transaction and submit it to node1's mempool. | ||
| tx_before_reorg = self.wallet.send_self_transfer(from_node=self.nodes[1]) | ||
|
|
||
| # Create a child of that transaction and submit it to node1's mempool. | ||
| tx_child = self.wallet.send_self_transfer(utxo_to_spend=tx_disconnected["new_utxo"], from_node=self.nodes[1]) | ||
| assert_equal(self.nodes[1].getmempoolentry(tx_child["txid"])["ancestorcount"], 1) | ||
| assert_equal(len(peer1.get_invs()), 0) | ||
|
|
||
| # node0 has a longer chain in which tx_disconnected was not confirmed. | ||
| self.generate(self.nodes[0], 3, sync_fun=self.no_op) | ||
|
|
||
| # Reconnect the nodes and sync chains. node0's chain should win. | ||
| self.connect_nodes(0, 1) | ||
| self.sync_blocks() | ||
|
|
||
| # Child now has an ancestor from the disconnected block | ||
| assert_equal(self.nodes[1].getmempoolentry(tx_child["txid"])["ancestorcount"], 2) | ||
| assert_equal(self.nodes[1].getmempoolentry(tx_before_reorg["txid"])["ancestorcount"], 1) | ||
|
|
||
| # peer1 should not have received an inv for any of the transactions during this time, as not | ||
| # enough time has elapsed for those transactions to be announced. Likewise, it cannot | ||
| # request very recent, unanounced transactions. | ||
| assert_equal(len(peer1.get_invs()), 0) | ||
| # It's too early to request these two transactions | ||
| requests_too_recent = msg_getdata([CInv(t=MSG_TX, h=int(tx["txid"], 16)) for tx in [tx_before_reorg, tx_child]]) | ||
| peer1.send_and_ping(requests_too_recent) | ||
| for _ in range(len(requests_too_recent.inv)): | ||
| peer1.sync_with_ping() | ||
| with p2p_lock: | ||
| assert "tx" not in peer1.last_message | ||
| assert "notfound" in peer1.last_message | ||
|
|
||
| # Request the tx from the disconnected block | ||
| request_disconnected_tx = msg_getdata([CInv(t=MSG_TX, h=int(tx_disconnected["txid"], 16))]) | ||
| peer1.send_and_ping(request_disconnected_tx) | ||
|
|
||
| # The tx from the disconnected block was never announced, and it entered the mempool later | ||
| # than the transactions that are too recent. | ||
| assert_equal(len(peer1.get_invs()), 0) | ||
| with p2p_lock: | ||
| # However, the node will answer requests for the tx from the recently-disconnected block. | ||
| assert_equal(peer1.last_message["tx"].tx.sha256, int(tx_disconnected["txid"], 16)) | ||
|
|
||
| self.nodes[1].setmocktime(int(time.time()) + 30) | ||
| peer1.sync_with_ping() | ||
| # the transactions are now announced | ||
| assert_equal(len(peer1.get_invs()), 3) | ||
| for _ in range(3): | ||
| # make sure all tx requests have been responded to | ||
| peer1.sync_with_ping() | ||
| last_tx_received = peer1.last_message["tx"] | ||
|
|
||
| tx_after_reorg = self.wallet.send_self_transfer(from_node=self.nodes[1]) | ||
| request_after_reorg = msg_getdata([CInv(t=MSG_TX, h=int(tx_after_reorg["txid"], 16))]) | ||
| assert tx_after_reorg["txid"] in self.nodes[1].getrawmempool() | ||
| peer1.send_and_ping(request_after_reorg) | ||
| with p2p_lock: | ||
| assert_equal(peer1.last_message["tx"], last_tx_received) | ||
|
|
There was a problem hiding this comment.
File operation mismatch confirmed
This test is being added to mempool_reorg.py, but Bitcoin PR bitcoin#27675 only modifies test/functional/p2p_filter.py. This test appears to validate the new mempool sequence number functionality which is not part of the original Bitcoin commit.
The test itself has implementation issues if this functionality were to be kept:
- Line 40:
time.time()should useself.mocktimefor consistency - Line 95: Magic number 30 should be a named constant
- The test validates sequence-based relay logic that doesn't exist in Bitcoin PR p2p: Drop m_recently_announced_invs bloom filter bitcoin/bitcoin#27675
🧰 Tools
🪛 Ruff (0.12.2)
36-36: Missing return type annotation for public function test_reorg_relay
Add return type annotation: None
(ANN201)
🤖 Prompt for AI Agents
In test/functional/mempool_reorg.py from lines 36 to 110, replace the use of
time.time() at line 40 with self.mocktime to maintain consistent mock time
usage. At line 95, replace the magic number 30 with a descriptive named constant
to improve code clarity. Additionally, since the sequence-based relay logic
tested here is not part of Bitcoin PR #27675, either remove this test or
refactor it to align with the actual functionality introduced in that PR.
❌ Backport Verification - CATASTROPHIC FAILUREOriginal Bitcoin commit: CRITICAL VIOLATIONS DETECTED: 🚨 Scope Explosion Violation
🚨 File Operation Mismatch
🚨 Intent ViolationWhat Bitcoin Did: Modified a single test file to extend MEMPOOL msg functional test coverage for bloom filter removal
🚨 CI Status: CATASTROPHIC
🚨 Reviewer Feedback AnalysisPastaPastaPasta comments reviewed: 0 🚨 Auto-Reject Conditions Met
Required Action: COMPLETE REWRITEThis PR violates EVERY fundamental principle of faithful backporting:
Example of correct scope: Only file that should change:
test/functional/p2p_filter.py | 17 +++++++++++++----This PR represents a fundamental misunderstanding of Bitcoin commit Status: REJECTED - Requires complete rewrite 🚫 This PR has been automatically closed due to catastrophic validation failures. Please create a new PR with a proper backport implementation. |
|
Automatically closed due to catastrophic validation failures. Please see the detailed analysis above and create a new PR with a proper backport implementation. |
Summary
This PR backports Bitcoin Core PR bitcoin#27675 which replaces the
m_recently_announced_invsbloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer.Changes
Bitcoin PR Description
The philosophy here is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of
m_recently_announced_invs,m_last_mempool_reqandUNCONDITIONAL_RELAY_DELAYand associated logic.Test plan
[Checklist for testing the pull request]
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor