Enforce mandatory softfork flags for segwit block/tx #8635

pull jl2012 wants to merge 1 commits into bitcoin:master from jl2012:mandatorysegwitflags changing 4 files +23 −8

jl2012 commented at 9:16 AM on August 31, 2016: contributor

(untested) If a node is able to relay a block/tx with witness, it must know all existing script softfork rules: BIP66, 65, 112, 141, 143. All related verify flags should be mandatory for such blocks/txs
jl2012 force-pushed on Aug 31, 2016
fanquake added the label TX fees and policy on Aug 31, 2016
in src/main.cpp:None in b47f7a07dc outdated
```
2407 | @@ -2405,6 +2408,11 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2408 |      std::vector<std::pair<uint256, CDiskTxPos> > vPos;
2409 |      vPos.reserve(block.vtx.size());
2410 |      blockundo.vtxundo.reserve(block.vtx.size() - 1);
2411 | +    bool fHaveWitness = false;
```
sipa commented at 12:12 PM on September 5, 2016:

I believe this is harmless in this specific case, but not necessary, and makes reasoning about forking risks much harder. The flags enabled for block validation should purely depend on consensus rules.

sipa commented at 3:36 PM on September 5, 2016:

Actually, this shouldn't even have any effect. Since flags in the call to CheckInputs below only contains consensus flags, it should never overlap with STANDARD_NOT_MANDATORY_SEGWIT_VERIFY_FLAGS.

in src/main.cpp:None in b47f7a07dc outdated

1499 |              // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we
1500 |              // need to turn both off, and compare against just turning off CLEANSTACK
1501 |              // to see if the failure is specifically due to witness validation.
1502 | -            if (CheckInputs(tx, state, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true) &&
1503 | -                !CheckInputs(tx, state, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true)) {
1504 | +            if (CheckInputs(tx, state, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, NULL, fHaveWitness) &&

sipa commented at 3:22 PM on September 5, 2016:

This piece of code is removed in #8525. It may be worth it to rebase on top of that, to reduce the changes?

jl2012 force-pushed on Sep 8, 2016
jl2012 force-pushed on Sep 8, 2016
jl2012 commented at 1:44 PM on September 8, 2016: contributor

Rebased and removed the codes for block validation
Enforce mandatory softfork flags for segwit block/tx 234d2e63cb
jl2012 force-pushed on Sep 8, 2016
jl2012 commented at 5:00 PM on September 8, 2016: contributor

Is this a target of 0.13.1?
laanwj added this to the milestone 0.13.1 on Sep 8, 2016
MarcoFalke added the label Needs backport on Sep 9, 2016
instagibbs commented at 5:50 PM on September 12, 2016: member

Looks like you snagged a couple commits from another PR in here. That intentional?

Following this logic, any follow-up softfork from Core codebase would assume previous softforks will be followed then?
jl2012 commented at 6:14 PM on September 12, 2016: contributor

@instagibbs I just rebased on top of that PR. It was not merged at that time. Should I rebase again?

This one is special. Since segwit is a completely new tx format, there is no excuse for not respecting any known softforks, which have been enforced for months.

Following this logic, it should have been mandatory for version 2 transactions to follow all existing softforks (expect segwit), since no one should produce or relay v2 tx without knowing BIP68.

However, before the CSV softfork, we couldn't simply enforce mandatory flags since no change had been made in transaction version or format.
jonasschnelli removed this from the milestone 0.13.1 on Sep 15, 2016
MarcoFalke removed the label Needs backport on Sep 20, 2016
TheBlueMatt commented at 8:42 PM on July 11, 2017: member

Needs rebase. Not sure what the real value of this is, though...why is it a serious offense to relay such blocks? (we'd need careful to avoid any potential network-splitting issues with compact-fast-relay).
jl2012 commented at 7:11 AM on October 6, 2017: contributor

close due to lack of interests
jl2012 closed this on Oct 6, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels