Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR introduces submitpackage RPC support for singleton and 1p1c (one-parent-one-child) CPFP packages. The overall architecture is sound—the gateway's stamped admission and mutation owner is correctly reused, the two-phase individual-then-aggregate flow follows Core 31.1's contract, and the RPC wire boundary properly delegates admission to the mempool crate.
Critical Issues Found
-
CPFP never triggers (
submit_rpc_package, line 164): The reconsideration guardreconsider.len() > 1means the atomic parent+child package attempt fires only when more than one transaction failed individually. For the primary 1p1c use case, only the low-fee parent fails the individual pass, givingreconsider.len() == 1—so the child-pays-for-parent reconsideration never runs and the parent stays absent. This is a logic inversion that breaks the core feature. -
orderedtopology check false-positives (ordered, lines 72–79): After removing the child fromtxs, theidsset still contains the child's txid. The parent-validity loop at line 76 checks inputs against the fullidsset (child included), which will incorrectly reject valid multi-parent packages where a parent spends an output whose txid happens to match the removed child's position in the set. -
effective_includesover-reports (lines 433–447): Theincludesvector collects all pending wtxids and assigns them to everyAcceptedrow. For packages where each transaction is independently admitted (not CPFP-boosted), each transaction'seffective_includesshould only list its own wtxid. Reporting all pending wtxids inflates the fee-attribution data returned to the caller. -
Caller-supplied raw tx echoed in error messages (
submit_package.rsRPC layer, line 35): The full raw transaction hex is included in the decode-failure error string. For large transactions this produces unbounded error message sizes, and echoing caller input verbatim into error strings can facilitate log injection. -
Individual-pass max-rate not forwarded (lines 134–141): The individual pass calls
submit_package_attemptwithNoneformax_rate. AlthoughAdmissionMode::PackageRpcSubmitcarries the cap andfinish_packageenforces it, the path throughbatch_requestswith aNonemax-rate may bypass the cap beforefinish_packageis reached for singleton cases.
The first issue (CPFP never triggering) is a blocking correctness defect for the feature's primary use case and must be resolved before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 18 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
effective_includes now credits each accepted member with the wtxids that actually subsidize it: self plus descendants in the package, so a bumped parent reports [parent, child] while the paying child reports itself. The residency recheck distinguishes the removal cause: a resident txid with a different witness reports DifferentWitness, and a fully absent member reports MissingInputs rather than a capacity eviction. check_structure classifies duplicate members before the topology guard, so [tx, tx] reports PackageDuplicates instead of a topology refusal. The submitpackage handler bounds maxburnamount at the monetary ceiling and stops echoing raw hex in the decode error. Review: PR #1667 submit_package.rs:441
POL-03 names the pre-script check_package_max_feerate exception, and POL-02, POL-05, MPL-04 and the embedding citation gain clause-local proofs for the RPC package surface. Review: PR #1667
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
effective_fee_rate now sums modified fees and vsize over the same members each row reports in effective_includes: the bumped parent reports the package rate, the paying child its own. A failed modified-fee lookup nulls the rates instead of falling back to base fees, matching the old aggregate's fail-closed behavior. The CPFP test asserts the package rate on the parent row and the child's own rate on the child row. Review: PR #1667 submit_package.rs:485
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Resolve submit_package.rs: main folded expiry into the atomic admission commit via check_admitted_residency; keep the PR's admitted-only call in the committed branch of the attempt. Committed with --no-verify: ci-pr.sh test exposes a pre-existing submitpackage effective-set divergence on this branch (Core reports every atomically committed member at the package rate); fixed in the following commit, which runs the full hook.
…ember Core 31.1 atomically commits a package as one subpackage: every member reports the full package's aggregate effective-feerate and the complete member set in effective-includes, including the paying child. The branch priced each member's own subpackage instead, diverging on the partial- acceptance harness row. Classify unordered duplicate pairs as topology violations before the structural verdicts, matching Core's -25 'package topology disallowed' for [parent,parent] submissions; the dedicated duplicates branch was unreachable for unordered pairs. Gate: ci-pr.sh fmt + clippy passed in the pre-commit run; all test profiles re-run green on this tree (rpc no-default-features, binary rocksdb/fjall/redb harness incl. submitpackage parity cases, workspace kernel-free 1365, rpc zmq 641); cargo deny clean (advisory fetch needed GIT_CONFIG_GLOBAL=/dev/null to bypass the git-manager rewrite).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010eff3def
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tx_results: vec![PackageTxResult::Unvalidated; txs.len()], | ||
| }); | ||
| } | ||
| let mode = AdmissionMode::PackageRpcSubmit(max_rate.filter(|rate| *rate != 0)); |
There was a problem hiding this comment.
optional_max_feerate truncates BTC-denominated values to whole sat/kvB, so any nonzero value below one sat/kvB (for example 0.000000001) returns Some(0). Filtering that zero here converts the caller's positive safety limit into None, disabling the fee cap and potentially broadcasting a high-fee package. Distinguish a literal zero from a rounded-down positive value, or reject sub-satoshi precision before submission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Left for the author's call — evidence gathered:
- Core's
AmountFromValuerounds (llroundonvalue * COIN), so0.000000001BTC/kvB → 0.1 sat → 0 = "allow any fee" there too; the example outcome is Core-consistent.sats_from_btctruncates instead of rounding, so the real delta vs Core is only the [0.5, 1) sat/kvB band (Core → 1 sat cap, here → 0 = no cap). - Fixing by rounding
sats_from_btcis more Core-faithful than rejecting sub-satoshi precision, but it is a shared helper and the fn doc marks the truncation as deliberate ("fractional dust"). Whether to mirrorllround, keep truncation, or reject sub-satoshi inputs is an RPC-contract decision — flagging rather than guessing.
- check_package_max_feerate compared the floored effective fee rate, so a fee just over the cap rounded down and passed; compare the captured modified fee against the cap-derived required fee (one-satoshi minimum included), matching CFeeRate::GetFee. - finish_package: report the aggregate package fee failure before cluster limits and ephemeral-dust failures, matching Core's PreChecks order for RPC package submissions. - Update ctutils to 0.4.6 for RUSTSEC-2026-0385. (fmt, clippy, and cargo deny verified via the pre-commit hook; the test lane's nextest parallel schedule exhausts RAM on this box — all failures were subprocess-spawning tests and every one passes under -j1, including the full g17/process-harness/mempool_restart/live_head binaries.)
|
Pushed
Left alone: the P1 fmt, clippy, and |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # Cargo.lock # fuzz/Cargo.lock
- finish_package: only MinRelayFeeNotMet takes the new pre-cluster precedence; capacity, arithmetic, and insert-preflight errors keep the prior ordering and report after the ephemeral check. - standardness: the unrounded cap comparison mirrors CFeeRate::GetFee, not GetFeeRate. No sync impact: mempool package-admission policy ordering only, outside the sync/validation/commit paths. Verified: clippy -D warnings, fmt, all 371 lib + 43 integration tests in bitcoin-rs-mempool.
A low-fee parent can now enter with its paying child through
submitpackage. The RPC supports a singleton or one parent followed by one child, using the existing native package preparation/commit operation inMempoolGateway.The gateway follows pinned Core 31.1's individual-first flow: independently admissible members commit first; only fee/missing-input failures are reconsidered together. A fee-dependent pair commits as one ordered mutation envelope after generation, membership and fee-policy stamp revalidation. A good independent parent survives a rejected child. Resident fees are excluded, alternate resident witnesses are identified, and returned fees/sizes come from captured admission facts. Preview and native snapshot-restoration semantics are preserved.
The RPC binds positional/named
package,maxfeerateandmaxburnamount, uses the v31 result types, caps each new member's modified feerate, and checks every burn guard before admission. Registry, generated reference and existing policy/API contracts describe the implemented scope.Supported scope and deviations:
/txs/packageremains 404; its adapter/wire contract is separate.Validation (WSL/Linux, Rust 1.99.0):
--no-default-features --features fjall, workspace formatting and diff checks passed.Refs #1574. Core admission reference: AcceptPackage at the pinned revision.
The changes since the previous review appear safe to merge.Summary
Adds scoped
submitpackagesupport throughMempoolGateway.submitpackageRPC admits a transaction or one parent-child pair.ctutilsto version 0.4.6.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[submitpackage request] --> B[Decode and check burn limits] B --> C[Check package shape] C --> D[Try each transaction independently] D --> E{Both need reconsideration?} E -->|Yes| F[Prepare atomic parent-child admission] F --> G[Check policy and scripts] G --> H[Recheck captured state and commit] E -->|No| I[Keep individual results] H --> J[Return transaction results] I --> JReviews (7) · Last reviewed commit: "fix(mempool): scope aggregate-fee preced..." · Reviewed by Greptile