New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jul 3, 2015

Conversation

Projects
None yet
4 participants
@luke-jr
Member

luke-jr commented Jun 24, 2015

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.

@petertodd

View changes

Show outdated Hide outdated src/init.cpp
@@ -396,6 +396,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-testnet", _("Use the test network"));
strUsage += HelpMessageGroup(_("Node relay options:"));
strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf(_("Relay and mine \"non-standard\" transactions (testing only; default: %u)"), 0));

This comment has been minimized.

@petertodd

petertodd Jun 26, 2015

Contributor

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

@petertodd

petertodd Jun 26, 2015

Contributor

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

This comment has been minimized.

@luke-jr

luke-jr Jun 27, 2015

Member

How would you improve it?

@luke-jr

luke-jr Jun 27, 2015

Member

How would you improve it?

This comment has been minimized.

@ghost

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

@ghost

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

This comment has been minimized.

@petertodd

petertodd Jun 28, 2015

Contributor

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

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

@petertodd

petertodd Jun 28, 2015

Contributor

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

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

This comment has been minimized.

@luke-jr

luke-jr Jun 29, 2015

Member

Fixed.

@luke-jr

luke-jr Jun 29, 2015

Member

Fixed.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jun 30, 2015

Contributor

Modulo wording nit, tested ACK.

I'll close #6255 if this gets merged.

Contributor

petertodd commented Jun 30, 2015

Modulo wording nit, tested ACK.

I'll close #6255 if this gets merged.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 30, 2015

Member

Changed...

Member

luke-jr commented Jun 30, 2015

Changed...

@laanwj

View changes

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

This comment has been minimized.

@laanwj

laanwj Jul 3, 2015

Member

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

@laanwj

laanwj Jul 3, 2015

Member

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

@laanwj

View changes

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

This comment has been minimized.

@laanwj

laanwj Jul 3, 2015

Member

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

@laanwj

laanwj Jul 3, 2015

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 3, 2015

Member

utACK apart rom translation nits

Member

laanwj commented Jul 3, 2015

utACK apart rom translation nits

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 3, 2015

Member

Removed translation tags.

Member

luke-jr commented Jul 3, 2015

Removed translation tags.

@laanwj

View changes

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

This comment has been minimized.

@laanwj

laanwj Jul 3, 2015

Member

Also add if(showDebug)

@laanwj

laanwj Jul 3, 2015

Member

Also add if(showDebug)

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 3, 2015

Member

showDebug done

Member

luke-jr commented Jul 3, 2015

showDebug done

@laanwj laanwj merged commit 0c37634 into bitcoin:master Jul 3, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jul 3, 2015

Merge pull request #6329
0c37634 acceptnonstdtxn option to skip (most) "non-standard transaction" checks, for testnet/regtest only (Luke Dashjr)
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 6, 2015

Member

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.

Member

jtimon commented Jul 6, 2015

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.

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

This comment has been minimized.

@jtimon

jtimon Jul 6, 2015

Member

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

@jtimon

jtimon Jul 6, 2015

Member

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

This comment has been minimized.

@petertodd

petertodd Jul 6, 2015

Contributor

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.

@petertodd

petertodd Jul 6, 2015

Contributor

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.

This comment has been minimized.

@jtimon

jtimon Jul 7, 2015

Member

@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. master...jtimon:safe-configurable-policy
"Bitcoin core, now with configurable policy that is only configurable for testnet and regtest".

@jtimon

jtimon Jul 7, 2015

Member

@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. master...jtimon:safe-configurable-policy
"Bitcoin core, now with configurable policy that is only configurable for testnet and regtest".

This comment has been minimized.

@petertodd

petertodd Jul 9, 2015

Contributor

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

@petertodd

petertodd Jul 9, 2015

Contributor

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

This comment has been minimized.

@jtimon

jtimon Jul 9, 2015

Member

@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

jtimon Jul 9, 2015

Member

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

This comment has been minimized.

@jtimon

jtimon Jul 9, 2015

Member

Anyway, enough discussion. Please ack/nack jtimon@2d71478 in #6068 so that we can move on.

@jtimon

jtimon Jul 9, 2015

Member

Anyway, enough discussion. Please ack/nack jtimon@2d71478 in #6068 so that we can move on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment