backport: bitcoin#19988 - Overhaul transaction request logic#5943
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. |
|
🕓 Ready for review — 10 ahead in queue (commit 256fde9) |
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>
![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".
| const bool preferred = state->fPreferredDownload; | ||
| auto delay{0us}; | ||
| if (inv.IsMsgTx() && !preferred && m_nodeman == nullptr) delay += NONPREF_PEER_TX_DELAY; | ||
| const bool overloaded = m_object_request.CountInFlight(node.GetId()) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT; |
There was a problem hiding this comment.
When a peer sends a large INV batch before SendMessages() has issued any GETDATA, CountInFlight() is still 0 for every item in that batch, so none of the announcements get the overload delay. The next send loop will therefore request the whole batch at once (up to MAX_INV_SZ, and Dash keeps the per-peer announcement cap high for governance sync), whereas the previous code stopped at 100 in-flight objects. Include queued candidates in this threshold or cap the requestable set so one bulk announcement cannot immediately fan out tens of thousands of GETDATA requests to a single peer.
Useful? React with 👍 / 👎.
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
📒 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
| LOCK(cs_main); | ||
| const auto current_time{GetTime<std::chrono::microseconds>()}; | ||
| // Clear any COMPLETED leftovers first (they would suppress the new announcements), so the | ||
| // transaction is requested ASAP even if earlier requests for it failed. | ||
| m_object_request.ForgetTxHash(CInv(MSG_TX, txid)); | ||
| m_object_request.ForgetTxHash(CInv(MSG_DSTX, txid)); | ||
| for (PeerRef& peer : peersToAsk) { | ||
| LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__, | ||
| txid.ToString(), peer->m_id); | ||
|
|
||
| RequestObject(peer->m_id, inv, GetTime<std::chrono::microseconds>(), /*fForce=*/true); | ||
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip peers finalized before tracker registration.
The peer-map lock is released before cs_main is acquired, so FinalizeNode() can call DisconnectedPeer() in between. Registering that stale PeerRef afterward leaves a candidate for a nonexistent peer; if selected, it blocks live fallback peers indefinitely.
Proposed fix
for (PeerRef& peer : peersToAsk) {
+ if (State(peer->m_id) == nullptr) continue;
LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__,
txid.ToString(), peer->m_id);
m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOCK(cs_main); | |
| const auto current_time{GetTime<std::chrono::microseconds>()}; | |
| // Clear any COMPLETED leftovers first (they would suppress the new announcements), so the | |
| // transaction is requested ASAP even if earlier requests for it failed. | |
| m_object_request.ForgetTxHash(CInv(MSG_TX, txid)); | |
| m_object_request.ForgetTxHash(CInv(MSG_DSTX, txid)); | |
| for (PeerRef& peer : peersToAsk) { | |
| LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__, | |
| txid.ToString(), peer->m_id); | |
| RequestObject(peer->m_id, inv, GetTime<std::chrono::microseconds>(), /*fForce=*/true); | |
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); | |
| LOCK(cs_main); | |
| const auto current_time{GetTime<std::chrono::microseconds>()}; | |
| // Clear any COMPLETED leftovers first (they would suppress the new announcements), so the | |
| // transaction is requested ASAP even if earlier requests for it failed. | |
| m_object_request.ForgetTxHash(CInv(MSG_TX, txid)); | |
| m_object_request.ForgetTxHash(CInv(MSG_DSTX, txid)); | |
| for (PeerRef& peer : peersToAsk) { | |
| if (State(peer->m_id) == nullptr) continue; | |
| LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__, | |
| txid.ToString(), peer->m_id); | |
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); |
🤖 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/net_processing.cpp` around lines 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.
| assert self.nodes[1].getpeerinfo()[0]['inbound'] == False | ||
| timeout = 2 + NONPREF_PEER_TX_DELAY + GETDATA_TX_INTERVAL | ||
| self.log.info(f"Tx should be received at node 1 after {timeout} seconds of node time") | ||
|
|
||
| def tx_in_node1_mempool(): | ||
| self.bump_mocktime(1) | ||
| return tx['txid'] in self.nodes[1].getrawmempool() | ||
| self.wait_until(tx_in_node1_mempool) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the calculated node-time deadline.
wait_until() may call this predicate for 60 wall-clock seconds, advancing mocktime on every poll. The test can therefore pass after substantially more than timeout node seconds, hiding relay-delay regressions.
Proposed fix
- def tx_in_node1_mempool():
- self.bump_mocktime(1)
- return tx['txid'] in self.nodes[1].getrawmempool()
- self.wait_until(tx_in_node1_mempool)
+ self.bump_mocktime(timeout)
+ self.wait_until(lambda: tx['txid'] in self.nodes[1].getrawmempool())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert self.nodes[1].getpeerinfo()[0]['inbound'] == False | |
| timeout = 2 + NONPREF_PEER_TX_DELAY + GETDATA_TX_INTERVAL | |
| self.log.info(f"Tx should be received at node 1 after {timeout} seconds of node time") | |
| def tx_in_node1_mempool(): | |
| self.bump_mocktime(1) | |
| return tx['txid'] in self.nodes[1].getrawmempool() | |
| self.wait_until(tx_in_node1_mempool) | |
| assert self.nodes[1].getpeerinfo()[0]['inbound'] == False | |
| timeout = 2 + NONPREF_PEER_TX_DELAY + GETDATA_TX_INTERVAL | |
| self.log.info(f"Tx should be received at node 1 after {timeout} seconds of node time") | |
| self.bump_mocktime(timeout) | |
| self.wait_until(lambda: tx['txid'] in self.nodes[1].getrawmempool()) |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 100-100: comparison to False should be 'if cond is False:' or 'if not cond:'
(E712)
🪛 Ruff (0.15.21)
[error] 100-100: Avoid equality comparisons to False; use not self.nodes[1].getpeerinfo()[0]['inbound']: for false checks
Replace with not self.nodes[1].getpeerinfo()[0]['inbound']
(E712)
🤖 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 `@test/functional/p2p_tx_download.py` around lines 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.
| def test_spurious_notfound(self): | ||
| self.log.info('Check that spurious notfound is ignored') | ||
| self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(1, 1)])) | ||
|
|
||
| def test_oversized_notfound(self): | ||
| self.log.info('Check that oversized notfound increases misbehavior score') | ||
| oversized_notfound_count = MAX_NOTFOUND_SIZE + 1 | ||
| invs = [CInv(t=1, h=i) for i in range(oversized_notfound_count)] | ||
| with self.nodes[0].assert_debug_log(["Misbehaving", f"notfound message size = {oversized_notfound_count}"]): | ||
| self.nodes[0].p2ps[0].send_message(msg_notfound(vec=invs)) | ||
| self.nodes[0].p2ps[0].sync_with_ping() | ||
| self.restart_node(0) | ||
| peer = self.nodes[0].add_p2p_connection(TestP2PConn()) | ||
| peer.send_message(msg_notfound(vec=[CInv(MSG_TX, 1)])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize the spurious NOTFOUND before ending the test.
send_message() does not prove that dashd processed the message before the subsequent restart. Use a ping barrier so disconnects or processing failures are observable.
Proposed fix
- peer.send_message(msg_notfound(vec=[CInv(MSG_TX, 1)]))
+ peer.send_and_ping(msg_notfound(vec=[CInv(MSG_TX, 1)]))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_spurious_notfound(self): | |
| self.log.info('Check that spurious notfound is ignored') | |
| self.nodes[0].p2ps[0].send_message(msg_notfound(vec=[CInv(1, 1)])) | |
| def test_oversized_notfound(self): | |
| self.log.info('Check that oversized notfound increases misbehavior score') | |
| oversized_notfound_count = MAX_NOTFOUND_SIZE + 1 | |
| invs = [CInv(t=1, h=i) for i in range(oversized_notfound_count)] | |
| with self.nodes[0].assert_debug_log(["Misbehaving", f"notfound message size = {oversized_notfound_count}"]): | |
| self.nodes[0].p2ps[0].send_message(msg_notfound(vec=invs)) | |
| self.nodes[0].p2ps[0].sync_with_ping() | |
| self.restart_node(0) | |
| peer = self.nodes[0].add_p2p_connection(TestP2PConn()) | |
| peer.send_message(msg_notfound(vec=[CInv(MSG_TX, 1)])) | |
| def test_spurious_notfound(self): | |
| self.log.info('Check that spurious notfound is ignored') | |
| self.restart_node(0) | |
| peer = self.nodes[0].add_p2p_connection(TestP2PConn()) | |
| peer.send_and_ping(msg_notfound(vec=[CInv(MSG_TX, 1)])) |
🤖 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 `@test/functional/p2p_tx_download.py` around lines 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.
WalkthroughThis PR introduces Estimated code review effort: 5 (Critical) | ~120 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 |
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
| // Decide the TxRequestTracker parameters for this announcement: | ||
| // - "preferred": if fPreferredDownload is set (= outbound, or PF_NOBAN permission) | ||
| // - "reqtime": current time plus delays for: | ||
| // - NONPREF_PEER_TX_DELAY for announcements of transactions from non-preferred connections. | ||
| // Dash-specific object types are never delayed (they carry 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; | ||
| auto delay{0us}; | ||
| if (inv.IsMsgTx() && !preferred && m_nodeman == nullptr) delay += NONPREF_PEER_TX_DELAY; | ||
| const bool overloaded = m_object_request.CountInFlight(node.GetId()) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT; | ||
| if (overloaded) delay += OVERLOADED_PEER_OBJECT_DELAY; | ||
|
|
||
| m_object_request.ReceivedInv(node.GetId(), inv, preferred, current_time + delay); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve outbound/whitelisted priority for both transaction inventory types.
fPreferredDownload excludes peers without block-serving services, while IsMsgTx() lets inbound peers bypass the delay by announcing the same transaction as MSG_DSTX. Derive preferredness from connection/NoBan status and use IsGenTxMsg().
Proposed fix
- const bool preferred = state->fPreferredDownload;
+ const bool preferred =
+ !node.IsInboundConn() || node.HasPermission(NetPermissionFlags::NoBan);
auto delay{0us};
- if (inv.IsMsgTx() && !preferred && m_nodeman == nullptr) delay += NONPREF_PEER_TX_DELAY;
+ if (inv.IsGenTxMsg() && !preferred && m_nodeman == nullptr) {
+ delay += NONPREF_PEER_TX_DELAY;
+ }As per coding guidelines, preserve Dash-specific behavior when modifying or backporting Bitcoin Core C++ paths. <coding_guidelines>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Decide the TxRequestTracker parameters for this announcement: | |
| // - "preferred": if fPreferredDownload is set (= outbound, or PF_NOBAN permission) | |
| // - "reqtime": current time plus delays for: | |
| // - NONPREF_PEER_TX_DELAY for announcements of transactions from non-preferred connections. | |
| // Dash-specific object types are never delayed (they carry 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; | |
| auto delay{0us}; | |
| if (inv.IsMsgTx() && !preferred && m_nodeman == nullptr) delay += NONPREF_PEER_TX_DELAY; | |
| const bool overloaded = m_object_request.CountInFlight(node.GetId()) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT; | |
| if (overloaded) delay += OVERLOADED_PEER_OBJECT_DELAY; | |
| m_object_request.ReceivedInv(node.GetId(), inv, preferred, current_time + delay); | |
| // Decide the TxRequestTracker parameters for this announcement: | |
| // - "preferred": if fPreferredDownload is set (= outbound, or PF_NOBAN permission) | |
| // - "reqtime": current time plus delays for: | |
| // - NONPREF_PEER_TX_DELAY for announcements of transactions from non-preferred connections. | |
| // Dash-specific object types are never delayed (they carry 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 = | |
| !node.IsInboundConn() || node.HasPermission(NetPermissionFlags::NoBan); | |
| auto delay{0us}; | |
| if (inv.IsGenTxMsg() && !preferred && m_nodeman == nullptr) { | |
| delay += NONPREF_PEER_TX_DELAY; | |
| } | |
| const bool overloaded = m_object_request.CountInFlight(node.GetId()) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT; | |
| if (overloaded) delay += OVERLOADED_PEER_OBJECT_DELAY; | |
| m_object_request.ReceivedInv(node.GetId(), inv, preferred, current_time + delay); |
🤖 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/net_processing.cpp` around lines 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.
Source: Coding guidelines
| timeout = 2 + NONPREF_PEER_TX_DELAY + GETDATA_TX_INTERVAL | ||
| self.log.info("Tx should be received at node 1 after {} seconds of node time".format(timeout)) | ||
|
|
||
| def tx_in_node1_mempool(): | ||
| self.bump_mocktime(1) | ||
| return tx['txid'] in self.nodes[1].getrawmempool() | ||
| self.wait_until(tx_in_node1_mempool) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the computed mock-time deadline.
timeout is only logged; each poll advances mocktime without checking that bound. The test can pass after far more simulated time than advertised, missing relay-delay regressions. Fail after timeout mock-time increments or compare against an explicit mock-time deadline.
🤖 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 `@test/functional/p2p_tx_download.py` around lines 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.
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: