Skip to content

CHIA-3957 Improve requesting transactions advertised via NewTransaction#20667

Open
AmineKhaldi wants to merge 1 commit intoChia-Network:mainfrom
AmineKhaldi:improve_new_transaction_handling
Open

CHIA-3957 Improve requesting transactions advertised via NewTransaction#20667
AmineKhaldi wants to merge 1 commit intoChia-Network:mainfrom
AmineKhaldi:improve_new_transaction_handling

Conversation

@AmineKhaldi
Copy link
Contributor

@AmineKhaldi AmineKhaldi commented Mar 16, 2026

Note

Medium Risk
Changes the transaction-fetch path to use synchronous request_transaction RPC responses and enqueue results into the per-peer TransactionQueue, which could affect mempool propagation and retry behavior under load. Test-only adjustments to expected_mempool_responses reduce flakiness but also change assumptions about connection counters.

Overview
Improves how a full node fetches transactions it learns about via peer advertisements by switching from fire-and-forget request_transaction + sleep polling to an awaited call_api(FullNodeAPI.request_transaction) flow, then enqueuing the received spend into the TransactionQueue (with fallback to alternate peers if a peer queue is full).

Adjusts mempool response expectation accounting by resetting expected_mempool_responses (including the old-peer mempool sync case) instead of incrementing it, and updates several tests to match this new semantics.

Written by Cursor Bugbot for commit 9a0dcd5. This will update automatically on new commits. Configure here.

@AmineKhaldi AmineKhaldi self-assigned this Mar 16, 2026
@AmineKhaldi AmineKhaldi added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Mar 16, 2026
@arvidn
Copy link
Contributor

arvidn commented Mar 17, 2026

I don't understand how this can be backwards compatible. When you send a RequestMempoolTransactions to a peer, you will receive an unknown number of RespondTransaction messages in response. It looks like this change would just ban the peer.

Am I misunderstanding something?

@AmineKhaldi
Copy link
Contributor Author

I don't understand how this can be backwards compatible. When you send a RequestMempoolTransactions to a peer, you will receive an unknown number of RespondTransaction messages in response. It looks like this change would just ban the peer.

Am I misunderstanding something?

This doesn't ban the peer, the connection stays and the message gets ignored.

@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch 2 times, most recently from a7b1e69 to bd8f1f2 Compare March 19, 2026 21:53
@AmineKhaldi AmineKhaldi marked this pull request as ready for review March 19, 2026 21:54
@AmineKhaldi AmineKhaldi requested a review from a team as a code owner March 19, 2026 21:54
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from bd8f1f2 to 112ba3d Compare March 19, 2026 22:06
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch 2 times, most recently from 9ba3eb1 to 411ade2 Compare March 20, 2026 09:46
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from 411ade2 to 813d96c Compare March 20, 2026 10:03
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch 3 times, most recently from 5a053d1 to 7195d65 Compare March 20, 2026 12:51
random_peer.peer_node_id,
)
except TransactionQueueFull:
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continue doesn't seem right here. If the queue is full, I would think it's very likely to be full after we request the transaction from another peer as well. Don't we have a queue or some kind of pacing that's meant to make sure we don't request more transactions when our processing queue is full?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is per peer, it's full for this peer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the exception would come from here, wouldn't it?
full_node.transaction_queue.put()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I mean by per peer.

if self._peers_transactions_queues[peer_id].priority_queue.qsize() >= self.peer_size_limit:
            self.log.warning(f"Transaction queue full for peer {peer_id}")
            raise TransactionQueueFull(f"Transaction queue full for peer {peer_id}")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this code anywhere in this patch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how full_node.transaction_queue.put() raises the exception in question.

@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from 7195d65 to c2c8b03 Compare March 20, 2026 14:06
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from c2c8b03 to 151d5af Compare March 20, 2026 14:46
Comment on lines 332 to +334
if spend_name in self.full_node.full_node_store.pending_tx_request:
self.full_node.full_node_store.pending_tx_request.pop(spend_name)
else:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove full_node.full_node_store.pending_tx_request entirely now. right? As you pointed out, we always clear the request in the finally block in tx_request_and_timeout(), so it doesn't really mean much here. The request is quite unlikely to be found in this set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done.

if transaction_id in full_node.full_node_store.pending_tx_request:
full_node.full_node_store.pending_tx_request.pop(transaction_id)
if task_id in full_node.full_node_store.tx_fetch_tasks:
full_node.full_node_store.tx_fetch_tasks.pop(task_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this change of using call_api(), all of these should probably be reviewed. It's not obvious that we need them anymore. especially pending_tx_request, I would expect to not be necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need pending_tx_request for new_transaction.

@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from 151d5af to 0f5e5e2 Compare March 20, 2026 16:48
@AmineKhaldi AmineKhaldi requested a review from arvidn March 20, 2026 16:52
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from 0f5e5e2 to 4fc03ba Compare March 20, 2026 18:27
@coveralls-official
Copy link

coveralls-official bot commented Mar 20, 2026

Pull Request Test Coverage Report for Build 23496106266

Details

  • 17 of 21 (80.95%) changed or added relevant lines in 5 files are covered.
  • 266 unchanged lines in 27 files lost coverage.
  • Overall coverage decreased (-0.2%) to 91.136%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/full_node/full_node_api.py 10 14 71.43%
Files with Coverage Reduction New Missed Lines %
chia/cmds/plotnft_funcs.py 1 85.42%
chia/full_node/block_store.py 1 96.88%
chia/rpc/rpc_server.py 1 90.94%
chia/server/chia_policy.py 1 94.52%
chia/_tests/core/mempool/test_mempool_manager.py 1 99.82%
chia/_tests/rpc/test_rpc_server.py 1 99.42%
chia/_tests/simulation/test_simulation.py 1 96.49%
chia/wallet/wallet_state_manager.py 1 94.46%
chia/wallet/wallet_user_store.py 1 98.18%
chia/data_layer/data_layer.py 2 85.78%
Totals Coverage Status
Change from base Build 23492166799: -0.2%
Covered Lines: 105839
Relevant Lines: 115964

💛 - Coveralls

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Mar 20, 2026
@AmineKhaldi AmineKhaldi force-pushed the improve_new_transaction_handling branch from 4fc03ba to 9a0dcd5 Compare March 24, 2026 14:55
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Mar 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants