test: Test p2sh-witness and bech32 in wallet_import_rescan #16465

maflcko · 2019-07-25T21:05:29Z

This adds test coverage for segwit in the wallet_import_rescan test, among other cleanups.

maflcko · 2019-07-25T21:46:03Z

Fixed linter by adding the false-positive to a whitelist 😬

DrahtBot · 2019-07-26T00:13:40Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16597 ([WIP] Travis: run full test suite on native macOS by Sjors)
#15845 (wallet: Fast rescan with BIP157 block filters by MarcoFalke)
#15257 (Scripts and tools: Bump flake8 to 3.7.8 by Empact)
#13728 (lint: Run the CI lint stage on mac by Empact)

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.

Coverage

Coverage	Change (pull 16465, ba118a39c84bb79e5ed12db0050ff18d1595544c)	Reference (master, `e5fdda6`)
Lines	+0.0337 %	89.2260 %
Functions	-0.0157 %	82.1944 %
Branches	+0.0175 %	46.6152 %

^{Updated at: 2019-08-07T11:47:01.569823.}

Sjors · 2019-07-26T18:20:49Z

More dead code hits? https://gist.github.com/Sjors/e2704ff54fdc85b2b5833242a1d1e3e3

Tests pass on top of #16301.

maflcko · 2019-07-26T19:17:04Z

Tests pass on top of #16301

Thanks, was about to test this.

Sjors · 2019-08-06T19:09:34Z

ACK fa2a2bab0ffef281940c5383f2f4528145707ee6

practicalswift · 2019-08-07T12:16:01Z

@Sjors Regarding your posted gist: according to lint-python-dead-code.sh vulture should only look at files returned by git ls-files. Really weird that it checks .git/logs/ and depends/. Files under these directories should never be returned by git ls-files, right?

jnewbery

A few nits inline. Also, in commit test: Generate one block for each send in wallet_import_rescan, can you add a motivation to the commit log. I can see what it's doing, but I don't understand why.

test/functional/wallet_import_rescan.py

test/functional/test_framework/address.py

test/lint/lint-python-dead-code-whitelist

test/functional/wallet_import_rescan.py

This ... * ensures that enough coins are available/spendable, even when more variants are added * ensures that all mempool txs are mined, even when more variants are added * makes the test more specific to test that the confirmation height is properly reported and timestamps are correctly handled in the test logic * prepares the test for a future, where blocks are skipped for rescan if they are deemed irrelevant by a filter (c.f. BIP157)

maflcko · 2019-08-14T20:24:54Z

A few nits inline. Also, in commit test: Generate one block for each send in wallet_import_rescan, can you add a motivation to the commit log. I can see what it's doing, but I don't understand why.

Added a few bullet points why generating a block each makes sense.

Thanks for the review @jnewbery. I think I addressed all feedback

jnewbery

Looks good. Couple more comments inline.

test/functional/wallet_import_rescan.py

test/lint/lint-python-dead-code.sh

maflcko · 2019-08-15T12:08:30Z

Reworked the whitelist, as requested by @jnewbery

jnewbery

ACK fa3c657

jnewbery · 2019-08-15T14:21:33Z

test/lint/lint-python-dead-code-whitelist

@@ -0,0 +1,45 @@
+BadInputOutpointIndex  # unused class (test/functional/data/invalid_txs.py)


