Add several policy limits and disable uncompressed keys for segwit scripts

jl2012 commented at 4:24 pm on August 11, 2016: contributor

(See #8499 (comment) : updated on 8 Oct 2016)

btcdrak commented at 7:56 pm on August 11, 2016: contributor

Concept ACK

jl2012 commented at 0:50 am on August 12, 2016: contributor

I think this is only a temporary solution to protect some of the most common script types. It’s easy for P2WPKH. For P2WSH, it could only be done on a case-by-case basis. This approach won’t work for more complicated scripts like MAST

paveljanik commented at 11:33 am on August 12, 2016: contributor

*segwit.py tests fail.

jl2012 force-pushed on Aug 18, 2016

jl2012 force-pushed on Aug 19, 2016

jl2012 commented at 3:51 am on August 19, 2016: contributor

Updated to do more sanity checks:

If the public key in P2WPKH is bigger than 33 bytes, make sure it matches the witness program
Make sure the witnessScript is <= 10000 bytes and matches the witness program

jl2012 force-pushed on Aug 19, 2016

in src/policy/policy.cpp: in 47335e4b40 outdated

195+                return true;
196+            std::vector<unsigned char> hashPubKey(20);
197+            std::vector<unsigned char> pubKey = tx.wit.vtxinwit[i].scriptWitness.stack[1];
198+            if (tx.wit.vtxinwit[i].scriptWitness.stack[0].size() > 73 || pubKey.size() > 65)
199+                return true;
200+            if (pubKey.size() > 33) {

sipa commented at 10:58 am on August 22, 2016:

Why only when the size is above 33 bytes?

jl2012 commented at 11:28 am on August 22, 2016:

The main objective of this PR is to detect extra witness data in some known forms of scripts, and DoS ban if found.

The normal size for compressed key is 33. If the size is 33, we are sure that no extra data is added. Whether the provided key matches the hash or not will be tested later in script evaluation. Testing here is just duplicated work.

If the size is bigger than 33, we don’t know whether it is legit (uncompressed key) or not (malleated witness). To exclude the invalid case we must compare the hash early, and DoS ban if it does not match. If we don’t compare the hash here, an attacker may increase the key size up to 65 bytes, get the transaction rejected due to insufficient fee, and not get banned

instagibbs commented at 1:56 pm on September 2, 2016:

Unless I’m missing something, I think this is over-eager optimization. It took me a few reads of even this explanation to understand the reasoning behind this line, and even then I’m not quite sure if I’ve fully understood it. Is the only harm in being more general that we are duplicating work later?

instagibbs commented at 2:09 pm on September 2, 2016:

At a minimum there should be a comment something like: “We want to make sure malleated pubkeys don’t trigger too-low-fee to avoid DoS ban”

jl2012 commented at 3:43 pm on September 4, 2016:

@instagibbs >Is the only harm in being more general that we are duplicating work later? yes, because the only purpose of this PR is to early detect some kinds of witness mutation that would trigger too-low-fee ban. We don’t care if the size is same as expected or too small. Otherwise we are just doing the validation twice.

But this one is probably over optimized.

sipa commented at 12:44 pm on September 5, 2016:

I believe this is over-optimized, yes. Let’s just always check it.

in src/policy/policy.cpp: in 47335e4b40 outdated

214+            if (witnessScript.size() > 10000)
215+                return true;
216+            CSHA256().Write(begin_ptr(witnessScript), witnessScript.size()).Finalize(hashWitnessScript.begin());
217+            if (memcmp(hashWitnessScript.begin(), &witnessprogram[0], 32))
218+                return true;
219+            // more P2WSH tests could be added here

jl2012 commented at 2:23 pm on August 22, 2016:

More tests could be added here to protect some common scripts. For example, canonical n-of-m multisig must have exactly n + 1 stack items (excluding the witnessScript). The first one must be empty (assuming BIP146) and the followings must be <= 73 bytes. Otherwise we kick the peer. Some common HTLC could be protected in a similar way, until there is a more permanent solution

Non-canonical scripts are still accepted but they could be vulnerable to malleated witness attack

in src/policy/policy.cpp: in 47335e4b40 outdated

212+            CScript witnessScript = CScript(tx.wit.vtxinwit[i].scriptWitness.stack.back().begin(), tx.wit.vtxinwit[i].scriptWitness.stack.back().end());
213+            uint256 hashWitnessScript;
214+            if (witnessScript.size() > 10000)
215+                return true;
216+            CSHA256().Write(begin_ptr(witnessScript), witnessScript.size()).Finalize(hashWitnessScript.begin());
217+            if (memcmp(hashWitnessScript.begin(), &witnessprogram[0], 32))

jl2012 commented at 2:25 pm on August 22, 2016:

Comparing the hash here guarantees that the script size and sigOpCount must be correct. And we could further observe the script with static analysis

sipa commented at 11:34 am on August 23, 2016: member

I don’t think this belongs in policy. It detects witness transaction inputs that are unambiguously invalid by consensus rules, so I think it belongs in script/interpreter.cpp, but for that it can’t depend on CCoinsView. Would it be possible to just have a function per input, that just takes the prevout, scriptsig, and scriptwitness?

jl2012 commented at 5:44 pm on August 23, 2016: contributor

@sipa: can I still use Solver and txnouttype after moving to interpreter.cpp? That’d would useful when we define more common script types (e.g. HTLC)

The IsBadWitness function is only a very small subset of consensus rules and it should never be used in the consensus critical path. Actually, we don’t need IsBadWitness if we fully execute the script before doing any size related policy check. So it may be reasonable to consider this as part of policy.

This is more like an “assertion” for the following policy checking, rather than for consensus purpose

jl2012 force-pushed on Aug 24, 2016

jl2012 renamed this:
~~[Untested] Check bad witness~~
[WIP] Check bad witness
on Aug 24, 2016

jl2012 renamed this:
~~[WIP] Check bad witness~~
[rpc-tests needed] Check bad witness
on Aug 25, 2016

jl2012 commented at 2:49 am on August 25, 2016: contributor

need a 0.13.1 tag?

laanwj added this to the milestone 0.13.1 on Sep 1, 2016

jl2012 force-pushed on Sep 6, 2016

jl2012 commented at 9:06 am on September 6, 2016: contributor

The function is updated. It returns 1 for witness-related error, and 2 for P2SH-related error. We shouldn’t mark corruption possible for P2SH related error.

It also always checks the HASH160 in P2WPKH

in src/policy/policy.cpp: in cf802ffc86 outdated

153@@ -154,6 +154,105 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
154     return true;
155 }
156 
157+int IsBadWitness(const CTransaction& tx, const CCoinsViewCache& mapInputs)

sipa commented at 9:09 am on September 6, 2016:

Can you define an enum instead?

jl2012 commented at 11:28 am on September 6, 2016:

Revised

jl2012 force-pushed on Sep 6, 2016

sipa commented at 12:01 pm on September 6, 2016: member

utACK 056dc99bdc538a7af04b21fe3b25586afbc03861

in src/policy/policy.cpp: in 056dc99bdc outdated

209+            // P2WSH requires at least 1 witness stack item but we already know the witness is not empty.
210+            // Make sure the witnessScript size is <= 10000 bytes and matches witness program.
211+            if (witnessversion == 0 && witnessprogram.size() == 32) {
212+                CScript witnessScript = CScript(tx.wit.vtxinwit[i].scriptWitness.stack.back().begin(), tx.wit.vtxinwit[i].scriptWitness.stack.back().end());
213+                uint256 hashWitnessScript;
214+                if (witnessScript.size() > 10000)

instagibbs commented at 5:40 pm on September 6, 2016:

Please use the constant found here: https://github.com/bitcoin/bitcoin/blob/master/src/script/script.h#L31

instagibbs commented at 5:40 pm on September 6, 2016: member

nit aside utACK https://github.com/bitcoin/bitcoin/pull/8499/commits/056dc99bdc538a7af04b21fe3b25586afbc03861

jl2012 force-pushed on Sep 8, 2016

jl2012 commented at 8:24 am on September 8, 2016: contributor

08f5e22: addressing nits and include rpc-test

jl2012 renamed this:
~~[rpc-tests needed] Check bad witness~~
Check bad witness
on Sep 8, 2016

jl2012 force-pushed on Sep 9, 2016

jl2012 commented at 8:42 pm on September 9, 2016: contributor

(Ready for review)

This PR includes several policy rules for segwit:

It detects some types of bad witness injected by malicious relay nodes, before the fee and SigOpCount is calculated. All the rules examined are consensus critical. Nodes sending violating transactions will be banned. See also: #8279, #8525
It a policy limit for P2WSH, with witnessScript <= 3600 bytes, witness stack item size <= 80 bytes, and witness stack items <= 100 3600 bytes witnessScript and 100 stack items are adequate for a n-of-100 multisig using OP_CHECKSIG, OP_ADD, and OP_EQUAL The max size for ECDSA signature is 73 bytes and nothing should use more than that with the current scripting language. This is to prevent abuse of witness space, and reduce the risks of DoS attack with some unknown special and big scripts. Alternative to #8685
It redefines SCRIPT_VERIFY_STRICTENC to require that only compressed keys are allowed in segwit scripts. This is a policy only and non-compressed keys are still valid in a block. We can’t softfork non-compressed keys away for non-segwit scripts since there are many UTXOs being stored this way. Since segwit is a completely new script system, there is no strong reasons to support non-compressed keys.
As a consequence of 3, addwitnessaddress should not return a witness address if the key is an uncompressed one.

jl2012 renamed this:
~~Check bad witness~~
Check bad witness and implement several policy limits for segwit scripts
on Sep 9, 2016

rubensayshi commented at 8:03 am on September 13, 2016: contributor

utACK

btcdrak commented at 2:55 pm on September 13, 2016: contributor

Tested ACK 603fa20

in src/policy/policy.h: in 603fa2071f outdated

86+enum BadTransaction
87+{
88+    TX_MAYBE_OK = 0,
89+    TX_BAD_WITNESS = 1,
90+    TX_BAD_P2SH = 2,
91+    TX_NONSTD_BIG_P2WSH = 3,

instagibbs commented at 6:08 pm on September 13, 2016:

Please describe this enum value in the comment above

in src/policy/policy.cpp: in 603fa2071f outdated

225+                 * Count the witness stack size without the witnessScript.
226+                 *
227+                 * Max standard witness stack size is 100.
228+                 * Due to the implicit CLEANSTACK rule and nOpCount limit of 201, the maximum consensus valid
229+                 * witness stack size is 412, with 9 OP_CHECKMULTISIGVERIFY and 12 OP_2DROP in witnessScript.
230+                 * Anything bigger than 412 must not leave a single true value after execution.

instagibbs commented at 6:12 pm on September 13, 2016:

/must/can/ ?

in src/policy/policy.cpp: in 603fa2071f outdated

223+
224+                /*
225+                 * Count the witness stack size without the witnessScript.
226+                 *
227+                 * Max standard witness stack size is 100.
228+                 * Due to the implicit CLEANSTACK rule and nOpCount limit of 201, the maximum consensus valid

instagibbs commented at 6:33 pm on September 13, 2016:

Could you spell out the math here? I’m not a Script wizard and can’t figure what’s being asserted here.

jl2012 commented at 7:28 pm on September 13, 2016:

There is a 201 op limit in a script. Except CHECKMULTISIG(CMS), 2DROP is the most efficient code to clean up the stack (1 opcode to clean 2 items). A 20-of-20 CMS will clean up 1 dummy +20 sig + 1 nSig + 20 key + 1nKey = 43 items, and it is equivalent to 21 ops, so it’s more efficient (1 to 2.05 items).

Therefore, I could have a witnessScript = 9 CMSV + 12 2DROP, which is exactly 201 ops. They will clean up 411 items. So when I put one more item the script to evaluate to true. (total = 412)

jl2012 commented at 7:41 pm on September 13, 2016:

Sorry, I think the max is 100 CHECKMULTISIGVERIFY for 502 items. Will correct it or just use a bigger number like 1000. The propose is just anti-DoS so the actual number is not very important

jl2012 commented at 6:33 am on September 14, 2016: contributor

Comments added and max stack item calculation corrected.

There is a 201 nOpCount limit per script. n-of-m CHECKMULTISIG(VERIFY) is counted as m+1 nOp. 0-of-0 CHECKMULTISIG(VERIFY), despite totally useless, is valid and counts as 1 nOp. It removes 3 items from stack: the dummy item (must be 0 if NULLDUMMY is active), nSig (0 in this case), and nKey (also 0 in this case). This is the only way to remove more than 2 stack items with only 1 nOpCount.

Therefore, the worst case is 201 CHECKMULTISIGVERIFY, which remove 603 zero-value items, plus 1 true-value item at the beginning of stack so the evaluation will be true.

Therefore, if the stack size is above 604, we know that the evaluation must fail even without running the script, due to the implicit CLEANSTACK rule in segwit.

This example is demonstrated in p2p-segwit.py

(Other opcodes remove 2 items at most, including 2DROP, CHECKSIGVERIFY, EQUALVERIFY, NUMEQUALVERIFY. Multisig configurations other than 0-of-0 are worse stack cleaner. For example, 1-of-1 will clean 5 items, but is counted as 2 nOp)

MarcoFalke added the label Needs backport on Sep 15, 2016

in src/main.cpp: in 9199ff2ba9 outdated

1257@@ -1258,6 +1258,18 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1258         if (fRequireStandard && !AreInputsStandard(tx, view))
1259             return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
1260 
1261+        // Check for bad witness
1262+        bool fNonStandardWitness = false;
1263+        if (!tx.wit.IsNull()) {

NicolasDorier commented at 8:22 am on September 17, 2016:

I would do if(witnessEnabled && !tx.wit.IsNull()), as with it you can see what segwit is impacting by following where witnessEnabled is used. It makes review easier. (NIT)

jl2012 commented at 7:34 pm on September 17, 2016:

Addressed with ec344d9

in src/script/interpreter.cpp: in 9199ff2ba9 outdated

944@@ -941,7 +945,7 @@ bool EvalScript(vector<vector<unsigned char> >& stack, const CScript& script, un
945                         // Note how this makes the exact order of pubkey/signature evaluation
946                         // distinguishable by CHECKMULTISIG NOT if the STRICTENC flag is set.
947                         // See the script_(in)valid tests for details.
948-                        if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, serror)) {
949+                        if (!CheckSignatureEncoding(vchSig, flags, serror) || !CheckPubKeyEncoding(vchPubKey, flags, sigversion, serror)) {

NicolasDorier commented at 8:31 am on September 17, 2016:

Better creating a new ScriptFlags rather than hijacking an existing one. That we you won’t have to add a parameter to CheckPubkeyEncoding.

jl2012 commented at 7:52 pm on September 17, 2016:

Even with a new flag, the sigversion is still required in CheckPubkeyEncoding because applicability of the rules is based on the type of tx (base vs witness), not the flag.

However, a new error message type is added to correctly describe this error. (516f9f1)

NicolasDorier commented at 1:10 am on September 18, 2016:

Another reason to use a flag would be to enforce this rule at policy level for old type the other sigversion. But maybe this would need to be in another PR.

jl2012 commented at 12:30 pm on September 18, 2016:

Many people are still using and will use uncompressed keys. We could never have any policy to reject that

in src/policy/policy.cpp: in 9199ff2ba9 outdated

187+
188+            int witnessversion = 0;
189+            std::vector<unsigned char> witnessprogram;
190+
191+            // Non-witness program must not be associated with any witness
192+            if (!prevScript.IsWitnessProgram(witnessversion, witnessprogram) && !tx.wit.vtxinwit[i].IsNull())

NicolasDorier commented at 9:02 am on September 17, 2016:

&& !tx.wit.vtxinwit[i].IsNull() is not needed, you do already the check above.

jl2012 commented at 7:34 pm on September 17, 2016:

Addressed with ec344d9

in src/wallet/rpcwallet.cpp: in 9199ff2ba9 outdated

1048@@ -1045,6 +1049,18 @@ class Witnessifier : public boost::static_visitor<bool>
1049                 result = scriptID;
1050                 return true;
1051             }
1052+            txnouttype typ;
1053+            std::vector<std::vector<unsigned char> > vSolutions;
1054+            if (Solver(subscript, typ, vSolutions) && typ == TX_MULTISIG) {
1055+                for (size_t i = 1; i < vSolutions.size() - 1; i++) {
1056+                    if (vSolutions[i].size() != 33)

NicolasDorier commented at 9:47 am on September 17, 2016:

I would prefer you create a function for that, as you are repeating it twice.

jl2012 commented at 7:26 pm on September 17, 2016:

the types are different. One is CPubKey and one is std::vector

NicolasDorier commented at 1:20 am on September 18, 2016:

I think you should add empty signature as standard signature.

jl2012 commented at 6:38 am on September 18, 2016:

This is pub key, not signature.

jl2012 force-pushed on Sep 17, 2016

in src/script/interpreter.cpp: in 516f9f114c outdated

203+bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags, const SigVersion &sigversion, ScriptError* serror) {
204     if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && !IsCompressedOrUncompressedPubKey(vchSig)) {
205         return set_error(serror, SCRIPT_ERR_PUBKEYTYPE);
206     }
207+    // Only compressed keys are accepted in segwit
208+    if ((flags & SCRIPT_VERIFY_STRICTENC) != 0 && sigversion == SIGVERSION_WITNESS_V0 && vchSig.size() != 33) {

NicolasDorier commented at 12:56 pm on September 18, 2016:

Does it makes sense to check flags & SCRIPT_VERIFY_STRICTENC at all ? if segwit is activated (and it is if the sigversion is SIGVERSION_WITNESS_V0 ) then SCRIPT_VERIFY_STRICTENC is enforced anyway by BIP66. To be sure it is always the case, we can add flags |= SCRIPT_VERIFY_DERSIG at https://github.com/bitcoin/bitcoin/blob/39ac1ec6426447b924052c2da3f80e0220c308c3/src/main.cpp#L2388

It would be an hard fork is there was a way to get an alt chain activating segwit before bip66… but I don’t think it is reasonable to think it is possible now. If it was, it would be bigger problem.

jl2012 commented at 1:16 pm on September 18, 2016:

BIP66 enforced DERSIG, not STRICTENC. STRICTENC is a pure policy and will never be a consensus rule.

Even in a parallel universe that DERSIG is enforced after WITNESS, they are still softforks. And this is actually impossible, since BIP9 will activate BIP66. Anyway, this is off-topic

NicolasDorier commented at 2:37 am on September 19, 2016:

oh indeed I always inverse them!

jl2012 force-pushed on Sep 23, 2016

jl2012 commented at 9:59 am on September 23, 2016: contributor

Rebased and squashed

in src/script/interpreter.cpp: in d216177332 outdated

198@@ -199,10 +199,14 @@ bool CheckSignatureEncoding(const vector<unsigned char> &vchSig, unsigned int fl
199     return true;
200 }
201 
202-bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags, ScriptError* serror) {
203+bool static CheckPubKeyEncoding(const valtype &vchSig, unsigned int flags, const SigVersion &sigversion, ScriptError* serror) {

instagibbs commented at 3:28 pm on September 23, 2016:

nit: I wouldn’t complain if you renamed vchSig to vchPubKey…

instagibbs approved

instagibbs commented at 3:58 pm on September 23, 2016: member

re-utACK https://github.com/bitcoin/bitcoin/pull/8499/commits/a4aa597b94a0d6ec0006e61cec279946d3695806

instagibbs commented at 3:58 pm on September 23, 2016: member

tests are failing for some reason?

jl2012 commented at 4:19 pm on September 23, 2016: contributor

@instagibbs , oh it failed the newly merged NULLDUMMY softfork. Will fix

jl2012 force-pushed on Sep 23, 2016

sdaftuar commented at 7:21 pm on September 23, 2016: member

Hmm, so I don’t know if everyone is already sold on the approach here, but conceptually, I’m not sure the benefits outweigh the costs of introducing the BAD_WITNESS and BAD_P2SH checks with their associated DoS-banning.

In order for the banning to be a reasonable thing to do, it should really only occur for things that would be invalid-according-to-consensus, or else we’d risk partitioning some software off the network over differences in node policy. So while I don’t think it’s likely, there are some negative consequences if somehow something gets flagged as BAD_WITNESS that actually passes consensus (eg the script with higher stack:opcount ratio @jl2012 came up with while discussing this above #8499 (comment)).

Whereas if we get this right, then the benefit is merely that nodes have one fewer method to waste our resources (after #8525, there should be no network-wide effects of an attacker relaying malleated segwit transactions). Given the wide variety of other ways our resources can be wasted, this seems like a small benefit for the complexity that would be introduced (as well as code and logic duplication – eg I believe the BAD_P2SH+subsequent ban behavior is duplicative of the existing ban-behavior if a peer sends us an invalid P2SH?).

I suggest that we drop the node-banning behavior in this PR, and simplify IsBadWitness to only check for the proposed policy changes (number of witness program script elements, size of witness script, size of script elements), rather than look for harder-to-reason-about characteristics that we think the consensus rules imply. This would substantially reduce the negative consequences just in case we got something wrong and reduce the amount of code in this PR, which taken together would make the changes easier to review.

Then in the future, I’d suggest we pursue something in the direction of #8593, where we could try to limit peer misbehavior using our existing calls into consensus functions, and in particular without introducing a lot of new code that tries to re-implement consensus calculations.

Anyway, I know there are some ACKs here already, so if everyone is comfortable with this code and approach as-is then I don’t object too strongly; I just wanted to lay out my reasons for preferring an alternate approach.

jl2012 commented at 8:50 am on September 24, 2016: contributor

@sdaftuar I think your suggestions make sense as long as #8525 does what it is supposed to do. Especially, since IsStandardTx may reject a tx based on its weight, and this is done before the IsBadWitness check, the IsBadWitness is actually not very useful (unless we reject based on the witness-stripped size).

Anyway, I have created a policy-only version based on your suggestions, which has a much smaller diff: https://github.com/jl2012/bitcoin/commits/badwitnesscheck-new If there is no objection I will move on to this direction

(p.s. you may notice that this PR was proposed before #8525, and therefore I was not aware of other approaches to fix this problem)

jl2012 commented at 6:00 pm on September 25, 2016: contributor

@btcdrak @sipa @instagibbs @rubensayshi @NicolasDorier As you have reviewed this PR, do you agree with @sdaftuar ’s #8499 (comment) to remove the DoS-banning parts in this PR?

I have created a policy-only version, which has a much smaller diff: https://github.com/jl2012/bitcoin/commits/badwitnesscheck-new If it’s ok I will replace it so we could move on.

instagibbs commented at 8:13 pm on September 26, 2016: member

@jl2012 I like less code when possible, and DoS banning should indeed be done sparingly. concept ACK on the alternative version.

sipa commented at 3:24 pm on September 27, 2016: member

@jl2012 utACK f55cee5b91c9dfe11437e647e69046e64cbf851c (from https://github.com/jl2012/bitcoin/commits/badwitnesscheck-new)

jl2012 force-pushed on Sep 27, 2016

jl2012 commented at 3:46 pm on September 27, 2016: contributor

replaced with f55cee5 (policy only)

jl2012 renamed this:
~~Check bad witness and implement several policy limits for segwit scripts~~
Add several policy limits for segwit scripts
on Sep 27, 2016

btcdrak commented at 4:04 pm on September 27, 2016: contributor

Prefer the policy only version.

utACK f55cee5

instagibbs commented at 6:54 pm on September 27, 2016: member

utACK ~~https://github.com/bitcoin/bitcoin/pull/8499/commits/fdb47150c55bcaf0f0569a77ece0abacadbfef38~~ https://github.com/bitcoin/bitcoin/commit/f55cee5b91c9dfe11437e647e69046e64cbf851c

in src/policy/policy.cpp: in fdb47150c5 outdated

161+
162+    for (unsigned int i = 0; i < tx.vin.size(); i++)
163+    {
164+        // We don't care if witness for this input is empty, since it must not be bloated.
165+        // If the script is invalid without witness, it would be caught sooner or later during validation.
166+        if (!tx.wit.vtxinwit[i].IsNull()) {

NicolasDorier commented at 2:57 am on September 28, 2016:

nit: would prefer early exit of this iteration with continue; it makes review easier by not having to check what happen if it is indeed null.

CodeShark commented at 1:01 pm on September 30, 2016:

Agree with @NicolasDorier

jl2012 commented at 2:10 pm on September 30, 2016:

done with 8d7146c

NicolasDorier commented at 3:04 am on September 28, 2016: contributor

utACK fdb47150c55bcaf0f0569a77ece0abacadbfef38

gmaxwell commented at 9:22 am on September 28, 2016: contributor

utACK, testing now on testnet with RequireStandard = true;

laanwj commented at 10:14 am on September 28, 2016: member

utACK f55cee5

in src/main.cpp: in fdb47150c5 outdated

1519@@ -1520,6 +1520,10 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
1520                 __func__, hash.ToString(), FormatStateMessage(state));
1521         }
1522 
1523+        // Check for non-standard witness in P2WSH after transaction is validated

sdaftuar commented at 7:30 pm on September 28, 2016:

Why is this being checked after transaction validation? I thought that the goal of these segwit policy limits was to mitigate potential DoS risk from unwieldy scripts that we haven’t thought of; is there any downside to having this happen before transaction validation?

jl2012 commented at 8:00 pm on September 28, 2016:

It’s about the #8279. But since this is just one of the many ways to mutate the witness and we won’t ban the peer anyway, maybe it makes more sense to move it before validation and after AreInputsStandard?

TheBlueMatt commented at 8:16 pm on September 28, 2016: member

The first and last commits seem reasonable to me (haven’t reviewed in detail yet), but I’m not sure about actually enforcing compressed pubkeys as a standardness rule. In general, I think we should be considering standardness rules only for a) things which we intend to soft fork or b) things which we are afraid of DoS risks from. Obviously uncompressed pubkeys dont particularly fit into category (b), but I’m not sure about (a). I’m really not a fan of the idea of soft-forking out something like uncompressed pubkeys in a future soft-fork: we dont really have strong motivation to do it and it risks (though not a ton, due to policy rules today) UTXOs becomming unspendable. IMO it should either be a day-1 part of segwit (I dont have a strong opinion, but I think consensus would be that this is maybe a bit late) or not happen right now.

instagibbs commented at 8:22 pm on September 28, 2016: member

On that note, could we get a justification for this PR’s commits up at the top? The linked one is out of date now anyways. Makes reviewing much easier.

sipa commented at 8:28 pm on September 28, 2016: member

@TheBlueMatt The compressed pubkeys rule is only applied to segwit scripts, so if it is nonstandard from the start it’s absolutely potentially softforkable in the future.

jl2012 commented at 4:44 am on September 29, 2016: contributor

This PR includes several policy rules for segwit:

It implements a policy limit for P2WSH, with witnessScript <= 3600 bytes, witness stack item size <= 80 bytes, and witness stack items <= 100
3600 bytes witnessScript and 100 stack items are adequate for a n-of-100 multisig using OP_CHECKSIG, OP_ADD, and OP_EQUAL
The max size for ECDSA signature is 73 bytes and nothing should use more than that with the current scripting language.
This is to prevent abuse of witness space, and reduce the risks of DoS attack with some unknown special and big scripts.
Alternative to #8685
It redefines SCRIPT_VERIFY_STRICTENC to require that only compressed keys are allowed in segwit scripts. This is a policy only and non-compressed keys are still valid in a block.
We can’t softfork non-compressed keys away for non-segwit scripts since there are many UTXOs being stored this way. Since segwit is a completely new script system, there is no strong reasons to support non-compressed keys.
If we implement it as a policy now, we may have a softfork later to disable non-compressed keys in segwit.
To avoid creation of unspendable address, addwitnessaddress should not return a witness address if the key is an uncompressed one.

TheBlueMatt commented at 1:41 pm on September 29, 2016: member

@sipa I do not agree that it is a good candidate for soft-fork in the future. There is little direct benifit from doing so and “it was always policy in Bitcoin Core to not let you spend this” is not sufficient to make some not-insane outputs unspendable to me. I’m happy to be overruled on this (because I dont think its a huge deal), but I would not advocate for such a soft-fork in the future.

jl2012 commented at 3:03 pm on September 29, 2016: contributor

2 commits added. 620fee0 is a minor fix for the test. 60ea68a is moving IsWitnessStandard before validation, as suggested by @sdaftuar. If we think it’s a better idea, I will squash it

sipa commented at 3:18 pm on September 29, 2016: member

@TheBlueMatt I don’t see any reason why we’d continue to support uncompressed pubkeys. They’re a waste of space for no benefit.

It would indeed have been better to include this in segwit’s proposed consensus rules, but it’s worth changing at this point I think. If we don’t use this opportunity now to get rid of them, we probably won’t ever be until a new script revision entirely.

TheBlueMatt commented at 3:42 pm on September 29, 2016: member

@sipa agreed on everything there except that its reasonable to soft-fork them out if we do make it non-standard now. It would absolutely be wonderful to get rid of them in segwit on day 1, but if we think its too late for that, then I think its too late, full stop. In any case, I think its marginally setting the wrong precedent (I dont think policy is a strong enough “you may not use this” flag to make UTXOs unspenable in non-obvious ways (ie not things like NOPcodes, etc) unless there is a really significant reason to), but if others actually feel strongly, then OK.

jl2012 force-pushed on Sep 29, 2016

jl2012 commented at 3:46 pm on September 29, 2016: contributor

Revised the new commits a bit. Now the tests in 0c4ef07 shows that moving IsWitnessStandard before validation wouldn’t blind a node to a transaction due to #8525

jl2012 commented at 3:53 pm on September 29, 2016: contributor

@sipa @TheBlueMatt I don’t have strong opinion about having the uncompressed key policy. Maybe one reason for doing it now is to procrastinate the debate about the softfork. I don’t think this is a big deal and we should try to decide in today’s meeting.

laanwj commented at 4:00 pm on September 29, 2016: member

I prefer not allowing uncompressed keys with segwit.

With normal transactions there’s the argument that uncompressed keys have to remain supported forever, because there may be old time-locked transactions that use them.

By never allowing them for segwit that won’t be an issue there.

Policy is not 100% strong but experience has shown that it is an effective way to discourage people from trying it in the first place.

gmaxwell commented at 5:06 pm on September 29, 2016: contributor

@TheBlueMatt

Look at p2sh usage– there is no ongoing uncompressed pubkey usage there. I expect effectively none for segwit. But because there has been uncompressed pubkey usage in p2sh, we’re stuck forever supporting it there and taking the costs of doing so. Several weeks ago we wanted to simply bar uncompressed keys with segwit and you argued that it was too late to add a consensus rule. OKAY, so a compromise– make it non-standard and state outright that it will be soft-forked out later. And now you’re arguing that it should be consensus enforced? :-/

The “don’t use it” comes not primarily from the standardness but from the widespread obvious intent to remove it. Making it non-standard makes the next step safe from griefers, and expresses more clearly what not to use. And every soft-fork takes some pattern of bytes from only-prohibited-by-policy to prohibited.

instagibbs commented at 1:23 pm on September 30, 2016: member

Unless there is a compelling use-case(or expectation thereof) for uncompressed pubkeys in Segwit, I think policy and eventual softfork is desired.

jtimon commented at 3:21 pm on September 30, 2016: contributor

I would also prefer not allowing uncompressed keys with segwit as consensus rule from the start. I disagree it is too late for that. But if other people do, I don’t mind having it as a default policy (I don’t really like saying “standardness rule”). Fast review ACK.

sdaftuar commented at 8:10 pm on September 30, 2016: member

ACK.

FYI I was able to hack up a test to check that uncompressed pubkeys with segwit are valid in blocks, though not accepted under default policy.

Regarding the uncompressed pubkey discussion: if people think we might want to softfork out uncompressed pubkeys in segwit altogether in the future, then perhaps it’s worth considering whether the policy should be implemented with its own validation flag rather than an extra behavior of STRICTENC? (I have no opinion on this myself.)

jl2012 commented at 2:59 am on October 1, 2016: contributor

I have a scenario but haven’t tested. I have an used uncompressed key in my wallet, say 0x04aabbccddeeff… . Someone finds that on the blockchain, and without asking me he tries to send a few mBTC to scriptPubKey = 0x00 Hash160(0x04aabbccddeeff…), would that show up in the balance of my account and would my wallet keep trying to spending it? If yes we need more fixes for the wallet

jl2012 force-pushed on Oct 1, 2016

jl2012 commented at 3:39 am on October 1, 2016: contributor

Added a new flag for compressed key policy and rebased

jl2012 force-pushed on Oct 1, 2016

jl2012 commented at 4:09 pm on October 1, 2016: contributor

fixed the issue I mentioned earlier (https://github.com/bitcoin/bitcoin/pull/8499#issuecomment-250888504) with deae55f . Squashed and rebased

in src/script/ismine.cpp: in 92e8bf8f96 outdated

87@@ -80,9 +88,8 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey)
88         CScriptID scriptID = CScriptID(hash);
89         CScript subscript;
90         if (keystore.GetCScript(scriptID, subscript)) {
91-            isminetype ret = IsMine(keystore, subscript);
92-            if (ret == ISMINE_SPENDABLE)
93-                return ret;
94+            if (AreMultisigKeysCompressed(subscript))

sipa commented at 4:12 pm on October 1, 2016:

I think you should keep the IsMine call.

jl2012 commented at 4:28 pm on October 1, 2016:

oh, you are right

jl2012 force-pushed on Oct 1, 2016

sipa commented at 3:13 pm on October 5, 2016: member

utACK 3a4048de42e351d302c202ee8879a8fd4706e7f2

jl2012 commented at 7:59 am on October 7, 2016: contributor

Added rpc tests by @sdaftuar, and some script tests

jl2012 force-pushed on Oct 7, 2016

NicolasDorier commented at 8:50 am on October 7, 2016: contributor

tACK 137a84e (replicated it on NBitcoin https://github.com/MetacoSA/NBitcoin/commit/b7425bcc564f593e3c0156dc21e4520e96b2716b)

jl2012 commented at 11:48 am on October 8, 2016: contributor

(71cd2df) This PR includes several policy rules for segwit:

It implements a policy limit for P2WSH, with witnessScript <= 3600 bytes, witness stack item size <= 80 bytes, and witness stack items <= 100
- 3600 bytes witnessScript and 100 stack items are adequate for a n-of-100 multisig using OP_CHECKSIG, OP_ADD, and OP_EQUAL
- The max size for ECDSA signature is 73 bytes and nothing should use more than that with the current scripting language.
- This is to prevent abuse of witness space, and reduce the risks of DoS attack with some unknown special and big scripts.
It creates a new verification flag SCRIPT_VERIFY_WITNESS_PUBKEYTYPE to require that only compressed keys are allowed in segwit scripts.
- At this moment, it is a policy only and non-compressed keys are still valid in a block.
- We can’t softfork non-compressed keys away for non-segwit scripts since there are many UTXOs being stored this way. Since segwit is a completely new script system, there is no strong reasons to support non-compressed keys.
- If we implement it as a policy now, we may have a softfork later to disable non-compressed keys in segwit.
To avoid creation of unspendable address, addwitnessaddress should not return a witness address if the key is an uncompressed one.
Bitcoin sent to a witness address with uncompressed keys should not be shown in the wallet balance. Signing is also disabled for such outputs.

droark commented at 12:37 pm on October 9, 2016: contributor

(CAVEAT: I need to confirm the exact state of the code. If I’m wrong, I’ll come back and delete or edit out this comment.)

Hello. Just FYI, it’s possible that, for now at least, this relay policy will break Armory. Right now, Armory supports only uncompressed keys for regular transactions. Compressed key support was going to be added eons ago but never happened. (Long story.) It’s still in the cards. The problem is that it requires a major wallet refactor, meaning it could be awhile.

I’m not trying to throw up a roadblock or anything like that. I’m just making an observation. Also, please keep in mind that I don’t know what Armory does under the hood to support SegWit. It may be that it uses only compressed keys, hence my caveat at the top.

achow101 commented at 2:16 pm on October 9, 2016: member

@droark This PR would indeed break Armory. We have discussed this on the IRC channel and there is already a solution in the works. The uncompressed keys will be dynamically converted to compressed keys when segwit scripts are being created.

jl2012 commented at 2:27 pm on October 9, 2016: contributor

@droark @achow101 The policy is for segwit txs only, and no existing released version of Armory support segwit. So any existing versions of Armory will work exactly as it works today, with or without this PR.

To support segwit, any existing wallets need some level of refactor anyway. Using uncompressed keys is a loss for users for absolutely no benefit. As an Armory user since 2012 I hope the Armory team could take this major update as an opportunity to support compressed key, at least in segwit. @achow101 I don’t know how Armory’s wallet exactly works but I could imagine that you will store and process the key uncompressed internally, and have one more step to compress it when generating the address. Since you need new address encoding and decoding codes anyway, this should be just a few lines of codes on top of that.

achow101 commented at 2:56 pm on October 9, 2016: member

@jl2012 Current releases of Armory actually completely break with segwit because they read the block files from the disk. Our next release already has that part fixed.

With compressed keys, the plan is to store the keys uncompressed as we do now and then convert them to compressed for any segwit scripts. It is indeed just a few extra lines between getting the key and putting it into the script.

mmgen commented at 8:31 pm on October 9, 2016: none

@sipa @TheBlueMatt @laanwj @gmaxwell @jl2012 Dropping uncompressed public key support with segwit is dangerous and can easily lead to people creating unspendable outputs, losing their money and being very upset. Consider the following scenario:

~~User generates a key/address pair with vanitygen, which uses uncompressed public keys.~~
~~User creates a transaction with RPC ‘createrawtransaction’ spending to the uncompressed address, then signs and broadcasts the tx. The output of this transaction is now unspendable.~~

jl2012 commented at 9:00 pm on October 9, 2016: contributor

@mmgen : To make it clear this PR affects ONLY segwit outputs. Your wallet, which apparently does not yet support segwit, is totally unaffected.

Vanitygen (https://github.com/samr7/vanitygen), which is not updated since 2012, will definitely be unaffected.

ELI5:

If you are dealing with addresses starting with 1, you are definitely unaffected.
If you are dealing with addresses starting with 3 and you have not tried to do anything to support segwit, you are very very likely unaffected.
If you are dealing with raw scriptPubKey and you don’t fully understand what you are doing, you are already at very high risks.

mmgen commented at 9:33 pm on October 9, 2016: none

@jl2012

To make it clear this PR affects ONLY segwit outputs.

My wallet generates addresses using vanitygen and uses Bitcoin Core RPC as a backend for tx creation. If Core’s ‘createrawtransaction’ creates segwit outputs by default, then my wallet is affected, unless you can convice me otherwise.

achow101 commented at 9:37 pm on October 9, 2016: member

@mmgen createrawtransaction will not create transactions with segwit outputs. If an address begins with a 1 (as vanitygen addresses do), then it will create a P2PKH output because that is what an address beginning with a 1 means.

mmgen commented at 9:41 pm on October 9, 2016: none

@achow101 Thanks! That answers my question and puts my fears to rest.

achow101 commented at 9:44 pm on October 9, 2016: member

It isn’t necessary to document that IMO. By definition, an address with a 1 means the output is P2PKH, an address with a 3 means the output is P2SH (with any script you want). Segwit outputs have no address so you should not confuse them with any address.

mmgen commented at 9:49 pm on October 9, 2016: none

@achow101

Segwit outputs have no address

Interesting. How is that possible? How does one receive payment then?

achow101 commented at 9:53 pm on October 9, 2016: member

@mmgen we’re getting OT… If you have questions on segwit, go to the IRC channel.

Segwit scripts are nested inside P2SH addresses. Like P2SH scripts, they can be put into the output script, but most wallets aren’t going to recognize that so you have to do it by hand. Remember, addresses are just abstractions.

jl2012 commented at 4:36 am on October 10, 2016: contributor

@mmgen @achow101 : If you are using an “1” address, Bitcoin Core wallet will always send it in the old way, to a scriptPubKey that does not support segwit.

However, I won’t be surprised if some wallet devs will try to ignore the spec and convert an “1” address to a segwit output. This is extremely dangerous with or without this PR. There are thousands of ways people could do stupid things like this and it’s very difficult to prevent it at protocol level. Please stop it if you find anyone trying to do this.

mmgen commented at 12:11 pm on October 10, 2016: none

@jl2012 Thanks much to both you and @achow101 for your help. My wallet deals only with ‘1’ addresses, so it turns out I’m not affected.

And of course, my concerns were silly in the first place, since obviously the core devs aren’t going to introduce a change in the protocol that would turn existing addresses into ‘black holes’.

It would be great if info like what you’ve provided in this discussion were published in a Segwit FAQ for the benefit of idiots like me. Or better yet, if someone created a Segwit Developer’s Guide. I, for one, would like to make my wallet segwit capable, but I don’t know how to go about that or whether it will require major changes. The wallet is basically a front-end for Bitcoin Core, generating key/address pairs independently from a seed but using the Core wallet for tracking addresses and Core RPC commands for creating, signing and sending transactions.

Edit: BIP 143 answered a lot of my questions. A FAQ could point developers to BIPS 141, 143 and 144.

jl2012 force-pushed on Oct 10, 2016

jl2012 commented at 12:31 pm on October 10, 2016: contributor

Squashed and rebased

jl2012 renamed this:
~~Add several policy limits for segwit scripts~~
Add several policy limits and disable uncompressed keys for segwit scripts
on Oct 10, 2016

laanwj commented at 1:32 pm on October 10, 2016: member

re-utACK 69e8362

It would be great if info like what you’ve provided in this discussion were published in a Segwit FAQ for the benefit of idiots like me.

Yes, this would definitely be useful. There’s a segwit benefits FAQ, but no technical one. There are so many things that even hard-core devs get confused once in a while. But indeed this is not the place for discussing this, maybe open an issue for the bitcoincore.org site.

achow101 commented at 1:38 pm on October 10, 2016: member

@mmgen @laanwj changes that happen here should probably be documented on https://bitcoincore.org/en/segwit_wallet_dev/ (and all the other stuff that has happened that isn’t there). Also that page should be a bit more accessible.

mmgen commented at 2:01 pm on October 10, 2016: none

Thanks, @laanwj @achow101. Will take under advisement.

sdaftuar commented at 11:08 am on October 12, 2016: member

A couple of issues with the wallet changes came up during in-person code review the last couple days; specifically there are some open issues with IsMine() and how addwitnessaddress() is using it that need to be addressed before this should be merged. Patch in progress…

jl2012 force-pushed on Oct 14, 2016

jl2012 commented at 9:20 pm on October 14, 2016: contributor

The commits e6110a0 and f275d55 are new to cover many edge cases we found, in order to make uncompressed keys in segwit scripts invisible in wallet for both spendable and watch-only keys

jl2012 force-pushed on Oct 15, 2016

jl2012 commented at 9:51 am on October 15, 2016: contributor

More tests added to cover premature addwitnessaddress. addwitnessaddress should fail if the P2SH version of the given address is unknown

sipa commented at 1:18 pm on October 16, 2016: member

utACK 61c15dfad3e94d2c30e2a6aee96971017c287ff1

in src/policy/policy.cpp: in 61c15dfad3 outdated

167+            continue;
168+
169+        const CTxOut &prev = mapInputs.GetOutputFor(tx.vin[i]);
170+
171+        std::vector <std::vector<unsigned char> > vSolutions;
172+        txnouttype whichType;

TheBlueMatt commented at 2:46 pm on October 16, 2016:

You either need to initialize this or check the return value of Solver.

TheBlueMatt commented at 3:16 pm on October 16, 2016:

Also, probably Solver should init the type to NONSTD always…

TheBlueMatt commented at 3:17 pm on October 16, 2016:

Also, in this case, you’re using Solver to just check IsPayToScriptHash(), so maybe do that.

jl2012 commented at 3:54 pm on October 16, 2016:

fixed and squashed

Add standard limits for P2WSH with tests 3ade2f64cf

Require compressed keys in segwit as policy and disable signing with uncompressed keys for segwit scripts 4c0c25a604

Make test framework produce lowS signatures 9f0397aff7

[qa] Add tests for uncompressed pubkeys in segwit b811124202

in src/script/ismine.cpp: in e6110a0acb outdated

76     case TX_WITNESS_V0_KEYHASH:
77+    {
78+        if (!keystore.HaveCScript(CScriptID(CScript() << OP_0 << vSolutions[0]))) {
79+            // We do not support bare witness outputs unless the P2SH version of it would be
80+            // acceptable as well. This protects against matching before segwit activates or
81+            // uncompressed pubkey matching. This also applies to the P2WSH case.

instagibbs commented at 3:52 pm on October 16, 2016:

How does this protect against uncompressed pubkey matching?

sipa commented at 3:57 pm on October 16, 2016:

Because it means the keystore needs to contain a witness-specific CScript for this branch not to trigger.

jl2012 commented at 4:01 pm on October 16, 2016:

If the P2SH version of a witness program is unknown, we don’t consider that is “mine”. That means the user has to explicitly import the P2SH version with importaddress or addwitnessaddress, and the latter is disabled by default before segwit activation. So it is not possible to confuse users by picking random pubkey on the blockchain and send some dust to a corresponding witness output, which is unspendable before segwit activation.

But I think now this is not for uncompressed key protection. The uncompressed key comment is just a leftover of an earlier version

jl2012 commented at 4:03 pm on October 16, 2016:

I think @instagibbs is correct that it is no longer needed for uncompressed pubkey matching. But this is still needed for my other reason provided.

jl2012 force-pushed on Oct 16, 2016

in src/script/ismine.h: in 26bd36ba64 outdated

27@@ -28,7 +28,9 @@ enum isminetype
28 /** used for bitflags of isminetype */
29 typedef uint8_t isminefilter;
30 
31-isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey);
32-isminetype IsMine(const CKeyStore& keystore, const CTxDestination& dest);
33+isminetype IsMine(const CKeyStore& keystore, const CScript& scriptPubKey, bool& isInvalid, SigVersion = SIGVERSION_BASE);

instagibbs commented at 4:06 pm on October 16, 2016:

Could we get an explanation of the different arguments here? isInvalid seems to have a particular meaning and use that is non-obvious, so it would benefit reviewers.

instagibbs commented at 4:22 pm on October 16, 2016:

ie If it’s only going to be checking for uncompressed pubkeys, we should say so, or rename the variable.

jl2012 commented at 4:26 pm on October 16, 2016:

I think this could be generalized to a similar situation in the future, if we have valid script in one SIGVERSION which is invalid in another

instagibbs commented at 4:34 pm on October 16, 2016:

isInvalidForSigVersion or something similar might make sense then.

in src/script/ismine.cpp: in 26bd36ba64 outdated

80+            // acceptable as well. This protects against matching before segwit activates or
81+            // uncompressed pubkey matching. This also applies to the P2WSH case.
82+            break;
83+        }
84+        isminetype ret = ::IsMine(keystore, GetScriptForDestination(CKeyID(uint160(vSolutions[0]))), isInvalid, SIGVERSION_WITNESS_V0);
85+        if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))

instagibbs commented at 4:08 pm on October 16, 2016:

this isInvalid check is to prevent the wallet from “finding” funds that have WatchOnly with uncompressed pubkey in Segwit via https://github.com/bitcoin/bitcoin/pull/8499/commits/26bd36ba64cd090c862ca19d64e8e73ee79184d2#diff-1dbee12e01d094e7366545ec024c5041R153?

jl2012 commented at 4:18 pm on October 16, 2016:

yes, both watchonly and spendable uncompressed keys in segwit scripts should be caught by this logic.

Fix ismine and addwitnessaddress: no uncompressed keys in segwit 248f3a76a8

test segwit uncompressed key fixes 9260085377

in src/script/ismine.cpp: in 26bd36ba64 outdated

101@@ -67,21 +102,24 @@ isminetype IsMine(const CKeyStore &keystore, const CScript& scriptPubKey)
102         CScriptID scriptID = CScriptID(uint160(vSolutions[0]));
103         CScript subscript;
104         if (keystore.GetCScript(scriptID, subscript)) {
105-            isminetype ret = IsMine(keystore, subscript);
106-            if (ret == ISMINE_SPENDABLE)
107+            isminetype ret = IsMine(keystore, subscript, isInvalid);
108+            if (ret == ISMINE_SPENDABLE || ret == ISMINE_WATCH_SOLVABLE || (ret == ISMINE_NO && isInvalid))

instagibbs commented at 4:18 pm on October 16, 2016:

isInvalid here will never be true. This value is only set to true if sigversion != SIGVERSION_BASE. This call has SIGVERSION_BASE as its default argument in the line above.

jl2012 commented at 4:20 pm on October 16, 2016:

If the subscript is a P2WPKH or P2WSH, the sigversion will become SIGVERSION_WITNESS_V0

jl2012 force-pushed on Oct 16, 2016

instagibbs commented at 4:36 pm on October 16, 2016: member

Sync is failing to complete for some reason in p2p-segwit.py

utACK fixes at 26bd36ba64cd090c862ca19d64e8e73ee79184d2 with only a couple nits

jl2012 commented at 4:36 pm on October 16, 2016: contributor

Comments added and edited. By the way, it seems the p2p-segwit.py would fail randomly

File “./qa/rpc-tests/p2p-segwit.py”, line 2000, in run_test self.test_non_standard_witness() File “./qa/rpc-tests/p2p-segwit.py”, line 1909, in test_non_standard_witness self.std_node.announce_tx_and_wait_for_getdata(p2sh_txs[1]) File “./qa/rpc-tests/p2p-segwit.py”, line 108, in announce_tx_and_wait_for_getdata self.wait_for_getdata(timeout) File “./qa/rpc-tests/p2p-segwit.py”, line 98, in wait_for_getdata self.sync(test_function, timeout) File “./qa/rpc-tests/p2p-segwit.py”, line 82, in sync raise AssertionError(“Sync failed to complete”)

remove redundant tests in p2p-segwit.py 67d6ee1e36

jl2012 commented at 7:24 pm on October 16, 2016: contributor

I’m removing the tests that causing random sync fail. A similar test is done already to show that policy-rejection won’t blind a node to the tx, so I think it’s just redundant to try it again.

TheBlueMatt commented at 9:02 pm on October 16, 2016: member

utACK 67d6ee1e3679504f46473fe0818970565ff3b137

laanwj merged this on Oct 17, 2016

laanwj closed this on Oct 17, 2016

laanwj referenced this in commit 53133c1c04 on Oct 17, 2016

btcdrak commented at 11:56 am on October 17, 2016: contributor

utACK 67d6ee1e3679504f46473fe0818970565ff3b137

laanwj referenced this in commit 540413d995 on Oct 17, 2016

laanwj referenced this in commit 821f3e6751 on Oct 17, 2016

laanwj referenced this in commit b4b85279a9 on Oct 17, 2016

laanwj referenced this in commit 908fced296 on Oct 17, 2016

laanwj referenced this in commit 4ec21e8a64 on Oct 17, 2016

laanwj referenced this in commit fef7b46841 on Oct 17, 2016

laanwj referenced this in commit 9777fe1272 on Oct 17, 2016

laanwj commented at 12:21 pm on October 17, 2016: member

Backported in #8916

laanwj removed the label Needs backport on Oct 17, 2016

instagibbs commented at 1:49 pm on October 17, 2016: member

post-merge ACK https://github.com/laanwj/bitcoin/commit/9777fe12722ddab7b1b17df11077d448c7a25006

str4d referenced this in commit 7bc673977a on Nov 14, 2019

str4d referenced this in commit ebd0669816 on Dec 3, 2019

str4d referenced this in commit 82beb18901 on Dec 4, 2019

furszy referenced this in commit bf132c4e6a on Jun 18, 2020

furszy referenced this in commit fe31b31eec on Jun 18, 2020

UdjinM6 referenced this in commit 1403f214de on May 21, 2021

UdjinM6 referenced this in commit 73e5966f60 on Jun 5, 2021

UdjinM6 referenced this in commit 4ae641ca18 on Jun 5, 2021

DrahtBot locked this on Sep 8, 2021

Add several policy limits and disable uncompressed keys for segwit scripts #8499