test: refactor use of getrawmempool in functional tests for efficiency

mjdietzx commented at 4:16 pm on August 15, 2021: contributor

I don’t think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I’ve seen our CI macOS 11 native [gui] [no depends] runs mempool_updatefrom.py in ~135 seconds. After this PR it should run in ~105 seconds

I noticed this improvement should probably be made when testing performance/runtimes of #22698. But I wanted to separate this out from that PR so the affects of each is decoupled

Edit: The major change in this PR is improving mempool_updatefrom.py’s runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here

DrahtBot added the label Tests on Aug 15, 2021

fanquake requested review from ariard on Aug 16, 2021

fanquake requested review from glozow on Aug 16, 2021

in test/functional/mempool_updatefromblock.py:115 in 99fb430128 outdated

114-            assert_equal(self.nodes[0].getrawmempool(True)[tx]['ancestorcount'], k + 1)
115-            assert_equal(self.nodes[0].getrawmempool(True)[tx]['ancestorsize'], sum(tx_size[0:(k + 1)]))
116+            assert_equal(self.nodes[0].getmempoolentry(tx)['descendantcount'], size - k)
117+            assert_equal(self.nodes[0].getmempoolentry(tx)['descendantsize'], sum(tx_size[k:size]))
118+            assert_equal(self.nodes[0].getmempoolentry(tx)['ancestorcount'], k + 1)
119+            assert_equal(self.nodes[0].getmempoolentry(tx)['ancestorsize'], sum(tx_size[0:(k + 1)]))

glozow commented at 10:04 am on August 16, 2021:

If you’re going to do this to save time, you might as well just call the RPC once.

0            entry = self.nodes[0].getmempoolentry(tx)
1            assert_equal(entry['descendantcount'], size - k)
2            assert_equal(entry['descendantsize'], sum(tx_size[k:size]))
3            assert_equal(entry['ancestorcount'], k + 1)
4            assert_equal(entry['ancestorsize'], sum(tx_size[0:(k + 1)]))

glozow commented at 10:15 am on August 16, 2021:

This puts me at 13sec (vs 33sec on master).

glozow commented at 10:18 am on August 16, 2021: member

Concept ACK, it certainly saves time to just fetch the entry we need instead of the entire mempool, particularly because this test puts a lot of transactions in there. It is significantly faster on my machine as well.

Should do this for other functional tests as well, e.g:

test/functional/mempool_compatibility.py test/functional/rpc_fundrawtransaction.py test/functional/mempool_package_limits.py

(From a quick grep for getrawmempool(True) and getrawmempool(verbose=True) and eliminating the ones that should actually use getrawmempool)

mjdietzx commented at 12:51 pm on August 16, 2021: contributor

Thanks @glozow - good suggestions, I’ll follow up with these shortly

test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns 86dbd54ae8

test: use getmempoolentry instead of getrawmempool in functional tests when appropriate 77349713b1

test: only use verbose for getrawmempool when necessary in functional tests 47c48b5f35

mjdietzx force-pushed on Aug 16, 2021

DrahtBot commented at 6:44 pm on August 16, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22689 (rpc: deprecate top-level fee fields in getmempool RPCs by josibake)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

josibake approved

josibake commented at 8:38 am on August 17, 2021: member

Code Review and Concept ACK

+1 for always preferring getmempoolentry over getrawmempool when you are referencing specific txns.

left a suggestion about using deprecated fee fields, but like i mentioned, it might be out of scope for this PR.

i would also suggest updating the title and description of the PR to reflect that you are globally replacing references to getrawmempool (right now it’s still specific to mempool_updatefrom)

after updating the title and description, https://github.com/bitcoin/bitcoin/pull/22707/commits/86dbd54ae8a8f9c693c0ea67114bbff24a0754df and https://github.com/bitcoin/bitcoin/pull/22707/commits/77349713b189e80f2c140db4df50177353a1cb83 should be squashed

you could even squash all three, since both changes fall under refactoring for efficiency :man_shrugging:

mjdietzx renamed this:
~~test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns~~
test: refactor use of getrawmempool in functional tests for efficiency
on Aug 17, 2021

mjdietzx commented at 10:59 am on August 17, 2021: contributor

Thanks for the review @josibake ! Updated PR title/description accordingly

left a suggestion about using deprecated fee fields, but like i mentioned, it might be out of scope for this PR.

I think this is/should be done in your PR #22689. If we have merge conflicts one of us will just need to rebase depending on whose PR gets merged first.

theStack commented at 11:21 am on August 19, 2021: member

Concept ACK

theStack approved

theStack commented at 5:23 pm on August 19, 2021: member

Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80

On my machine, the speed-up of mempool_updatefromblock.py is quite impressive (>2x):

master branch:

0$ time ./test/functional/mempool_updatefromblock.py
1    3m18.16s real     2m04.55s user     0m33.54s system

PR branch:

0$ time ./test/functional/mempool_updatefromblock.py
1    1m29.11s real     0m34.37s user     0m14.79s system

darosior commented at 9:48 am on August 20, 2021: member

Concept ACK

MarcoFalke merged this on Aug 20, 2021

MarcoFalke closed this on Aug 20, 2021

sidhujag referenced this in commit 994629a3c1 on Aug 20, 2021

DrahtBot locked this on Aug 20, 2022

test: refactor use of getrawmempool in functional tests for efficiency #22707

Conflicts