test: Use MiniWallet in mempool_limit.py #22543

ShubhamPalriwala · 2021-07-25T07:35:46Z

This is a PR proposed in #20078

This PR enables running another non-wallet functional test even when the wallet is disabled thanks to the MiniWallet, i.e. it can be run even when bitcoin-core is compiled with --disable-wallet.

It also includes changes in wallet.py in the form of a new method, create_large_transactions() for the MiniWallet to create large transactions.

Efforts for this feature started in #20874 but were not continued and that PR was closed hence I picked this up.

To test this PR locally, compile and build bitcoin-core without the wallet and run:

$ test/functional/mempool_limit.py

amitiuttarwar · 2021-07-25T19:07:00Z

approach ACK :)

practicalswift · 2021-07-25T20:05:05Z

Concept ACK

DrahtBot · 2021-07-28T10:28:36Z

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

Conflicts

No conflicts as of last run.

test/functional/mempool_limit.py

theStack · 2021-07-31T20:18:21Z

Concept ACK

Zero-1729 · 2021-08-03T15:22:25Z

Concept ACK

Shubhankar-Gambhir · 2021-08-05T18:45:14Z

Concept ACK

test/functional/mempool_limit.py

NikhilBartwal

Concept and Approach ACK and Tested dcd75d8

Checked that:

Running test/functional/mempool_limit.py after compiling bitcoin core from source with --disable-wallet leads to Test Skips: Test Skipped: wallet has not been compiled.
Running the same test with the same flag with the PR paches now successfully runs the tests: Tests successful

Just had a few comments to add :)

test/functional/mempool_limit.py

hg333

tACK dcd75d8

tested on Ubuntu 20.04
On Master Branch:

$ time ./test/functional/mempool_limit.py 
...
real	0m17.054s
user	0m3.117s
sys	0m0.515s

On PR Branch:

$ time ./test/functional/mempool_limit.py 
...

real	0m7.905s
user	0m2.777s
sys	0m0.139s

test/functional/mempool_limit.py

Kirandevraj

Concept and Approach ACK and Tested c6b17eb
Compiled Bitcoin core with this PR patch and --disable-wallet flag and ran the test/functional/mempool_limit.py test: Test successful
Compiled Bitcoin core without this PR patch with the same --disable-wallet flag and ran the test: Test Skipped

DariusParvin

ACK bec7849
Tested it and looks good! A few suggestions

test/functional/mempool_limit.py

test/functional/test_framework/wallet.py

test/functional/mempool_limit.py

josibake

thanks for taking the suggestion for send_large_transactions! one small pep8 fix suggested

test/functional/mempool_limit.py

josibake · 2021-09-02T12:14:30Z

ACK e8a3ff3

nicely done, happy to see the send_large_transactions helper function being added to miniwallet!

Zero-1729

re-ACK e8a3ff3

LGTM 💾

test/functional/mempool_limit.py

test/functional/test_framework/wallet.py

test/functional/mempool_limit.py

ShubhamPalriwala · 2021-09-10T11:52:45Z

PR ready for review

Thank you to everybody who has reviewed it. However, the recent suggestions made the PR better and more helpful hence the changes as suggested have been implemented.

test/functional/mempool_limit.py

maflcko

review ACK 22c7392 🍚

Show signature and timestamp

Signature:

Timestamp of file with hash 668a0177b4b7850b6d72a3a9f719e35848f7d38faaf8b37423b4d77b70903b93 -

test/functional/mempool_limit.py

maflcko · 2021-09-13T08:31:02Z

lmk if you want this merged or address the nits

ShubhamPalriwala · 2021-09-13T11:58:48Z

@MarcoFalke the nits have been addressed! PTAL.
The PR is ready to be merged from my end :)

maflcko · 2021-09-13T12:26:17Z

ack

amitiuttarwar

I find the git commit breakup a bit confusing - I think the last commit 41fa226 should be merged into the first commit cdf1bf6, because it's cleaning up the code that was introduced in this PR. It makes more sense to just introduce it correctly in the first place- it simplifies review & git blame history in the future.

Also, I'd prefer if the git commit messages were more descriptive & adhered to the conventions of this codebase. For example, I've never seen the prefix feat:. I used the command git log -S "miniwallet" to see that most of the miniwallet refactors use the prefix test: or maybe refactor:. A more common prefix instead of replace: or fix: would probably be style:. In terms of descriptiveness, commit messages are the most useful when you explain why the change is being made. While this is fine to skip on self-evident changes (like fixing a typo or renaming to node), the first commit could describe the motivation. If you're interested in learning more, check out https://chris.beams.io/posts/git-commit/.

All that said, I think the code introduced looks reasonable, so review ACK 41fa226. If you decide to clean up the git history, I'm happy to reack- it should not produce a code diff.

Co-authored-by: ShubhamPalriwala <spalriwalau@gmail.com> Co-authored-by: stackman27 <sishirg27@gmail.com> Signed-off-by: ShubhamPalriwala <spalriwalau@gmail.com>

ShubhamPalriwala · 2021-09-13T19:33:00Z

Thank you for the git suggestions @amitiuttarwar
Updated and force-pushed the commits for a more clear understanding of the changes.

The PR is once again up for review. Hopefully one last time

amitiuttarwar · 2021-09-13T20:02:19Z

ACK 08634e8, only git changes since last push (and one new line).

Zero-1729

ACK 08634e8 🧉

maflcko · 2021-09-14T09:12:49Z

Thanks for taking the feedback. Looks good now.

theStack · 2021-09-14T09:32:09Z

Note that the helper send_large_txs with its current interface is misleading, as the passed fee_rate is not respected. It is merely passed to MiniWallet's create_self_transfer, but this function can't look in the future and thus is unable to know that several outputs are appended to the transaction after, modifying its size and hence also decreasing it's feerate. I'd suggest to either pass an absolute fee that is simply deducted from tx.vout[0].nValue before the transaction is sent (simple solution), or keep the interface, but fix the fee calculation by deducting dependend on the tx's final vsize (a little more complex solution). In both cases, create_self_transfer needs to be called with a zero fee_rate first. Will take a look into that later.

maflcko · 2021-09-14T09:38:52Z

@theStack See also #22543 (comment) , where I mentioned in-lining helps. I think we should pick the simplest/shortest code to achieve the goal of creating txs with different fee levels. The feerate doesn't need to be exact for the purposes of mempool eviction. The current code achieves its goal, but improvements are welcome if there are any.

theStack · 2021-09-14T14:10:34Z

@MarcoFalke: Sure, but in this case the fee-rate is not just "not exact", but off by several orders of magnitute, considering that the tx's vsize changes from 96 to 67552 vbytes (>700x), and MiniWallet only calculates the fee based on the 96 vbytes. So the value passed to this function is neither really a fee-rate nor an absolute fee, but something weird in-between that has just been increased (by 10x) until the test passes. I tried to increase the clarity of the test in #22972.

…_limit.py 2600db6 test: fix misleading fee unit in mempool_limit.py (Sebastian Falbesoner) Pull request description: The PR is a follow-up to #22543. The helper `send_large_txs` in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet's `create_self_transfer` method, which can't know that we append several tx outputs after, increasing the tx's vsize and decreasing it's fee rate accordingly. In our case, the fee rate is off by several orders of magnitude, as the tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. It was suggested to simply in-line this helper as it's currently only used in this single test (bitcoin/bitcoin#22543 (comment), bitcoin/bitcoin#22543 (comment)), but I could imagine that this helper may also become useful for other tests and may be moved to a library (e.g. wallet.py) in the future. Clarify the interface by passing an absolute fee that is deducted in the end (and also verified, via testmempoolaccept) and also describe how we come up with the value passed. On master, the comment says that the fee rate needs to increased "massively"; this word is also removed because the fee rate only needs to be higher for the test to succeed. ACKs for top commit: stratospher: ACK 2600db6. Tree-SHA512: 0bfacc3fa87603970d86c1d0186e51511f6c20c64b0559e19e7e12a68647f79dcb4f436000dee718fd832ce6a68e3bbacacb29145e0287811f1cb03d2f316843

DrahtBot added the Tests label Jul 25, 2021

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch from 0acb21e to 8ac10d8 Compare July 25, 2021 16:08

DrahtBot mentioned this pull request Jul 28, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

maflcko reviewed Jul 28, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch 3 times, most recently from 78de7bf to 4beb43c Compare August 3, 2021 14:47

DrahtBot mentioned this pull request Aug 6, 2021

mempool/validation: mempool ancestor/descendant limits for packages #21800

Merged

DrahtBot added the Needs rebase label Aug 9, 2021

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch 3 times, most recently from 4cc1c96 to a2e4555 Compare August 9, 2021 20:22

DrahtBot removed the Needs rebase label Aug 9, 2021

NikhilBartwal reviewed Aug 10, 2021

View reviewed changes

test/functional/mempool_limit.py Show resolved Hide resolved

NikhilBartwal reviewed Aug 10, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

NikhilBartwal reviewed Aug 10, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

NikhilBartwal reviewed Aug 10, 2021

View reviewed changes

test/functional/mempool_limit.py Show resolved Hide resolved

NikhilBartwal approved these changes Aug 10, 2021

View reviewed changes

NikhilBartwal reviewed Aug 10, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch from dcd75d8 to c6b17eb Compare August 11, 2021 05:06

hg333 approved these changes Aug 12, 2021

View reviewed changes

test/functional/mempool_limit.py Show resolved Hide resolved

Kirandevraj approved these changes Aug 12, 2021

View reviewed changes

DariusParvin reviewed Aug 16, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

