-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
itest+lntest: speed up test setup #9195
itest+lntest: speed up test setup #9195
Conversation
This commit fixes the methods used in `lntest` so they stop using pointers to chainhash.
So we only need to do one `GetRawMempool` lookup when checking the exclusion of multiple txns.
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.
Nice! LGTM 🎉
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 🎉, great improvement for single itest runs, thanks for the investigation!
@@ -342,6 +342,7 @@ func (h *HarnessTest) SetupStandbyNodes() { | |||
// above a good number of confirmations. | |||
const totalTxes = 200 | |||
h.MineBlocksAndAssertNumTxes(numBlocksSendOutput, totalTxes) | |||
h.MineBlocks(numBlocksSendOutput) |
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.
nit: may move this change to its own commit, the motivation is not clear
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.
it's a mistake...was doing a performance comparison and forgot to remove it🤦🏻
will remove it in my other PRs since it's minor
// We require the RPC call to be succeeded and won't wait for | ||
// it as it's an unexpected behavior. |
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.
nit: not sure I understand that comment
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.
Seems to be copy/paste, appears in a couple of other places. I interpret it as: "this will call ht.t.Fail()
if it doesn't succeed, which will abort the test, so we can continue below assuming it didn't fail".
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.
Very nice!
// We require the RPC call to be succeeded and won't wait for | ||
// it as it's an unexpected behavior. |
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.
Seems to be copy/paste, appears in a couple of other places. I interpret it as: "this will call ht.t.Fail()
if it doesn't succeed, which will abort the test, so we can continue below assuming it didn't fail".
Turns out we are doing 200
GetRawMempool
RPC calls when setting up the test case, which is now optimized.Before this change,
After this change,
Also changes the methods so we don't pass pointers of
chainhash.Hash
anymore in the tests.Thanks @bitromortac for pointing this out!