-
Notifications
You must be signed in to change notification settings - Fork 117
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
sweepbatcher: set max batch size to 1000 sweeps #813
sweepbatcher: set max batch size to 1000 sweeps #813
Conversation
sweepbatcher/sweep_batcher_test.go
Outdated
// Wait for the batcher to be initialized. | ||
<-batcher.initDone | ||
|
||
const swapsNum = 2500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reduce test runtime, would just 1001 work? Also the test seems to be logging excessively, so perhaps we could reduce that too. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea! I changed the number of swaps to 1001. Disabling logging has negligible effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling logging is helpful when running make unit
locally, as it's easier to follow test logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabled logging in the test.
ad3980d
to
dcd8105
Compare
This is needed to avoid non-standard batch transactions (larger than 400k wu). A non-cooperative input is 393 wu, so 1000 inputs are still under 400k wu.
dcd8105
to
f21a21e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 💾
// the batch, do not add another sweep to prevent the tx from becoming | ||
// non-standard. | ||
if len(b.sweeps) >= MaxSweepsPerBatch { | ||
return false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to log such an occurence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added Info log message in cases when addSweep returns accept=false and err=nil in a separate commit "sweepbatcher: log the reason of acceptance failure".
Add Info log message in cases when addSweep returns accept=false and err=nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This is needed to avoid non-standard batch transactions (larger than 400k wu). A non-cooperative input is 393 wu, so 1000 inputs are still under 400k wu.
Pull Request Checklist
release_notes.md
if your PR contains major features, breaking changes or bugfixes