Page MenuHomePhabricator

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

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

Details

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
Branch
arcpatch-D3880_1
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7582
Build 13204: Bitcoin ABC Buildbot (legacy)
Build 13203: arc lint + arc unit

Event Timeline

markblundeberg created this object with visibility "Custom Policy".
markblundeberg added reviewers: jasonbcox, Fabien.

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

resubmit from arc (with lint changes)

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
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
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

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 :)

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

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.

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

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 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.
test/functional/abc-wallet-standardness.py
5 ↗(On Diff #14587)

need to update this too

rebase to master and tweak a few comments

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
deadalnix changed the visibility from "Custom Policy" to "Public (No Login Required)".Dec 6 2019, 18:38