Merge bitcoin/bitcoin#29619: refactor: consolidate MempoolAcceptResult processing#1118
Merge bitcoin/bitcoin#29619: refactor: consolidate MempoolAcceptResult processing#1118DashCoreAutoGuix wants to merge 1 commit intobackport-0.28-batch-740from
Conversation
…sing 07cd510 [refactor] consolidate invalid MempoolAcceptResult processing (glozow) 9353aa4 [refactor] consolidate valid MempoolAcceptResult processing (glozow) Pull request description: Every time we try to `ProcessTransaction` (i.e. submit a tx to mempool), we use the result to update a few net processing data structures. For example, after a failure, the {wtxid, txid, both, neither} (depending on reason) should be cached in `m_recent_rejects` so we don't try to download/validate it again. There are 2 current places and at least 1 future place where we need to process `MempoolAcceptResult`: - In the `ProcessMessage` logic after receiving and validating a tx - In `ProcessOrphanTx` where we retry orphans - With bitcoin#28970, after processing a package of transactions, we should do these updates for each tx in the package. Consolidate this code so it isn't repeated in 2 places and so we can reuse it in a future PR. ACKs for top commit: instagibbs: ACK 07cd510 achow101: ACK 07cd510 dergoegge: Code review ACK 07cd510 TheCharlatan: ACK 07cd510 Tree-SHA512: c4e74cb65e4f52882fca52e6682efa5dcf1562d98418454e09be256ffd026caae642a90aa5b9cccaae214be240d6f4be9d87b516953b2ee69a655f16ea569ed9
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@DashCoreAutoGuix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
✨ 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. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bugbot free trial expires on August 22, 2025
Learn more in the Cursor dashboard.
| state.GetResult() != TxValidationResult::TX_UNKNOWN && | ||
| state.GetResult() != TxValidationResult::TX_NO_MEMPOOL && | ||
| state.GetResult() != TxValidationResult::TX_RESULT_UNSET)) { | ||
| ProcessInvalidTx(from_peer, porphanTx, state, /*maybe_add_extra_compact_tx=*/false); |
There was a problem hiding this comment.
Bug: Mutex Assertion Failures in Transaction Processing
ProcessValidTx and ProcessInvalidTx assert that g_msgproc_mutex is held. However, ProcessOrphanTx calls these functions, and ProcessOrphanTx is invoked from BlockConnected without g_msgproc_mutex held, leading to assertion failures and potential data races. Additionally, Assume(...) is incorrectly used within a boolean expression in ProcessOrphanTx, causing a compile error.
Additional Locations (2)
| state.ToString()); | ||
| MaybePunishNodeForTx(pfrom.GetId(), state); | ||
| m_llmq_ctx->isman->TransactionRemovedFromMempool(ptx); | ||
| ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true); |
There was a problem hiding this comment.
Bug: Orphan Transaction Handling Regression
The commit introduces a compilation error in ProcessOrphanTx by using Assume(...) as a boolean condition. Furthermore, policy-rejected orphan transactions (not missing inputs and not invalid) are no longer added to m_recent_rejects or removed from the orphanage, nor are they added to vExtraTxnForCompact. This is because the new ProcessInvalidTx function, which handles these actions, is only called for state.IsInvalid() results. This omission leads to repeated reconsideration of these orphans, potential orphanage memory growth, and repeated re-requests from peers.
Additional Locations (1)
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Critical Issues Found:🚨 Severe Under-Implementation (11.4% of Bitcoin changes)
🚨 Missing Critical Files (10 of 11 files missing)Missing from your PR:
🚨 Functional Bugs in ImplementationBot-detected issues:
🚨 Incomplete Bitcoin PR ImplementationBitcoin PR bitcoin#29619 had 9 commits, your PR only implements 1:
Reviewer Feedback Analysis:PastaPastaPasta comments reviewed: 0 Why This Fails Verification:
Recommended Actions:
This PR represents a fundamental under-implementation of Bitcoin commit Status: FAILED - Requires substantial additional implementation to match Bitcoin's scope |
DashCoreAutoGuix
added
the
needs-fixes
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: 🚨 Critical Validation FailuresSize Validation Failure
Missing Files from Bitcoin Commit10 missing files that are part of the original Bitcoin changes:
CI Status: Critical
Reviewer Feedback AnalysisHuman reviewer comments: 2 critical bug reports identified
🚨 What Went WrongThis backport represents a severe under-implementation of Bitcoin commit What Bitcoin Did:
What Your PR Does:
Required Action: Complete Backport ReworkThis PR is missing 90% of the Bitcoin commit. The current implementation:
Correct approach:
This is not a simple refactor - it's a significant architectural change that requires implementing the complete Bitcoin infrastructure. Status: FAILED - Requires complete backport rework |
❌ Backport Verification - CATASTROPHIC FAILUREOriginal Bitcoin commit: CRITICAL VIOLATIONS DETECTED: 🚨 Scope Explosion Violation
🚨 File Operation Mismatch
🚨 Intent ViolationWhat Bitcoin Did: Comprehensive refactor to consolidate MempoolAcceptResult processing across multiple components with new helper functions ProcessValidTx/ProcessInvalidTx, orphanage integration, package handling infrastructure, and comprehensive test coverage
🚨 CI Status: CRITICAL
🚨 Reviewer Feedback Analysiscursor[bot] comments reviewed: 2 critical violations identified
ALL comments indicate fundamental implementation bugs - this is not a faithful Bitcoin backport. 🚨 Auto-Reject Conditions Met
Required Action: COMPLETE REWRITEThis PR violates EVERY fundamental principle of faithful backporting:
Example of correct scope: # Should include ALL these files:
+ src/policy/packages.cpp (new package handling)
+ src/policy/packages.h (new package headers)
+ src/test/fuzz/txorphan.cpp (improved fuzzing)
+ src/test/orphanage_tests.cpp (expanded tests)
+ src/test/txpackage_tests.cpp (new package tests)
+ src/txorphanage.cpp (orphanage improvements)
+ src/txorphanage.h (orphanage headers)
+ test/functional/p2p_1p1c_network.py (new functional test)
+ test/functional/p2p_opportunistic_1p1c.py (new functional test)
+ test/functional/test_runner.py (test runner updates)
+ src/net_processing.cpp (comprehensive refactor)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. |
2 participants
Backports bitcoin#29619
Original commit: 264ca9d
Backported from Bitcoin Core v0.28