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

config: make Tx dedupe cache maxSize configurable #5419

Merged
merged 7 commits into from Jun 2, 2023

Conversation

iansuvak
Copy link
Contributor

@iansuvak iansuvak commented May 25, 2023

Summary

Allow node runners to set custom max size for the incoming transaction deduplication. Setting it higher could decrease the CPU load and the amount of egress data on the networking size at the cost of extra memory.

This PR introduces a new config TxIncomingFilterMaxSize set to 500k. This is just under a 10x increase from the previous value which depended on the TxBacklogSize the default for which is 26000 * 2. At periods of high transaction volume it's possible to see many more transactions than the two block estimate and this change allows individual operators to set appropriate limits based on the traffic they observe and the memory overhead that they have.

Test Plan

Existing tests

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

LGTM.
Max memory estimate is 2 x 500,000 x 32 b = 32 Mb?

also need to add/generate config-v28.json (by running make generate I guess)

@iansuvak
Copy link
Contributor Author

also need to add/generate config-v28.json (by running make generate I guess)

Thanks for the catch. I generated it locally to ensure that config tests pass locally but forgot to add the new file to my commit. Fixed now

@algorandskiy algorandskiy requested review from cce and icorderi May 25, 2023 21:17
algorandskiy
algorandskiy previously approved these changes May 25, 2023
@algorandskiy
Copy link
Contributor

Please rebase - I think the e2e test failure is fixed in master

@codecov
Copy link

codecov bot commented May 26, 2023

Codecov Report

Merging #5419 (41706dc) into master (e249905) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5419      +/-   ##
==========================================
+ Coverage   55.39%   55.40%   +0.01%     
==========================================
  Files         447      447              
  Lines       63313    63312       -1     
==========================================
+ Hits        35072    35080       +8     
+ Misses      25849    25842       -7     
+ Partials     2392     2390       -2     
Impacted Files Coverage Δ
config/localTemplate.go 60.60% <ø> (ø)
data/txHandler.go 73.13% <100.00%> (-0.08%) ⬇️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -171,13 +171,11 @@ func MakeTxHandler(opts TxHandlerOpts) (*TxHandler, error) {
streamVerifierDropped: make(chan *verify.UnverifiedTxnSigJob),
}

// use defaultBacklogSize = approx number of txns in a full block as a parameter for the dedup cache size
Copy link
Contributor

Choose a reason for hiding this comment

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

Your PR describes what change is being made, but could you also include why? it would be helpful if we ever need to refer back to this change. This comment describes a reasonable estimation, so why 10x it?
(I know we've discussed it plenty in team settings, but I want to make sure it's recorded)

AlgoAxel
AlgoAxel previously approved these changes May 26, 2023
@algorandskiy algorandskiy dismissed stale reviews from AlgoAxel and themself via e080fe0 June 2, 2023 16:14
algorandskiy
algorandskiy previously approved these changes Jun 2, 2023
@algorandskiy algorandskiy merged commit 3cbb422 into algorand:master Jun 2, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants