acceptnonstdtxn option to skip (most) "non-standard transaction" checks, for testnet/regtest only

luke-jr commented at 3:43 am on June 24, 2015: member

Inspired by #6255, this makes it possible to enable IsStandard for regtest/testnet (but doesn’t allow disabling it on mainnet), while remaining compatible with long-standing options in other node software.

luke-jr force-pushed on Jun 24, 2015

in src/init.cpp: in 811bff3584 outdated

395@@ -396,6 +396,7 @@ std::string HelpMessage(HelpMessageMode mode)
396     strUsage += HelpMessageOpt("-testnet", _("Use the test network"));
397 
398     strUsage += HelpMessageGroup(_("Node relay options:"));
399+    strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf(_("Relay and mine \"non-standard\" transactions (testing only; default: %u)"), 0));

petertodd commented at 7:52 pm on June 26, 2015:

This is confusing, as it doesn’t make clear that the default is true on testnet and regtest.

luke-jr commented at 6:42 pm on June 27, 2015:

How would you improve it?

unknown commented at 3:20 pm on June 28, 2015:

One possibility: "Relay and mine \"non-standard\" transactions (testnet/regtest only, default: true)"

Edit: Want to clarify that a bit, I believe petertodd is confused by the default flag displayed (‘0’) when the actual flag setting code uses !Params().RequireStandard() defaulting true for testnet/regtest

petertodd commented at 9:19 pm on June 28, 2015:

@faizkhan00 Yup, that’s exactly the issue I’m worried about.

I think your wording is fine, modulo s/true/1/

luke-jr commented at 5:39 am on June 29, 2015:

Fixed.

luke-jr force-pushed on Jun 29, 2015

petertodd commented at 0:43 am on June 30, 2015: contributor

Modulo wording nit, tested ACK.

I’ll close #6255 if this gets merged.

luke-jr force-pushed on Jun 30, 2015

luke-jr commented at 11:34 pm on June 30, 2015: member

Changed…

luke-jr force-pushed on Jun 30, 2015

luke-jr force-pushed on Jul 1, 2015

in src/init.cpp: in 35853d9055 outdated

395@@ -396,6 +396,7 @@ std::string HelpMessage(HelpMessageMode mode)
396     strUsage += HelpMessageOpt("-testnet", _("Use the test network"));
397 
398     strUsage += HelpMessageGroup(_("Node relay options:"));
399+    strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf(_("Relay and mine \"non-standard\" transactions (%sdefault: %u)"), _("testnet/regtest only; "), !Params(CBaseChainParams::TESTNET).RequireStandard()));

laanwj commented at 7:07 am on July 3, 2015:

This should be part of the debugging options (if(showDebug)). Consequently, also the message should be left un-translated.

in src/init.cpp: in 35853d9055 outdated

808@@ -808,6 +809,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
809             return InitError(strprintf(_("Invalid amount for -minrelaytxfee=<amount>: '%s'"), mapArgs["-minrelaytxfee"]));
810     }
811 
812+    fRequireStandard = !GetBoolArg("-acceptnonstdtxn", !Params().RequireStandard());
813+    if (Params().RequireStandard() && !fRequireStandard)
814+        return InitError(strprintf(_("acceptnonstdtxn is not currently supported for %s chain"), chainparams.NetworkIDString()));

laanwj commented at 7:08 am on July 3, 2015:

Same here - no need to get translators involved in translating for a testing/debugigng-only option.

laanwj commented at 7:09 am on July 3, 2015: member

utACK apart rom translation nits

luke-jr force-pushed on Jul 3, 2015

luke-jr commented at 7:45 am on July 3, 2015: member

Removed translation tags.

acceptnonstdtxn option to skip (most) "non-standard transaction" checks, for testnet/regtest only 0c376340a4

in src/init.cpp: in 81ec58c2c0 outdated

395@@ -396,6 +396,7 @@ std::string HelpMessage(HelpMessageMode mode)
396     strUsage += HelpMessageOpt("-testnet", _("Use the test network"));
397 
398     strUsage += HelpMessageGroup(_("Node relay options:"));
399+    strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !Params(CBaseChainParams::TESTNET).RequireStandard()));

laanwj commented at 7:53 am on July 3, 2015:

Also add if(showDebug)

luke-jr force-pushed on Jul 3, 2015

luke-jr commented at 8:29 am on July 3, 2015: member

showDebug done

laanwj merged this on Jul 3, 2015

laanwj closed this on Jul 3, 2015

laanwj referenced this in commit d0a10c1959 on Jul 3, 2015

jtimon commented at 9:46 pm on July 6, 2015: contributor

How did I missed this while being so insistent with @luke-jr and @petertodd about #5180 …this is the wrong way to expose this…instead of -acceptnonstdtxn you would have just used -policy=test.

in src/init.cpp: in 0c376340a4

804@@ -803,6 +805,10 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
805             return InitError(strprintf(_("Invalid amount for -minrelaytxfee=<amount>: '%s'"), mapArgs["-minrelaytxfee"]));
806     }
807 
808+    fRequireStandard = !GetBoolArg("-acceptnonstdtxn", !Params().RequireStandard());
809+    if (Params().RequireStandard() && !fRequireStandard)

jtimon commented at 9:55 pm on July 6, 2015:

The semantics are wrong, why would the user be prevented from overrriding a particular chain’s default? This check is completely unnecessary. Also, any reason to name the variable amd the option as opposites (instead of both RequireStandard or both acceptnonstdtxn)?

petertodd commented at 10:38 pm on July 6, 2015:

It’s actually pretty dangerous to override the IsStandard() check on mainnet if you are a miner; you can end up creating blocks that take unreasonable amounts of time to verify, losing money.

Best go force people to edit the source code to so that.

Anyway, this is mainly useful for testing applications, so no need IMO to make this into a complex game of policy stuff when a simple and intuitive flag works.

jtimon commented at 6:25 pm on July 7, 2015:

@petertodd Then miners shouldn’t use this option and it can be documented. There’s more users besides miners who may be interested in accepting non-std transactions and give the choice to the users instead of endlessly discuss the best default or how hard should it be to change it for less and more technical users. This particularly surprises me because @luke-jr once said “the testing policy is actually a sane policy to run in production”, supposedly as an argument in favor of “-acceptnonstdtxn=0/1” over “policy=standard/test”. Actually, prohibiting the option in certain chains adds more code complexity and couples CStandardPolicy to Params() forever. I’m sure nobody would be in favor of prohibiting the -datacarriersize option for the main chain. https://github.com/bitcoin/bitcoin/compare/master...jtimon:safe-configurable-policy “Bitcoin core, now with configurable policy that is only configurable for testnet and regtest”.

petertodd commented at 4:41 am on July 9, 2015:

@jtimon There’s DoS attacks that IsStandard() prevents.

Anyway, without peers that also accept your non-default transactions, changing policy locally is pretty pointless; solve that issue first. One idea would be a hashcash-based relay network, maybe using Bitmessage if you want to get it implemented quickly.

jtimon commented at 6:59 am on July 9, 2015:

@jtimon There’s DoS attacks that IsStandard() prevents.

There’s DoS attacks that minRelayTxFee prevents, yet still -minrelaytxfee=1 is allowed (and -minrelaytxfee=0 should be allowed as well IMO).

Anyway, without peers that also accept your non-default transactions, changing policy locally is pretty pointless; solve that issue first.

No, I’m fixing this first. Solve that yourself if you want to. I’ve been wanting to expose this option as -policy=test for very long (#5180). Then 2 competing PRs appear and one of them gets merged very fast. Nobody asked me for review (even though I had been very insistent demanding review for #5180 myself) and thus I wasn’t able to nit the proposals and thus what was merged us wrong, and I can fix some of the mistakes for free in #6068. #5180 couldn’t be fairly compared with #6329 without some previous steps (#6068 among them) that are taking forever to review, but whatever, what’s done it’s done.

Anyway, if “changing policy locally is pretty pointless” then nobody will do it and we don’t need to maintain this additional stupid restriction, do we?

jtimon commented at 7:01 am on July 9, 2015:

Anyway, enough discussion. Please ack/nack https://github.com/jtimon/bitcoin/commit/2d71478a68d3a2c6cb5f5210f76b37d1f60a22cf in #6068 so that we can move on.

random-zebra referenced this in commit 2ab948e1ad on May 4, 2021

MarcoFalke locked this on Sep 8, 2021

acceptnonstdtxn option to skip (most) “non-standard transaction” checks, for testnet/regtest only #6329