Page MenuHomePhabricator
Differential D3880

Add wallet acceptance / mempool acceptance tests for non-standard variants
ClosedPublic
Actions

Authored by markblundeberg on Aug 14 2019, 23:10.

Details

Reviewers
deadalnix
jasonbcox
Fabien
Group Reviewers
Restricted Project
Commits
rSTAGING25541e4824e5: Add wallet acceptance / mempool acceptance tests for non-standard variants
rABC25541e4824e5: Add wallet acceptance / mempool acceptance tests for non-standard variants
Summary

We want to make sure that everything that is considered 'spendable' by
the wallet will not cause the generation of a nonstandard transaction when
the wallet tries to actually spend it.

Test Plan
./test_runner.py abc-wallet-nonminpush

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markblundeberg created this revision.Aug 14 2019, 23:10
markblundeberg created this object with visibility "Custom Policy".
Herald added a reviewer: deadalnix. · View Herald TranscriptAug 14 2019, 23:10
markblundeberg edited the summary of this revision. (Show Details)Aug 14 2019, 23:11
markblundeberg added a parent revision: D3865: recognize bare multisigs as standard only when using minimal pushes.
markblundeberg added reviewers: jasonbcox, Fabien.
markblundeberg removed a parent revision: D3872: make CheckMinimalPush available to codebase.

This is a variant of D3818 that tests all the forms, and does some extra testing on each one.

markblundeberg updated this revision to Diff 10805.Aug 14 2019, 23:15

resubmit from arc (with lint changes)

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 14 2019, 23:15
jasonbcox requested changes to this revision.Aug 14 2019, 23:34
jasonbcox added inline comments.
test/functional/abc-wallet-nonminpush.py
2 ↗(On Diff #10804)

heh. my bad from D3818. s/Core //g

81 ↗(On Diff #10804)

note: you can actually test if the tx is in the mempool. it may be a little verbose, but it can't hurt to try that here.

88 ↗(On Diff #10804)

No matter how many times I look at this, woutpoints looks like a typo to me. Please consider a more verbose name to make it clear.

100 ↗(On Diff #10804)

spaces around *

115 ↗(On Diff #10804)

This comment should probably come before the assert, otherwise it looks out of place.

131 ↗(On Diff #10804)

Trying to be more consistent with D3865 and other places, these pubkey tests should appear before the pubkeyhash tests.

This revision now requires changes to proceed.Aug 14 2019, 23:34
markblundeberg updated this revision to Diff 10808.Aug 15 2019, 02:33

update per comments

markblundeberg changed the visibility from "Custom Policy" to "Custom Policy".Aug 15 2019, 04:26
jasonbcox changed the visibility from "Custom Policy" to "Custom Policy".Aug 15 2019, 16:23
teamcity added a comment.Aug 15 2019, 16:23

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 15 2019, 16:29

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7158: Diff 10808.Aug 15 2019, 16:29
Fabien requested changes to this revision.Aug 15 2019, 19:39
Fabien added inline comments.
test/functional/abc-wallet-nonminpush.py
50 ↗(On Diff #10808)

shouldBeStandard and shouldBeInWallet should be the same variable

This revision now requires changes to proceed.Aug 15 2019, 19:39
markblundeberg updated this revision to Diff 10821.Aug 15 2019, 22:19

rebase for landed parent

teamcity added a comment.Aug 15 2019, 22:19

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg added inline comments.Aug 15 2019, 22:22
test/functional/abc-wallet-nonminpush.py
50 ↗(On Diff #10808)

oh, sorry, just noticed this...

I'm not totally sure about that, for example once we backport this then bare multisigs will be standard but not accepted into wallet:
https://github.com/bitcoin/bitcoin/pull/13002

teamcity added a comment.Aug 15 2019, 22:27

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7168: Diff 10821.Aug 15 2019, 22:27
Fabien accepted this revision.Aug 15 2019, 22:33
Fabien added inline comments.
test/functional/abc-wallet-nonminpush.py
50 ↗(On Diff #10808)

Good point, then this is not a bug this is a feature :)

markblundeberg planned changes to this revision.Aug 18 2019, 05:56

Going to pull this off review queue for now, since the intent is to delay landing this until some time later.

markblundeberg updated this revision to Diff 13131.Sep 25 2019, 01:46

rebase & revive

teamcity added a comment.Sep 25 2019, 01:46

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Sep 25 2019, 01:55

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7582: Diff 13131.Sep 25 2019, 01:55
jasonbcox added a comment.Sep 25 2019, 17:20

Update on my side: we're waiting for one last ETA from a responsible disclosure. I will green this once they have applied a fix.

deadalnix added a comment.Oct 6 2019, 21:20

Ping ?

jasonbcox added a comment.Oct 15 2019, 19:02

No word from them yet, though they're expected to do a release soon. Pinging them again.

jasonbcox added a comment.Nov 20 2019, 19:16

A release has gone out, but the fix wasn't included. I'm urging them to give us an actual ETA, because we need to land this soon.

markblundeberg updated this revision to Diff 14280.Nov 20 2019, 23:13

bump (rebased)

teamcity added a comment.Nov 20 2019, 23:13

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Nov 20 2019, 23:27

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8205: Diff 14280.Nov 20 2019, 23:27
markblundeberg updated this revision to Diff 14587.EditedDec 3 2019, 01:11
markblundeberg retitled this revision from Added wallet acceptance / mempool acceptance tests for non-minpush variants of standard scriptpubkeys to Add wallet acceptance / mempool acceptance tests for non-standard variants.
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)
markblundeberg removed a reviewer: Restricted Project.

a number of updates

  • added a couple of behaviours that are nonstandard to fund but the wallet will pick up, and will spend successfully.
  • rename test, the general idea is to make sure wallet is not DoSable.
Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 3 2019, 01:11
teamcity added a comment.Dec 3 2019, 01:11

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg added inline comments.Dec 3 2019, 01:13
test/functional/abc-wallet-standardness.py
5 ↗(On Diff #14587)

need to update this too

markblundeberg updated this revision to Diff 14589.Dec 3 2019, 01:20

rebase to master and tweak a few comments

teamcity added a comment.Dec 3 2019, 01:20

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 3 2019, 01:25

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8383: Diff 14587.Dec 3 2019, 01:25
teamcity added a comment.Dec 3 2019, 01:38

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8384: Diff 14589.Dec 3 2019, 01:38
jasonbcox accepted this revision.Dec 5 2019, 18:30

Let's land this today.

test/functional/abc-wallet-standardness.py
89 ↗(On Diff #14589)

Ah this is kind of a neat syntax element. I'll need to use this more :)

This revision is now accepted and ready to land.Dec 5 2019, 18:30
markblundeberg updated this revision to Diff 14651.Dec 5 2019, 18:32

rebase

teamcity added a comment.Dec 5 2019, 18:32

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Dec 5 2019, 18:42

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8428: Diff 14651.Dec 5 2019, 18:42
Closed by commit rABC25541e4824e5: Add wallet acceptance / mempool acceptance tests for non-standard variants (authored by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyDec 5 2019, 19:16
This revision was automatically updated to reflect the committed changes.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rABC25541e4824e5: Add wallet acceptance / mempool acceptance tests for non-standard variants.
deadalnix changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 6 2019, 18:38
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rSTAGING25541e4824e5: Add wallet acceptance / mempool acceptance tests for non-standard variants.Apr 7 2020, 21:59

Revision Contents
Changeset List

PathSize
test/
functional/
178 lines

Diff 14657

View Options

test/functional/abc-wallet-standardness.py

Loading...