backport: bitcoin#19988, #20162, partial #20171 - Overhaul transaction request logic#5943
backport: bitcoin#19988, #20162, partial #20171 - Overhaul transaction request logic#5943knst wants to merge 3 commits into
Conversation
|
This pull request has conflicts, please rebase. |
❌ Backport Verification - Issues DetectedOriginal Bitcoin commit: Issues found:
The Bitcoin commit shows a clean 15-file change adding transaction request logic, but this PR has extensive modifications to Dash-specific modules (governance, LLMQ, spork) that weren't part of the original Bitcoin change. ❌ File Operation ViolationsCritical missing files:
Extra Dash-specific files not in Bitcoin:
Please address these issues:
Re-run verification after addressing these issues. |
|
⛔ Blockers found — Sonnet deferred (commit 1aed44a) |
![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: 257de3874b
ℹ️ 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".
WalkthroughIntroduces Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/net_processing.cpp`:
- Around line 2340-2350: Revalidate each PeerRef from peersToAsk after acquiring
cs_main and before calling m_object_request.ReceivedInv in the transaction
request flow. Skip peers that were finalized or disconnected by
FinalizeNode/DisconnectedPeer between peer-map collection and tracker
registration, so only currently registered peers become candidates.
In `@test/functional/p2p_tx_download.py`:
- Around line 199-203: Update test_spurious_notfound to synchronize processing
of the sent NOTFOUND message before the test proceeds. After peer.send_message,
use the TestP2PConn ping/barrier mechanism and wait for its completion, ensuring
message-processing or disconnect failures are surfaced before any subsequent
restart.
- Around line 100-107: Enforce the calculated node-time deadline in the
tx_in_node1_mempool polling flow: track elapsed mocktime or polling duration
against timeout and fail once timeout is exceeded, while retaining the existing
mempool check and mocktime advancement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8e324552-fc19-4984-8f3f-5aefa3d1eeee
📥 Commits
Reviewing files that changed from the base of the PR and between ca2912d and 257de3874bd2ba67a0b442902dbba93faf555ef9.
📒 Files selected for processing (16)
src/Makefile.amsrc/Makefile.test.includesrc/coinjoin/server.cppsrc/governance/net_governance.cppsrc/instantsend/net_instantsend.cppsrc/msg_result.hsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/protocol.hsrc/test/fuzz/txrequest.cppsrc/test/net_tests.cppsrc/test/txrequest_tests.cppsrc/txrequest.cppsrc/txrequest.htest/functional/p2p_tx_download.py
💤 Files with no reviewable changes (1)
- src/net.h
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PeerManagerImpl
participant TxRequestTracker
participant Peer
PeerManagerImpl->>TxRequestTracker: register inventory announcement
TxRequestTracker-->>PeerManagerImpl: return requestable inventory
PeerManagerImpl->>Peer: send GETDATA
PeerManagerImpl->>TxRequestTracker: mark request in flight
Peer-->>PeerManagerImpl: return object or NOTFOUND
PeerManagerImpl->>TxRequestTracker: complete or expire request
Possibly related PRs
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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/net_processing.cpp`:
- Around line 1547-1561: Update the TxRequestTracker preference and delay logic
around preferred and overloaded announcements: derive preferredness from the
connection’s outbound or NoBan status rather than state->fPreferredDownload, and
use IsGenTxMsg() for the transaction-delay condition so both transaction
inventory types receive the correct treatment. Preserve the existing
Dash-specific m_nodeman behavior and overload delay when calling ReceivedInv.
In `@test/functional/p2p_tx_download.py`:
- Around line 102-108: Update the tx_in_node1_mempool polling flow to enforce
the computed timeout rather than only logging it. Track the mock-time elapsed
from the start of the wait, or establish an explicit deadline, and make
wait_until fail once timeout is exceeded while preserving the existing mempool
success check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cb9f42f1-ff8c-4f41-8e6c-94b43a64487b
📒 Files selected for processing (16)
src/Makefile.amsrc/Makefile.test.includesrc/coinjoin/server.cppsrc/governance/net_governance.cppsrc/instantsend/net_instantsend.cppsrc/msg_result.hsrc/net.hsrc/net_processing.cppsrc/net_processing.hsrc/protocol.hsrc/test/fuzz/txrequest.cppsrc/test/net_tests.cppsrc/test/txrequest_tests.cppsrc/txrequest.cppsrc/txrequest.htest/functional/p2p_tx_download.py
💤 Files with no reviewable changes (1)
- src/net.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This is a backport of bitcoin#19988 (TxRequestTracker overhaul) generalized to Dash's full CInv-based object types. Both Codex and Sonnet specialists independently verified the functional Dash adaptations (delay policy, per-type expiry, consume-once ReceivedResponse wiring, retained MAX_PEER_OBJECT_ANNOUNCEMENTS cap, retained limitedmap for ChainLocks) are correct and no upstream backport prerequisites are missing. Independent git forensics confirm a real, verifiable commit-hygiene defect: the sole commit in this range is a single-parent commit dressed up as a genuine GitHub merge commit, squashing 9 upstream Pieter Wuille commits into one commit attributed solely to the backporter, which is anomalous compared to every other 'Merge #NNNN' commit sampled in this repository's history (all of which are real 2-parent merges).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— dash-core-commit-history (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— backport-reviewer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— backport-reviewer (failed),claude-sonnet-5— backport-reviewer (failed),claude-sonnet-5— backport-reviewer (completed)
🔴 1 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:256fde99c94>`:
- [BLOCKING] <commit:256fde99c94>:1: Single squashed commit masquerades as a genuine 9-commit merge, losing upstream authorship, bisectability, and message accuracy
Commit 256fde99c9424d40fc9729de5bd0e6fc51287dba is titled 'Merge #19988: Overhaul transaction request logic' and its body lists 9 upstream Bitcoin Core commits by SHA/author (all Pieter Wuille), formatted exactly like a genuine GitHub merge commit. But `git cat-file -p 256fde99c94` shows only one `parent` line (ca2912d9024836e825cdd3e8cc4db83926c3790f) — this is not an actual merge, and `git rev-list --count ca2912d..256fde9` confirms exactly 1 commit in the range. Sampling ~30 other 'Merge #NNNN' commits in this repository's history (ca2912d9024, 58cc1771eed, f2b15fbfe84, 912e64e8b8f, 94d00f70f65, etc.) shows every single one is a real 2-parent merge; this commit is the sole anomaly. The result: all 9 upstream commits plus the Dash-specific CInv-generalization adaptations (governance/InstantSend/ChainLock/spork/CoinJoin object-type support, ReceivedResponse consume-once semantics, MAX_PEER_OBJECT_ANNOUNCEMENTS retention, limitedmap retention, p2p_tx_download.py mocktime port) are bundled into one indivisible ~2367-line commit authored/committed entirely by the backporter, with `git blame` now attributing Pieter Wuille's networking-behavior rewrite to someone else and `git bisect` unable to isolate which of the 9 logical upstream steps or which Dash adaptation introduced a regression in a codepath Dash also uses for InstantSend locks, ChainLocks, governance votes, and quorum messages. This also explains why the copied upstream message is materially misleading on its own terms: it claims the announcement limit is reduced to 5000 and that 86f50ed10f66 deletes limitedmap, while the actual Dash diff retains the larger MAX_PEER_OBJECT_ANNOUNCEMENTS cap and keeps limitedmap for the ChainLock handler, with no adaptations section explaining either divergence or the CInv generalization. This directly contradicts this project's stated convention to keep backport commits close to 1:1 and preserve original Bitcoin Core authorship/messages per commit. Recommend re-splitting into the individual upstream commits (each with `cherry picked from commit <sha>` trailers and original Pieter Wuille authorship preserved) followed by a separate, clearly-labeled Dash-adaptation commit that documents the retained cap, retained limitedmap, and CInv generalization — consistent with how other backports in this repository are structured.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bde874e7b
ℹ️ 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".
| // time-sensitive consensus data), and neither is anything in masternode mode. | ||
| // - OVERLOADED_PEER_OBJECT_DELAY for announcements from peers which have at least | ||
| // MAX_PEER_OBJECT_REQUEST_IN_FLIGHT requests in flight. | ||
| const bool preferred = state->fPreferredDownload; |
There was a problem hiding this comment.
When a non-transaction inv such as MSG_QUORUM_CONTRIB is announced by both an inbound peer and an outbound peer before the next SendMessages(), this marks the outbound announcement as preferred, so TxRequestTracker will select it even though both candidates are immediately requestable. If that outbound peer withholds the object, fallback waits the per-type expiry (GetObjectInterval(), 60s for DKG objects) despite an inbound copy already being available, reintroducing an outbound-peer delay for time-sensitive Dash object relay. Limit the preferred-peer bias to the transaction types that need the Bitcoin censorship-resistance behavior.
AGENTS.md reference: AGENTS.md:L153-L161
Useful? React with 👍 / 👎.
knst
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07b8bc6558
ℹ️ 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".
| m_object_request.ForgetTxHash(CInv(MSG_TX, txid)); | ||
| m_object_request.ForgetTxHash(CInv(MSG_DSTX, txid)); |
There was a problem hiding this comment.
When AskPeersForTransaction() runs after an ISLOCK for a missing transaction, these calls erase every live MSG_TX/MSG_DSTX CANDIDATE or REQUESTED announcement, even though the loop below re-adds at most the first four peers from the inventory filters. If those selected peers withhold the transaction or disconnect, announcements from other peers that had already advertised the tx are gone, so the tracker has no fallback candidates until a reannouncement occurs and the locked tx can remain unfetched. Clear only completed leftovers, or re-add/preserve the existing live candidates before forcing the immediate retry.
AGENTS.md reference: AGENTS.md:L160-L161
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36c2887f7c
ℹ️ 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".
| // MAX_PEER_OBJECT_REQUEST_IN_FLIGHT requests in flight. | ||
| const bool preferred = state->fPreferredDownload; | ||
| auto delay{0us}; | ||
| if (inv.IsGenTxMsg() && !preferred && m_nodeman == nullptr) delay += NONPREF_PEER_TX_DELAY; |
There was a problem hiding this comment.
When a non-preferred inbound peer announces a MSG_DSTX, this condition now adds the 2-second transaction delay because IsGenTxMsg() includes both MSG_TX and MSG_DSTX. The pre-backport path only delayed inv.IsMsgTx(), so zero-fee CoinJoin/DSTX announcements that used to be requested immediately are now held back on regular nodes; keep the delay limited to MSG_TX to preserve the existing Dash-specific relay behavior.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The backport remains blocked by the previously reported history-fidelity issue and by an InstantSend retry regression that discards live fallback announcements outside the four peers re-registered for retry. The current head also broadens the non-preferred transaction delay to MSG_DSTX despite the PR's documented Dash adaptation policy, so that behavior should either be restored or explicitly justified and tested.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:8fb303d2f75>`:
- [BLOCKING] <commit:8fb303d2f75>:1: Single squashed commit masquerades as a genuine 9-commit merge, losing upstream authorship, bisectability, and message accuracy
Commit 8fb303d2f7520c978353a542bd615f693cd97406 still has only one parent and is the sole commit over ba143fe163a7dd37cd8222cb1fb61f12ac7f4410, but its `Merge #19988` message enumerates nine separate upstream commits authored by Pieter Wuille. The combined 16-file backport is authored by Konstantin Akimov, so the upstream authorship and logical boundaries needed for blame and bisect are not preserved. The copied message is also inaccurate for this tree: it says the announcement limit was reduced to 5000 while the code retains `max(5000, 2 * MAX_INV_SZ)`, which is 100000 here, and it lists deletion of limitedmap even though `src/limitedmap.h` remains in use by the ChainLock handler. Reconstruct the upstream commit series with its original authorship and messages, followed by clearly identified Dash adaptation commits.
In `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:2342-2355: Preserve fallback announcements when forcing an ISLock transaction retry
The comment says this clears completed leftovers, but `ForgetTxHash()` erases every candidate, requested, and completed announcement for both transaction inventory identities across all peers. `AskPeersForTransaction()` then re-registers only MSG_TX candidates for at most four peers. Any additional connected peers that had already advertised the transaction are removed from the tracker; if the selected four withhold, fail, or disconnect, no remaining peer can be selected unless it independently reannounces. The pre-backport force path left other peers' queued announcements intact. Add tracker support for resetting the stale state required by this retry while preserving live fallback candidates, or rebuild every preserved announcement before clearing the existing state.
- [SUGGESTION] src/net_processing.cpp:1550-1557: Backport adaptation regression: MSG_DSTX announcements now receive NONPREF_PEER_TX_DELAY
`CInv::IsGenTxMsg()` matches both MSG_TX and MSG_DSTX, so non-preferred inbound DSTX announcements now receive the two-second delay. The merge-base implementation and the previously reviewed head both used `IsMsgTx()`, and the PR description explicitly documents that only MSG_TX receives this delay to preserve existing Dash behavior. This also delays orphan-parent retrieval, which deliberately uses MSG_DSTX to retain possible CoinJoin metadata. Restore the documented MSG_TX-only condition, or explicitly revise the adaptation policy and add functional coverage for the intended DSTX timing.
fd9a006 Report and verify expirations (Pieter Wuille) 86f50ed Delete limitedmap as it is unused now (Pieter Wuille) cc16fff Make txid delay penalty also apply to fetches of orphan's parents (Pieter Wuille) 173a1d2 Expedite removal of tx requests that are no longer needed (Pieter Wuille) de11b0a Reduce MAX_PEER_TX_ANNOUNCEMENTS for non-PF_RELAY peers (Pieter Wuille) 242d164 Change transaction request logic to use txrequest (Pieter Wuille) 5b03121 Add txrequest fuzz tests (Pieter Wuille) 3c7fe0e Add txrequest unit tests (Pieter Wuille) da3b8fd Add txrequest module (Pieter Wuille) Pull request description: This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests). The major changes are: * Announcements from outbound (and whitelisted) peers are now always preferred over those from inbound peers. This used to be the case for the first request (by delaying the first request from inbound peers), and a bias afters. The 2s delay for requests from inbound peers still exists, but after that, if viable outbound peers remain for any given transaction, they will always be tried first. * No more hard cap of 100 in flight transactions per peer, as there is less need for it (memory usage is linear in the number of announcements, but independent from the number in flight, and CPU usage isn't affected by it). Furthermore, if only one peer announces a transaction, and it has over 100 in flight already, we still want to request it from them. The cap is replaced with a rule that announcements from such overloaded peers get an additional 2s delay (possibly combined with the existing 2s delays for inbound connections, and for txid peers when wtxid peers are available). * The limit of 100000 tracked announcements is reduced to 5000; this was excessive. This can be bypassed using the PF_RELAY permission (to accommodate locally dumping a batch of many transactions). This replaces bitcoin#19184, rebased on bitcoin#18044 and with many small changes. ACKs for top commit: ariard: Code Review ACK fd9a006. I've reviewed the new TxRequestTracker, its integration in net_processing, unit/functional/fuzzing test coverage. I looked more for soundness of new specification rather than functional consistency with old transaction request logic. MarcoFalke: Approach ACK fd9a006 🏹 naumenkogs: Code Review ACK fd9a006. I've reviewed everything, mostly to see how this stuff works at the lower level (less documentation-wise, more implementation-wise), and to try breaking it with unexpected sequences of events. jnewbery: utACK fd9a006 jonatack: WIP light ACK fd9a006 have read the code, verified that each commit is hygienic, e.g. debug build clean and tests green, and have been running a node on and off with this branch and grepping the net debug log. Am still unpacking the discussion hidden by GitHub by fetching it via the API and connecting the dots, storing notes and suggestions in a local branch; at this point none are blockers. ryanofsky: Light code review ACK fd9a006, looking at txrequest implementation, unit test implementation, and net_processing integration, just trying to understand how it works and looking for anything potentially confusing in the implementation. Didn't look at functional tests or catch up on review discussion. Just a sanity check review focused on: Tree-SHA512: ea7b52710371498b59d9c9cfb5230dd544fe9c6cb699e69178dea641646104f38a0b5ec7f5f0dbf1eb579b7ec25a31ea420593eff3b7556433daf92d4b0f0dd7 Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com>
…dd getter/setter c8abbc9 p2p: declare Announcement::m_state as uint8_t, add getter/setter (Jon Atack) Pull request description: Change `Announcement::m_state` in `tx_request.cpp` from type `State` to `uint8_t` and add a getter and setter for the conversion to/from `State`. This should silence these travis ci gcc compiler warnings: ``` txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is too small to hold all values of ‘enum class {anonymous}::State’ State m_state : 3; ^ ``` The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414. ACKs for top commit: sipa: utACK c8abbc9 ajtowns: ACK c8abbc9 -- quick code review hebasto: ACK c8abbc9, tested on Bionic (x86_64, gcc 7.5.0): Tree-SHA512: 026721dd7a78983a72da77638d3327d2b252bef804e489278a852f000046c028d6557bbd6c2b4cea391d4e01f9264a1be842d502047cb90b2997cc37bee59e61 Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Backport Note: Contains: bc4a230 Remove redundant p2p lock tacking for tx download functional tests New functional test coverage of tx download was added by bitcoin#19988, but `with p2p_lock` is redundant for some tests with `wait_until` test helper, already guaranteeing test lock tacking. Co-authored-by: Antoine Riard <ariard@student.42.fr>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The latest head fixes both previously reported runtime adaptation issues: InstantSend retries preserve fallback announcements, and the non-preferred delay is again restricted to MSG_TX. The bitcoin#19988 backport remains blocked because nine upstream commits are still collapsed into one single-parent, backporter-authored commit with misleading unadapted message content. The latest partial bitcoin#20171 commit also has a subject that claims a test addition although its patch only removes redundant locking.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— specialist (completed),gpt-5.6-sol— specialist (completed) - Verifier:
claude-sonnet-5— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:0c5f6b39ab3>`:
- [BLOCKING] <commit:0c5f6b39ab3>:1: Single squashed commit masquerades as a genuine 9-commit merge, losing upstream authorship, bisectability, and message accuracy
Commit 0c5f6b39ab39e79db86823b998574a90a1f1e8a1 has only parent ba143fe163a7dd37cd8222cb1fb61f12ac7f4410, and `git rev-list --count ba143fe163a..0c5f6b39ab3` returns 1. Its message nevertheless enumerates nine separate upstream commits authored by Pieter Wuille, while the combined 16-file, 2382-insertion/363-deletion change is recorded under Konstantin Akimov without the original authorship or logical commit boundaries. The copied description also does not describe the resulting Dash adaptation accurately: it claims the announcement limit was reduced to 5000, but `MAX_INV_SZ` is 50000 and `MAX_PEER_OBJECT_ANNOUNCEMENTS` is `max(5000, 2 * MAX_INV_SZ)`, yielding 100000; it also lists deletion of limitedmap even though `src/chainlock/handler.h` still uses it. Reconstruct the nine upstream commits with their original authorship, subjects, and provenance, then place the Dash-specific CInv generalization and retained-cap/limitedmap changes in clearly identified adaptation commits.
In `<commit:1aed44a45c2>`:
- [SUGGESTION] <commit:1aed44a45c2>:1: Partial #20171 commit subject claims a test addition but only removes redundant locking
Commit 1aed44a45c28ed37cb0ab10fe048dfd2de253794 is titled `partial Merge #20171: Add functional test test_txid_inv_delay`, but its complete patch is eight deletions from `test/functional/p2p_tx_download.py`. The body identifies the selected upstream change as `bc4a23008762702ffcd6868bcdb8fe2a732640ba Remove redundant p2p lock tacking for tx download functional tests` and states that #19988 already added the test coverage. Use a subject describing the change actually contained in this partial backport, while retaining the bitcoin#20171 and upstream-SHA provenance in the body.
3 participants
Issue being fixed or feature implemented
This replaces the transaction request logic with an encapsulated class that maintains all the state surrounding it. By keeping it stand alone, it can be easily tested (using included unit tests and fuzz tests).
What was done?
The major changes are (from original PR description):
Differences between Bitcoin Core and Dash Core
Bitcoin uses the inv/getdata machinery only for transactions;
Dash uses it for many object types (governance objects and votes, InstantSend locks, ChainLocks, sporks, coinjoin queues, quorum messages).
This backport therefore generalizes TxRequestTracker for CInv.
Announcementss of the same hash under two types (MSG_TX vs MSG_DSTX) are
tracked independently, and the priority function hashes the type too.
announcements from non-preferred peers, and not in masternode mode --
preserving current Dash behavior. Dash-specific object types are never
delayed at announcement time: they carry time-sensitive consensus data
(DKG and signing would stall otherwise).
(CLSIG 5s, ISDLOCK 10s, recovered sig 15s, default 60s). Under the
tracker, expiry doubles as the fallback-to-another-peer trigger, so
time-sensitive types fail over quickly.
follow-up commit reducing the cap to 5000 is deliberately skipped:
governance vote sync legitimately announces up to MAX_INV_SZ objects
from a single peer.
CANDIDATE/REQUESTED announcement. This implements the pre-existing
PeerConsumeObjectRequest consume-once semantics that governance uses to
reject unsolicited objects, without a separate non-atomic lookup.
inv. Transaction paths forget both MSG_TX and MSG_DSTX for a txid;
orphan-parent fetching announces MSG_DSTX only (a MSG_DSTX getdata is
answered with the DSTX wrapper when available and a plain TX otherwise,
so nothing is lost, and announcing both types would fetch twice).
deleted it entirely): Dash's chainlock handler still uses it.
the in-flight count is no longer hard-capped.
mocktime, so all delay waits use bump_mocktime instead of wall-clock
sleeps. test_large_inv_batch and test_oversized_notfound are not ported
(the Dash cap and NOTFOUND bound cannot be reached within a single
protocol message).
How Has This Been Tested?
Run unit/functional tests
Breaking Changes
N/A
Checklist: