test: use MiniWallet for mempool_updatefromblock.py #24183

pg156 · 2022-01-28T01:32:37Z

This PR enables a non-wallet functional test mempool_updatefromblock.py to be run without wallet compiled, by using the MiniWallet proposed in #20078. This picks up from where #21999 left off.

Commit 1 of 2: MiniWallet changes only to minimize diffs for easier reviewing
Commit 2 of 2: Refactor (renaming a function and variables)

The test run time decreases from 16.5s to 4.3s.

brunoerg · 2022-01-28T11:54:22Z

test/functional/mempool_updatefromblock.py

There is a whitespace here, you have to remove it to pass the linter.

Thanks. Fixed. Don't know why the linter now complains about the spelling errors in unchanged files.

^---- failure generated from test/lint/lint-python.sh
src/random.h:92: occuring ==> occurring
src/rpc/blockchain.cpp:794: nWe ==> new
src/util/syscall_sandbox.cpp:128: creat ==> create
test/functional/data/rpc_decodescript.json:81: ba ==> by, be
test/functional/data/rpc_decodescript.json:84: ba ==> by, be
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt

Cool, now you should squash the commits. See: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

Thanks. Done.

The problem of the linter now is "test/functional/mempool_updatefromblock.py:12:1: F401 'decimal.Decimal' imported but unused" - this PR removes all uses.

maflcko · 2022-02-22T11:50:35Z

Are you still working on this?

pg156 · 2022-02-27T14:12:11Z

Are you still working on this?

Yes. Read mzumsande's comment too quickly and didn't realize it was a request to change. Will fix soon.

pg156 · 2022-02-28T02:40:26Z

The linter problem is fixed.

aureleoules

tACK 203a3e065ecd0ec8a9fb84f4deb512ef2b66cdf7.
Tested on NixOS 22.05 64 bits with --disable-wallet.

I verified that the migration to MiniWallet does not alter what is being tested.
It takes 3s to run on my machine 🚀

maflcko · 2022-03-22T08:47:57Z

test/functional/mempool_updatefromblock.py

Why is this changed?

Changed back to getmempoolentry.

I chose getrawmempool over getmempoolentry to follow some existing functional test files. After a closer look at those two RPCs, I agree the existing getmempoolentry here is better as it indicates the "query using a transaction id" intention.

maflcko · 2022-03-22T08:48:16Z

test/functional/mempool_updatefromblock.py

Why does this need SEQUENCE_FINAL?

Without SEQUENCE_FINAL, the test broke at assert_equal(len(node.getrawmempool()), size) after the first block is invalidated, as the mempool has 26 transactions instead of the expected 100.

In a simpler example with fewer transactions without SEQUENCE_FINAL, I can see after the block is invalidated, transactions are removed from the mempool because of this place:

bitcoin/src/consensus/tx_verify.cpp

Line 53 in f05cf59

bool fEnforceBIP68 = static_cast<uint32_t>(tx.nVersion) >= 2

maflcko · 2022-03-22T08:49:19Z

test/functional/mempool_updatefromblock.py

Why is this changed? (from getmempoolentry to getrawmempool)?

Changed back to getmempoolentry.

maflcko · 2022-03-23T08:42:04Z

You'll need to do the changes from yesterday in the first commit, not in the second.

pg156 · 2022-03-25T16:10:17Z

Fixed. Thank you @MarcoFalke.

maflcko · 2022-03-30T14:32:22Z

Can you explain why it is ok to "flatten" the tournament into a linked list?

mjdietzx

I think this PR introduces an unwanted behavior change. It looks like now you have a simple straight line of spends tx0->tx1->...->txN.

Before this test created a huge, complicated "cyclic tournament" graph of transactions where:

Transaction tx[K] is a child of each of previous transactions tx[0]..tx[K-1] at their output K-1
Transaction tx[K] is an ancestor of each of subsequent transactions tx[K+1]..tx[N-1].

That's kind of hard to visualize. But the first transaction tx0 has 99 outputs, and tx1, ... tx100 all spend one of tx0's outputs. And tx100 has 99 inputs, an input from tx1, ... tx99. Looking at L51-L80 (before this PRs changes) you can try to imagine what tx4 looks like and that's kinda hard!

It's easy to see by the runtime of this test. Before this PR running this test in our CI took ~5 minutes. Now this test runs in ~1s.

I've found this test to be extremely valuable bc it highlights a possible denial of service attack vector. Imagine someone creates a graph of transactions like this and gets them all mined into a single block (let's just say they mine the block or collude w/ a miner). Even though the mempool has DEFAULT_ANCESTOR_LIMIT and DEFAULT_DESCENDANT_LIMIT of 25, this test just shows how large and complicated a web of dependent transactions you can make within those limits.

Now imagine that huge graph takes up a single block that's mined. And then somehow (maybe on purpose) that block gets reorged. All of a sudden this entire block of transactions enters the mempool again. And all the nodes need to validate every single transaction in this graph before adding them back to their mempool. Imagine if we did something stupid (or a lot of extra looping work) in the mempool validation logic. When that block gets reorged it could potentially crash or stall a bunch of nodes.

So I think this test is a really good stress test as-is. If you do any looping or weird stuff in the mempool validation logic this test can very easily timeout. I learned this the hard way when trying out some RBF changes, where I saw this test timing out. And I started to really appreciate the value of this specific test at that point

mjdietzx · 2022-04-03T23:11:17Z

test/functional/mempool_updatefromblock.py

        If all of the N created transactions tx[0]..tx[N-1] reside in the mempool,
        the following holds:
            the tx[K] transaction:
            - has N-K descendants (including this one), and
            - has K+1 ancestors (including this one)


I don't think these conditions hold true after this PR

mjdietzx · 2022-04-03T23:47:40Z

Concept ACK - I think this is a valuable test and it'll be good to update it so that it's not skipped when the wallet isn't compiled. My suggestion would be to minimize changes as much as possible to make it work w/ MiniWallet - and implementing this test as-is w/ MiniWallet will probably be difficult! But I think it'll be a good exercise, and I think MiniWallet can probably support what this test needs without any internal code changes to it (although I'm not totally sure bc I haven't looked at that code in a while)

pg156 · 2022-04-07T00:57:02Z

Thank you so much @mjdietzx for the review with the context and insights. I am changing this to draft status, to explore how to keep the graph structure.

DrahtBot · 2022-07-02T04:50:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25525 (test: remove wallet dependency from mempool_updatefromblock.py by ayush933)

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.

fanquake added the Tests label Jan 28, 2022

brunoerg reviewed Jan 28, 2022

View reviewed changes

pg156 force-pushed the mempool-updatefromblock-miniwallet branch from 47e8d66 to 934d09e Compare January 29, 2022 22:32

bitcoin deleted a comment from Ferrydh Feb 27, 2022

pg156 force-pushed the mempool-updatefromblock-miniwallet branch from 934d09e to 4b8eb3c Compare February 27, 2022 17:16

aureleoules reviewed Mar 22, 2022

View reviewed changes

maflcko approved these changes Mar 22, 2022

View reviewed changes

pg156 force-pushed the mempool-updatefromblock-miniwallet branch from 4b8eb3c to 7be943f Compare March 23, 2022 01:42

pg156 added 2 commits March 25, 2022 11:58

test: use MiniWallet for mempool_updatefromblock.py

f93b739

refactor: rename a function and variables

72ad8ad

pg156 force-pushed the mempool-updatefromblock-miniwallet branch from 7be943f to 72ad8ad Compare March 25, 2022 16:06

mjdietzx suggested changes Apr 3, 2022

View reviewed changes

pg156 marked this pull request as draft April 7, 2022 00:57

shommel mentioned this pull request Apr 22, 2022

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

Closed

DrahtBot mentioned this pull request Jul 1, 2022

test: remove wallet dependency from mempool_updatefromblock.py #25525

Merged

maflcko closed this Jul 7, 2022

bitcoin locked and limited conversation to collaborators Jul 7, 2023

test: use MiniWallet for mempool_updatefromblock.py #24183

test: use MiniWallet for mempool_updatefromblock.py #24183

Uh oh!

Conversation

pg156 commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 22, 2022

Uh oh!

pg156 commented Feb 27, 2022

Uh oh!

pg156 commented Feb 28, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 23, 2022

Uh oh!

pg156 commented Mar 25, 2022

Uh oh!

maflcko commented Mar 30, 2022

Uh oh!

mjdietzx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjdietzx commented Apr 3, 2022

Uh oh!

pg156 commented Apr 7, 2022

Uh oh!

DrahtBot commented Jul 2, 2022

Conflicts

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

maflcko Mar 22, 2022 •

edited

Loading