backport: Merge bitcoin/bitcoin#29031#7388
Conversation
…ness 15f5a0d fuzz: Improve fuzzing stability for txorphan harness (dergoegge) Pull request description: The `txorphan` harness has low stability as eviction of orphan txs is entirely random at the moment. Fix this by passing the rng to `LimitOrphans`, which can be deterministic in tests. Also see bitcoin#29018. ACKs for top commit: maflcko: lgtm ACK 15f5a0d brunoerg: utACK 15f5a0d Tree-SHA512: 854ec34b3a0f16f26db6dc419096c6e7a380e8400119534aa278d6b1d54c253b572aa2fad13c383c796c431d8ff4263956e6f60326e99f8bf6abd16d9a280e97
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7deb6fb067
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // DoS prevention: do not allow m_orphans to grow unbounded (see CVE-2012-3789) | ||
| unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000; | ||
| m_orphanage.LimitOrphans(nMaxOrphanTxSize); | ||
| m_orphanage.LimitOrphans(nMaxOrphanTxSize, m_rng); |
There was a problem hiding this comment.
Add the missing RNG member before using it
This call now passes m_rng, but PeerManagerImpl does not declare any m_rng member or otherwise define that name (repo-wide search only finds this new use). As soon as net_processing.cpp is compiled, this path fails to build with an undeclared identifier; add the intended FastRandomContext member or pass an existing RNG.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| void TxOrphanage::LimitOrphans(unsigned int max_orphans_size) | ||
| void TxOrphanage::LimitOrphans(unsigned int max_orphans_size, FastRandomContext& rng) |
There was a problem hiding this comment.
Remove the shadowing RNG declaration
Adding the rng parameter here leaves the existing FastRandomContext rng; declaration later in the same function body, so txorphanage.cpp redeclares the parameter name and fails to compile. Remove the local declaration and use the injected RNG for the eviction loop.
Useful? React with 👍 / 👎.
Walkthrough
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/txorphanage.cpp (1)
119-149: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the stale local RNG declaration.
FastRandomContext rng;redeclares therngparameter in the same scope, so this won’t compile. It also drops the caller-supplied RNG needed by this path.Suggested fix
void TxOrphanage::LimitOrphans(unsigned int max_orphans_size, FastRandomContext& rng) { LOCK(m_mutex); @@ - FastRandomContext rng; while (!m_orphans.empty() && m_orphan_tx_size > max_orphans_size) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/txorphanage.cpp` around lines 119 - 149, In TxOrphanage::LimitOrphans, remove the stale local FastRandomContext declaration that shadows the rng parameter and prevents compilation. Keep using the caller-supplied rng argument for the random eviction path, and make sure the call to randrange in the orphan eviction loop uses that existing parameter rather than a new local variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/txorphanage.cpp`:
- Around line 119-149: In TxOrphanage::LimitOrphans, remove the stale local
FastRandomContext declaration that shadows the rng parameter and prevents
compilation. Keep using the caller-supplied rng argument for the random eviction
path, and make sure the call to randrange in the orphan eviction loop uses that
existing parameter rather than a new local variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7f9f4f53-6c87-41fa-92f4-66bf335723a1
📒 Files selected for processing (5)
src/net_processing.cppsrc/test/fuzz/txorphan.cppsrc/test/orphanage_tests.cppsrc/txorphanage.cppsrc/txorphanage.h
bitcoin back ports