test/functional/test_framework/wallet.py Outdated Show resolved Hide resolved

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch from bec7849 to 4a6d379 Compare August 16, 2021 09:17

josibake reviewed Sep 2, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

Zero-1729 reviewed Sep 2, 2021

View reviewed changes

maflcko reviewed Sep 2, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 6, 2021

Improve mempool_package_limits.py #22901

Closed

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch from e8a3ff3 to 22c7392 Compare September 10, 2021 11:46

maflcko reviewed Sep 13, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

maflcko approved these changes Sep 13, 2021

View reviewed changes

test/functional/mempool_limit.py Outdated Show resolved Hide resolved

amitiuttarwar reviewed Sep 13, 2021

View reviewed changes

ShubhamPalriwala and others added 3 commits September 14, 2021 00:52

test: use MiniWallet in mempool_limit.py

dddca38

Co-authored-by: ShubhamPalriwala <spalriwalau@gmail.com> Co-authored-by: stackman27 <sishirg27@gmail.com> Signed-off-by: ShubhamPalriwala <spalriwalau@gmail.com>

replace: self.nodes[0] with node

d447ded

fix typos in logging messages

08634e8

ShubhamPalriwala force-pushed the diswallet-mempool_limit branch from 41fa226 to 08634e8 Compare September 13, 2021 19:29

Zero-1729 reviewed Sep 13, 2021

View reviewed changes

maflcko merged commit 2b26497 into bitcoin:master Sep 14, 2021

theStack mentioned this pull request Sep 14, 2021

test: fix misleading fee unit in mempool_limit.py #22972

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2021

Merge bitcoin#22543: test: Use MiniWallet in mempool_limit.py

d3c8141

shommel mentioned this pull request Apr 22, 2022

test: Convert non-wallet tests to use our python MiniWallet #20078

Closed

bitcoin locked and limited conversation to collaborators Oct 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Use MiniWallet in mempool_limit.py #22543

test: Use MiniWallet in mempool_limit.py #22543

ShubhamPalriwala commented Jul 25, 2021

amitiuttarwar commented Jul 25, 2021

practicalswift commented Jul 25, 2021

DrahtBot commented Jul 28, 2021 •

edited

theStack commented Jul 31, 2021

Zero-1729 commented Aug 3, 2021

Shubhankar-Gambhir commented Aug 5, 2021

NikhilBartwal left a comment •

edited

hg333 left a comment

Kirandevraj left a comment

DariusParvin left a comment •

edited

josibake left a comment

josibake commented Sep 2, 2021 •

edited

Zero-1729 left a comment

ShubhamPalriwala commented Sep 10, 2021

maflcko left a comment

maflcko commented Sep 13, 2021

ShubhamPalriwala commented Sep 13, 2021

maflcko commented Sep 13, 2021

amitiuttarwar left a comment

ShubhamPalriwala commented Sep 13, 2021

amitiuttarwar commented Sep 13, 2021

Zero-1729 left a comment

maflcko commented Sep 14, 2021

theStack commented Sep 14, 2021

maflcko commented Sep 14, 2021

theStack commented Sep 14, 2021

test: Use MiniWallet in mempool_limit.py #22543

test: Use MiniWallet in mempool_limit.py #22543

Conversation

ShubhamPalriwala commented Jul 25, 2021

amitiuttarwar commented Jul 25, 2021

practicalswift commented Jul 25, 2021

DrahtBot commented Jul 28, 2021 • edited

Conflicts

theStack commented Jul 31, 2021

Zero-1729 commented Aug 3, 2021

Shubhankar-Gambhir commented Aug 5, 2021

NikhilBartwal left a comment • edited

Choose a reason for hiding this comment

hg333 left a comment

Choose a reason for hiding this comment

Kirandevraj left a comment

Choose a reason for hiding this comment

DariusParvin left a comment • edited

Choose a reason for hiding this comment

josibake left a comment

Choose a reason for hiding this comment

josibake commented Sep 2, 2021 • edited

Zero-1729 left a comment

Choose a reason for hiding this comment

ShubhamPalriwala commented Sep 10, 2021

PR ready for review

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Sep 13, 2021

ShubhamPalriwala commented Sep 13, 2021

maflcko commented Sep 13, 2021

amitiuttarwar left a comment

Choose a reason for hiding this comment

ShubhamPalriwala commented Sep 13, 2021

The PR is once again up for review. Hopefully one last time

amitiuttarwar commented Sep 13, 2021

Zero-1729 left a comment

Choose a reason for hiding this comment

maflcko commented Sep 14, 2021

theStack commented Sep 14, 2021

maflcko commented Sep 14, 2021

theStack commented Sep 14, 2021

DrahtBot commented Jul 28, 2021 •

edited

NikhilBartwal left a comment •

edited

DariusParvin left a comment •

edited

josibake commented Sep 2, 2021 •

edited