It is sorted, according to cat ./*whitelist|sort on my system

Fascinating:

→ cat test/lint/lint-python-dead-code-whitelist | LC_ALL=en_US.UTF-8 sort BadInputOutpointIndex # unused class (test/functional/data/invalid_txs.py) _.carbon_path # unused attribute (contrib/macdeploy/custom_dsstore.py) connection_lost # unused function (test/functional/test_framework/mininode.py) connection_made # unused function (test/functional/test_framework/mininode.py) [...] → cat test/lint/lint-python-dead-code-whitelist | LC_ALL=C sort BadInputOutpointIndex # unused class (test/functional/data/invalid_txs.py) DuplicateInput # unused class (test/functional/data/invalid_txs.py) InvalidOPIFConstruction # unused class (test/functional/data/invalid_txs.py) NonexistentInput # unused class (test/functional/data/invalid_txs.py) OutputMissing # unused class (test/functional/data/invalid_txs.py) SizeTooSmall # unused class (test/functional/data/invalid_txs.py) SpendNegative # unused class (test/functional/data/invalid_txs.py) SpendTooMuch # unused class (test/functional/data/invalid_txs.py) TooManySigops # unused class (test/functional/data/invalid_txs.py) _.carbon_path # unused attribute (contrib/macdeploy/custom_dsstore.py) _.converter # unused attribute (test/functional/test_framework/test_framework.py) [...]

fa3c657 lint: Add false positive to python dead code linter (MarcoFalke) fa25668 test: Test p2sh-witness and bech32 in wallet_import_rescan (MarcoFalke) fa79af2 test: Replace fragile "rng" with call to random() (MarcoFalke) fac3dcf test: Generate one block for each send in wallet_import_rescan (MarcoFalke) Pull request description: This adds test coverage for segwit in the `wallet_import_rescan` test, among other cleanups. ACKs for top commit: jnewbery: ACK fa3c657 Tree-SHA512: 877741763c62c1bf9d868864a1e3f0699857e8c028e9fcd65c7eeb73600c22cbe97b7b51093737743d9e87bcb991c1fe1086f673e18765aef0fcfe27951402f0

ryanofsky

post-merge utACK fa3c657. Sorry for not getting to this. I looked a while ago but was thrown off by the first commit, because it seemed to be making the test slower and more complicated for no reason, but it's very well explained now, and in general all the new improvements and suggestions are really nice.

…ort_rescan fa3c657 lint: Add false positive to python dead code linter (MarcoFalke) fa25668 test: Test p2sh-witness and bech32 in wallet_import_rescan (MarcoFalke) fa79af2 test: Replace fragile "rng" with call to random() (MarcoFalke) fac3dcf test: Generate one block for each send in wallet_import_rescan (MarcoFalke) Pull request description: This adds test coverage for segwit in the `wallet_import_rescan` test, among other cleanups. ACKs for top commit: jnewbery: ACK fa3c657 Tree-SHA512: 877741763c62c1bf9d868864a1e3f0699857e8c028e9fcd65c7eeb73600c22cbe97b7b51093737743d9e87bcb991c1fe1086f673e18765aef0fcfe27951402f0

…_import_rescan Summary: This ... * ensures that enough coins are available/spendable, even when more variants are added * ensures that all mempool txs are mined, even when more variants are added * makes the test more specific to test that the confirmation height is properly reported and timestamps are correctly handled in the test logic * prepares the test for a future, where blocks are skipped for rescan if they are deemed irrelevant by a filter (c.f. BIP157) This is a partial backport of Core [[bitcoin/bitcoin#16465 | PR16465]] - part 1 of 3 Commit bitcoin/bitcoin@fac3dcf Test Plan: `ninja && test/functional/test_runner.py wallet_import_rescan.py` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8054

Summary: This is a partial backport of Core [[bitcoin/bitcoin#16465 | PR16465]] - part 2 of 3 Commit bitcoin/bitcoin@fa79af2 Depends on D8054 Test Plan: `ninja && test/functional/test_runner.py wallet_import_rescan.py` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8055

Summary: The initial commit added tests that are not relevant for us, but also did some minor refactoring that could add more robustness if the tests evolve in the future (remember `extra_args` in `setup_network` in nodes are restarted). The initial commit introduced a bug later fixed in [[bitcoin/bitcoin#16920 | PR16920]]. This fix taken into accound here. This concludes backport of Core [[bitcoin/bitcoin#16465 | PR16465]] - part 3 of 3 Commit bitcoin/bitcoin@fa25668 Depends on D8055 Test Plan: `ninja && test/functional/test_runner.py wallet_import_rescan.py` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8056

…code linter

maflcko mentioned this pull request Jul 25, 2019

Use CWallet::Import* functions in all import* RPCs #16301

Merged

maflcko force-pushed the 1907-testAllAddressTypesImport branch 5 times, most recently from fabad6e to fabe6fa Compare July 25, 2019 21:44

DrahtBot added the Tests label Jul 25, 2019

maflcko force-pushed the 1907-testAllAddressTypesImport branch from fabe6fa to fa2a2ba Compare July 25, 2019 22:17

maflcko mentioned this pull request Aug 6, 2019

wallet: Fast rescan with BIP157 block filters #15845

Closed

maflcko closed this Aug 7, 2019

maflcko reopened this Aug 7, 2019

jnewbery reviewed Aug 14, 2019

View reviewed changes

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

maflcko force-pushed the 1907-testAllAddressTypesImport branch from fa2a2ba to fa97f2d Compare August 14, 2019 20:23

maflcko force-pushed the 1907-testAllAddressTypesImport branch from fa97f2d to fa2db10 Compare August 14, 2019 20:42

jnewbery reviewed Aug 14, 2019

View reviewed changes

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

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

MarcoFalke added 2 commits August 14, 2019 17:19

test: Replace fragile "rng" with call to random()

fa79af2

test: Test p2sh-witness and bech32 in wallet_import_rescan

fa25668

maflcko force-pushed the 1907-testAllAddressTypesImport branch from fa2db10 to faea912 Compare August 14, 2019 21:21

practicalswift reviewed Aug 15, 2019

View reviewed changes

test/lint/lint-python-dead-code.sh Outdated Show resolved Hide resolved

maflcko force-pushed the 1907-testAllAddressTypesImport branch 2 times, most recently from 716f00f to fbf2961 Compare August 15, 2019 11:58

maflcko force-pushed the 1907-testAllAddressTypesImport branch from fbf2961 to faea626 Compare August 15, 2019 12:02

lint: Add false positive to python dead code linter

fa3c657

maflcko force-pushed the 1907-testAllAddressTypesImport branch from faea626 to fa3c657 Compare August 15, 2019 12:05

jnewbery reviewed Aug 15, 2019

View reviewed changes

maflcko merged commit fa3c657 into bitcoin:master Aug 15, 2019

maflcko deleted the 1907-testAllAddressTypesImport branch August 15, 2019 14:33

ryanofsky reviewed Aug 15, 2019

View reviewed changes

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Sep 10, 2021

partial merge bitcoin#16465: lint: Add false positive to python dead …

8e5db79

…code linter

UdjinM6 pushed a commit to UdjinM6/dash that referenced this pull request Oct 5, 2021

partial merge bitcoin#16465: lint: Add false positive to python dead …

7aefec8

…code linter

pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021

partial merge bitcoin#16465: lint: Add false positive to python dead …

ea25e79

…code linter

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -0,0 +1,45 @@
		BadInputOutpointIndex # unused class (test/functional/data/invalid_txs.py)

test: Test p2sh-witness and bech32 in wallet_import_rescan #16465

test: Test p2sh-witness and bech32 in wallet_import_rescan #16465

Uh oh!

Conversation

maflcko commented Jul 25, 2019

Uh oh!

maflcko commented Jul 25, 2019

Uh oh!

DrahtBot commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Coverage

Uh oh!

Sjors commented Jul 26, 2019

Uh oh!

maflcko commented Jul 26, 2019

Uh oh!

Sjors commented Aug 6, 2019

Uh oh!

practicalswift commented Aug 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Aug 14, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Aug 15, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 15, 2019

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jul 26, 2019 •

edited

Loading

practicalswift commented Aug 7, 2019 •

edited

Loading