Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remember recent transactions we've received and don't ask for them again #19287

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Feb 18, 2025

Purpose:

Avoid requesting the same mempool transactions multiple time, especially ones that have failed validation.

Current Behavior:

New Behavior:

Testing Notes:

@arvidn arvidn requested a review from a team as a code owner February 18, 2025 14:54
@arvidn arvidn requested a review from Rigidity February 18, 2025 15:12
@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Feb 18, 2025
Rigidity
Rigidity previously approved these changes Feb 18, 2025
# any invalid transactions we've seen recently, we don't need
# to see again, so add those to the filter as well

# first remove transactions that are older than 2 minutes
Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain the 2 minutes aspect of this here

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 was an arbitrary timeout. it might make sense to make it longer. It seemed unnecessary to keep old transactions around indefinitely, that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bumped it to 10 minutes

Rigidity
Rigidity previously approved these changes Feb 18, 2025
@arvidn
Copy link
Contributor Author

arvidn commented Feb 18, 2025

I can't reproduce the symptom that this is supposed to address. I'm having second thoughts about including this PR. I'm tempted to rebase it on top of main and possibly explore it further there.
This patch also appear to be somewhat overlapping with the existing seen_bundle_hashes.

@OverActiveBladderSystem
Copy link

OverActiveBladderSystem commented Feb 18, 2025

I can't reproduce the symptom that this is supposed to address. I'm having second thoughts about including this PR. I'm tempted to rebase it on top of main and possibly explore it further there. This patch also appear to be somewhat overlapping with the existing seen_bundle_hashes.

I looked at a 30 second log in debug mode from yesterday and pulled out the lines which had double spend transactions, then I looked for duplicates among those transactions and can show a brief sample of numerous duplicates of the same transaction.

While I'm not certain this is the sole cause of my heavy CPU utilization, it shows reprocessing of data sometimes 15 seconds apart from the first time it was done, the double spends seemed to be present most during my high CPU utilization periods.

Note, I set MEMPOOL_BLOCK_BUFFER: 1 in the config yaml during this particular log period in case that has any impact.

Windows 10
Only Full_Node process running in CLI mode (farmer/harvester on a different machine)

processing-duplicate-transactions-results.txt

adding the unedited 30 second log below just in case you want to see other things within it for those timestamps
time-204640-to-204710.txt

@arvidn
Copy link
Contributor Author

arvidn commented Feb 19, 2025

@OverActiveBladderSystem there's a built-in CPU profiler that can be enabled in config.yaml, under full_node: there's enable_profiler: False, you you set that to true and restart the node, it will dump a CPU profile once per second (into the ~/.chia/mainnet/profile-node.

To look at the CPU usage over time, assuming you have the chia-blockchain source, python -m chia.util.profiler ~/.chia/mainnet/profile-node | less -r. It will print a graph of CPU usage over time along with timestamps (seconds).

To focus on a specific spike, run:

python -m chia.util.profiler ~/.chia/mainnet/profile-node <timestamp>

or

python -m chia.util.profiler ~/.chia/mainnet/profile-node <first-timestamp> <last-timestamp>

This will generate a gprof2dot graph and save it in the current directory. It requires pprof2dot and graphwiz (dot) be installed.

@arvidn
Copy link
Contributor Author

arvidn commented Feb 19, 2025

My understanding of the cost of having these invalid transactions in your peers' mempools is that it's proportional to your peer churn. Only when you connect to a peer do we request mempool items, we send our BIP158 filter of the transactions we have in our mempool (it's like a bloom filter) and the peer responds with its 100 "best" transactions that we don't have. Best meaning high fee-per-cost, so in this case it will be a random 100 invalid transactions.

According to your log, we validate each transaction in ~3 milliseconds (for 100 of them, that's 0.3 seconds). But then we'll never get any more bad transactions from that peer again, unless we disconnect and re-connect. During steady state, transactions are only propagated as we learn about new ones and we validate them and add them to our own mempool.

@arvidn arvidn marked this pull request as draft February 19, 2025 08:17
@arvidn arvidn force-pushed the invalid-transactions branch from 55b50db to e4535c8 Compare February 19, 2025 08:18
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Feb 19, 2025
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@arvidn arvidn changed the base branch from release/2.5.2 to main February 19, 2025 08:19
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Feb 19, 2025
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

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.

5 participants