test: run mempool_reorg.py even with wallet disabled #21178

DariusParvin · 2021-02-15T03:39:26Z

Run mempool_reorg.py test even when the wallet is disabled, as discussed in #20078.

As part of this PR I created a new method in MiniWallet, create_self_transfer, to return a raw tx (without broadcasting it) and its associated utxo.

DrahtBot · 2021-02-15T08:44:27Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Run mempool_limit.py even with wallet disabled #20874 (test: Run mempool_limit.py even with wallet disabled by stackman27)

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.

glozow

Concept ACK, very nice!

test/functional/test_framework/wallet.py

test/functional/mempool_reorg.py

test/functional/test_framework/wallet.py

felipsoarez

Concept ACK

maflcko · 2021-02-16T16:54:20Z

test/functional/mempool_reorg.py

Generating blocks takes a few seconds in valgrind, so I am thinking if this test may benefit from using miniwallet.scan_blocks (to be introduced in #21200)

I had a go but I don't think it helps in this case since the pre-mined chain doesn't send the coinbase transaction to self._address. I made it little bit faster though by only mining 100 blocks instead of 200.

It should. Specifically the last 25 mature blocks (including block 100). And the last 25 immature blocks (including block 200), but you don't need those.

Sorry, there was a bug. If you rebase, the bug should fix itself.

yes, it worked! I updated the PR to use miniwallet.scan_blocks starting from block 76.

stackman27

Concept ACK. mempool_reorg looks really good except, is there a reason why we aren't using self.log.info(...)? I always thought of them to be really helpful while running tests. Therefore, i think it'll be really nice to add the logs :)

test/functional/test_framework/wallet.py

fanquake · 2021-04-01T08:50:11Z

@DariusParvin are you planning on following up with the review comments here?

DariusParvin · 2021-04-02T20:58:58Z

@fanquake Yes, thanks for prompting me, I'll push updates this weekend!

DariusParvin · 2021-04-20T06:05:00Z

I've addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

DariusParvin · 2021-05-17T01:50:24Z

@stackman27 thanks for your review! I added logs instead of comments, as you suggested.

maflcko

cr ACK a856630c27cd28186d4e39dd1129f0cae8149261

test/functional/mempool_reorg.py

michaelfolkson · 2021-05-17T11:29:41Z

I've addressed the comments, but since there are conflicting changes with #20874, it would probably make sense for that one to get merged first.

Is this still waiting on #20874? Given Marco has reviewed this and not #20874 presumably this is closer to merging? 😅

edit: #20874 needs a rebase so yeah I think this is closer to merging. Obvious Concept ACK too btw.

michaelfolkson · 2021-05-17T12:27:34Z

The test is still skipped when wallet is disabled. I think you need to delete:

def skip_test_if_missing_module(self):
        self.skip_if_no_wallet()

DariusParvin · 2021-05-17T15:25:42Z

@michaelfolkson oops! I removed those lines. And yes, since #21762 where create_self_transfer was added, this PR is relatively independent of everything else, so it's not waiting on anything. Cheers

michaelfolkson

ACK 65dd482ba8c87443e4c27dc28f2027625ca43005

Reviewed code, test passes (and not skipped) with wallet disabled.

test/functional/mempool_reorg.py

michaelfolkson · 2021-05-29T17:49:18Z

Re-ACK 163e8881e88a798462a60b5232a6b4ff62e1969a

maflcko · 2021-05-31T11:32:56Z

s/disable/disabled/ in commit message 163e8881e88a798462a60b5232a6b4ff62e1969a

- run mempool_reorg.py even when the wallet is not compiled - add `locktime` argument to `create_self_transfer` and `send_self_transfer` - use more logs instead of comments

DariusParvin · 2021-05-31T17:41:43Z

thanks @MarcoFalke! typo fixed and rebased

maflcko · 2021-06-01T13:04:45Z

cr ACK a3f0cbf

…bled

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 8e3f29e to 2ff5034 Compare February 15, 2021 04:05

DrahtBot added the Tests label Feb 15, 2021

glozow reviewed Feb 16, 2021

View reviewed changes

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

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

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

felipsoarez reviewed Feb 16, 2021

View reviewed changes

maflcko reviewed Feb 16, 2021

View reviewed changes

stackman27 reviewed Feb 22, 2021

View reviewed changes

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 2ff5034 to 8062fb8 Compare April 5, 2021 05:01

This was referenced Apr 22, 2021

test: Run feature_cltv with MiniWallet #21754

Merged

test: Speed up mempool_spend_coinbase.py #21762

Merged

DrahtBot added the Needs rebase label Apr 29, 2021

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 8062fb8 to 2c53863 Compare May 17, 2021 00:34

DrahtBot removed the Needs rebase label May 17, 2021

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 2c53863 to a856630 Compare May 17, 2021 01:40

maflcko approved these changes May 17, 2021

View reviewed changes

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

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from a856630 to ded2a49 Compare May 17, 2021 15:22

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch 2 times, most recently from 25380cf to 65dd482 Compare May 18, 2021 07:15

michaelfolkson reviewed May 27, 2021

View reviewed changes

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

DrahtBot mentioned this pull request May 28, 2021

test: Use COINBASE_MATURITY in functional tests #21989

Merged

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from fb84bd3 to 163e888 Compare May 28, 2021 15:05

DrahtBot added the Needs rebase label May 31, 2021

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 163e888 to 1c9d072 Compare May 31, 2021 16:18

test: run mempool_reorg.py even with wallet disabled

a3f0cbf

- run mempool_reorg.py even when the wallet is not compiled - add `locktime` argument to `create_self_transfer` and `send_self_transfer` - use more logs instead of comments

DariusParvin force-pushed the mempool-reorg-to-miniwallet branch from 1c9d072 to a3f0cbf Compare May 31, 2021 16:28

DrahtBot removed the Needs rebase label May 31, 2021

maflcko merged commit 7e83e74 into bitcoin:master Jun 1, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 1, 2021

Merge bitcoin#21178: test: run mempool_reorg.py even with wallet disa…

2b04a4c

…bled

shommel mentioned this pull request Apr 22, 2022

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

Closed

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 7e83e74 into merged_master (Bitcoin PR bitcoin/bitcoin#21178)

f46e4fd

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

test: run mempool_reorg.py even with wallet disabled #21178

test: run mempool_reorg.py even with wallet disabled #21178

Uh oh!

Conversation

DariusParvin commented Feb 15, 2021

Uh oh!

DrahtBot commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felipsoarez left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

DariusParvin Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 20, 2021 • 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!

maflcko Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

DariusParvin May 17, 2021

Choose a reason for hiding this comment

Uh oh!

stackman27 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanquake commented Apr 1, 2021

Uh oh!

DariusParvin commented Apr 2, 2021

Uh oh!

DariusParvin commented Apr 20, 2021

Uh oh!

DariusParvin commented May 17, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelfolkson commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelfolkson commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DariusParvin commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelfolkson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelfolkson commented May 29, 2021

Uh oh!

maflcko commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DariusParvin commented May 31, 2021

Uh oh!

maflcko commented Jun 1, 2021

Uh oh!

Reviewers

Assignees

Labels

DrahtBot commented Feb 15, 2021 •

edited

Loading

maflcko Apr 20, 2021 •

edited

Loading

stackman27 left a comment •

edited

Loading

michaelfolkson commented May 17, 2021 •

edited

Loading

michaelfolkson commented May 17, 2021 •

edited

Loading

DariusParvin commented May 17, 2021 •

edited

Loading

maflcko commented May 31, 2021 •

edited

Loading